VulkanSceneGraph-1.0 TODO list

[Robert Osfield]

This morning I compiled a TODO list for the VulkanSceneGraph-1.0, most of the items I'll be doing personally. but there are a few places where community help/expertise would be appreciated.  I've put stuff into MUST DO, Should do, could do categories to make sure I/we know what items have to be done prior to the release through items that aren't critical so can be dropped if time is short.

If you can help with any of these items please let me know on this thread.

MUST DO:

    Code review of core VSG headers (underway, so far reviewed core headers)
    Code review of core VSG .cpp's:
    Code review of CMake build system

    Build and runtime testing against all main platforms.

    Fix all reported bugs and build issues in core VSG

    Discuss moving forum to Githib
    Review Githuib.io website
    Organize Launch party

    Make VulkanSceneGraph-1.0 branch
    Make VulkanSceneGraph-1.0.0 tag

Should do:

    Fix all reported bugs and build issues in main ancillary projects
    Review execution of all vsgExamples programs
    Build doxygen documentation - where to host, can it be done via github Actions?

    Make vsg::allocator_affinity_nodes more general

    Press release and Tweet and LinkedIn posts for it.

could do:

    Code review of ancillary projects
    ShaderCompileSettings::defines. std::set -> vsg::map<>
    Data::Layout -> Data::Properties
    Make Data::Properites a public member
    Move GLSL reader from vsgXchange into core VSG
    Implement handling of vsg::WriteError/WriteResult
    KHronos Mesh shaders
    Create example for vsg::External

    Code review of ancillary projects:

        vsgXchange
        osg2vsg
        vsgImGui
        vsgExamples

        vsgQt
        vsgSDL
        vsgWX
        vsgVR
        vsgPoints

        vsgopenmw
        VulkanPBRT

[Robert Osfield]

We have couple of Github actions that haven't been run in a while, so I ran them an saw issues, but I think these are due to the .yml files needing to be updated.

First up is the Cygwin build doesn't run, so I've created an Issue for it:

    https://github.com/vsg-dev/VulkanSceneGraph/issues/534

Second the Doxygen build is failing:

    https://github.com/vsg-dev/VulkanSceneGraph/issues/535

I don't expertise  on Github's Action/workflows so would appreciate help with resolve them.

Thanks,

Robert.

[Robert Osfield]

I have now completed my review of the include/vsg/maths headers.

I have added doxygen comments and moved a number of general maths functions from include/vsg/maths/transform.h into a new include/vsg/maths/common.h header, this is where GLM puts similar functions so it seems like a good enough naming.

    https://github.com/vsg-dev/VulkanSceneGraph/pull/536

The maths/transform.h header now includes maths/common.h so your existing code bases need to be updated.

[Robert Osfield]

I have now completed code review of include/vsg/io, so that's include/vsg/core, maths and io now completed. Mostly it's just a case of adding doxygen comments.

Changes just merged:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/537

This means I've now reviewed 9 thousand out of 23 thousand lines of code found in the include/vsg headers,  I've done a few other tasks today so if I'm really lucky/determined I may be able to complete the review of the rest of the headers tomorrow.

But... oh boy it's slow and tedious work, on a more positive note I'm pretty happy with the code, it all looks pretty coherent and sensible, following the minimal and complete guide pretty well.

[Robert Osfield]

I have now completed the review and refinement of the 24 include/vsg/vk headers, so that's another 3000 lines of code down.  Alas it's taken me all morning so I'm not exactly flying through the code base. 

Changes checked in are mostly doxygen comments and reordering of methods to make them more consistent across classes, but I also changes a couple of methods names to make them more consistent with other classes, and moved a SwapchainImage helper class from public scope into the .cpp.  I don't expect these changes will affect anyone's application builds as these changes are mostly not ones used in applications, but internally by the VSG itself to set up all the Vulkan objects used.  Changes are:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/538

I have now reviewed and refined 12 thousand out of 23 thousands lines of code in the headers.  I had hoped that I'd be able to complete this review today but as it's now midday and it's taken me a day and half to complete a review of just over half....

So it's likely this review is going to slip into the weekend.

[Robert Osfield]

My review of include/vsg/state headers is now finished, again mostly just addition of doxygen comments, with a few tweaks for consistency:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/539

That's 14687 header lines reviewed out of 23291 header lines.

[Robert Osfield]

I've run out of steam, so I am calling it a day.  Large scale code reviews are tough physically and mentally.  Normal coding is just so much easier.

My final batch of changes, against mostly adding/improving doxygen comments, this time to the about 1/3 the files include/vsg/commands:

   https://github.com/vsg-dev/VulkanSceneGraph/pull/541/files

I know doing the large code review is the right thing to do for the project but oh boy does it take it's pound of flesh.

[Robert Osfield]

I have just finished my code review of the include/vsg/command headers and checked in doxygen comment additions:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/542/files

That's 16 thousand lines reviewed, another 7 thousand lines more of headers to review...

[Robert Osfield]

Yesterday I had some time so I tackled one of the "could do" items on my TODO list (I presented this at the top of this thread), the items I completed was:

     Move GLSL reader from vsgXchange into core VSG

Changes to the VSG:

     https://github.com/vsg-dev/VulkanSceneGraph/pull/544/files

Changes to vsgXchange:

    https://github.com/vsg-dev/vsgXchange/pull/121/files

I wanted to move the GLSL reader/writer into the core VSG as the you can compile GLSL shaders with the core VSG, if you link to GLSLang, but not being able to easily load GLSL files to feed it just seemed unbalanced, especially as it takes much more code to do the shader compile than it does to read/write GLS - the vsg::glsl is a tiny little class.

This also means you can now write VSG programs that don't need to link to vsgXchange when all you need to loading native formats like .vsgt and .vsgb, and .tile, and the ancillary ones like .spv and now all the GLSL ones, so the later two types can be thought as native as well. 

So you can now do:

   auto vertexShader = vsg::read_cast<vsg::ShaderSet>("shader.vert");

    

--

As my attempt to just do code reviews for the second half of last week was so exhausting this week I will weave blocks of code reviews with other items on the MUST DO, Should Do and could do lists.  This means the core reviews will progress slower but I'll keep my sanity.

Cheers,

Robert.

[Robert Osfield]

I have completed the code review of the include/vsg/nodes headers, I checked them directly into VSG master:

    https://github.com/vsg-dev/VulkanSceneGraph/commit/1147bcd01d263f2a15c6b969e20cccede9fd48ab

    https://github.com/vsg-dev/VulkanSceneGraph/commit/90c3165b3781884df4560affe86591b667effd68

    https://github.com/vsg-dev/VulkanSceneGraph/commit/611e964808be2ba66352a47f52d9f77a0ffb9071

That's 17490 lines reviewed, with lots of Doxygen comments added.

[Robert Osfield]

I have completed the "MUST DO" item : Discuss moving forum to Githib.  I didn't receive any dissent so have gone ahead and enable the Discussion forum on VulkanSceneGraph repo:

    https://github.com/vsg-dev/VulkanSceneGraph/discussions

I have also completed the "could do" item:  Rename assimp.vert and assimp_*.frag standard.vert and standard_*.frag.

The changes are made to vsgExamples:

    https://github.com/vsg-dev/vsgExamples/pull/180

[Robert Osfield]

I have completed my review of include/vsg/threading. again just required addition of doxygen comments.

   https://github.com/vsg-dev/VulkanSceneGraph/pull/547

Feels like I've been reviewing headers for a long time now, but still have the following include/vsg headers directories left to complete:

        traversal
        ui
        utils
        viewer
        rtx
        platform

Really does take a lot of class/functionality to make a reasonably feature complete scene graph....

[Robert Osfield]

Two of my "could do" for VSG-1.0 items are:

    Data::Layout -> Data::Properties
    Make Data::Properites a public member

I discussed my intention to do this when I introduced the Layout.dataVariance capability into vsg::Data, but didn't have time originally to get this done, but with an extra week I've been able to make this change. Consider it provisional as changing this bit of the API so close to a stable release is risky.  The changes I've made so far are:

    https://github.com/vsg-dev/VulkanSceneGraph/compare/Data_Properties

My intention is deprecated but not remove the old Data::setLayout/getLayout methods and to keep a "using Layout = Properties'" to help with backwards compatibility so existing applications should come without problems.

Cheers,

Robert.

[Roland Hill]

Hi Robert,

I've got an easy one for you. Spelling mistake for additionalDescrptorSetLayout. 

 

Looks like everyone uses autocomplete!

Cheers,

Roland

--
You received this message because you are subscribed to the Google Groups "vsg-users : VulkanSceneGraph Developer Discussion Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vsg-users+...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/vsg-users/CAFN7Y%2BXfxj8ZntkXwt9b1E%3Db-OS-e_fpmwX7qePMWL-L%2BxOxBw%40mail.gmail.com.

[Robert Osfield]

On Tue, 1 Nov 2022 at 00:35, Roland Hill <role...@gmail.com> wrote:

I've got an easy one for you. Spelling mistake for additionalDescrptorSetLayout. 

Thanks for spotting this.  I've merged fixes to VSG, vsgXchange and vsgExamples.

[Robert Osfield]

On Mon, 31 Oct 2022 at 18:34, Robert Osfield <robert....@gmail.com> wrote:

Two of my "could do" for VSG-1.0 items are:

    Data::Layout -> Data::Properties
    Make Data::Properites a public member

I have completed this work, and updated osg2vsg, vsgXchange and vsgExamples so that they all build without using the now deprecated Data::Layout enum + Data::getLayout(), setLayout() methods. The changes are:

VulkanSceneGraph:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/548/files

osg2vsg:

    https://github.com/vsg-dev/osg2vsg/pull/69

vsgXchange:

    https://github.com/vsg-dev/vsgXchange/pull/123/files

vsgExamples:

    https://github.com/vsg-dev/vsgExamples/commit/65d4ad6a054441d809157540e584e7898b0984e0

I recommend that you update your own codebase along the same lines to use Data::Properties rather than Data::Layout as the latter is now officially deprecated.  I will keep the old API in place for 1.0 for compatibility purposes, but will remove this deprecated API after 1.0.

It's nice to get this item ticked off for 1.0 as the new API feels cleaner and more consistent with other parts of the VSG.

[Robert Osfield]

I have jumped back to the code review work and have completed my review of include/vsg/traversals and include/vsg/ui.  Most of the changes were addition of doyxgen comments with a few changes to improve consistency of layout.  Changes:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/549/files

My aim for today is to complete my review of the VSG headers, I have utils, viewer, rtx and platform left to complete.

I am also considering moving a few classes between the include directories and renaming the viewer directory to app as folks might use just the compute side of the VSG so never need a viewer, but will need classes from the current viewer directory. The aim is to make the VSG more logical for users.

[Robert Osfield]

I have now merged my review of the ui, utils and viewer header directories:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/549

This leaves rtx and platform header directories to complete.

[Robert Osfield]

I have just finished reviewing and adding doxygen comments and missing VSG_type_name entries for the include/vsg/raytracing, rtx and platform directories:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/550/files

That completes my code review of the VSG headers, all 23579 lines of them.

--

This is what I have left on TODO list:

MUST DO:

    Code review of CMake build system

    Review Githuib.io websites
    Fixed all reported bugs and build issues in core VSG

    Organize Launch party

    Make VulkanSceneGraph-1.0 branch
    Make VulkanSceneGraph-1.0.0 tag

    Press release and Tweet and LinkedIn posts for it.

Should do:

    Move CopyImageViewToWindow from viewer into commands direcotry
    Move traversal class into utils & viewer directory.
    Rename viewer directory to app.

    Code review of core VSG .cpp's:

        core
        maths
        io
        vk
        state
        commands
        nodes
        threading

        traversal
        ui
        utils
        viewer
        rtx
        platform

    Fix all reported bugs and build issues in main ancillary projects
    Review execution of all vsgExamples programs

    Build doxygen documentation.

    Make allocator_affinity_nodes more general

could do:

    Make ViewDependentState something that is built into ShaderSets.
    Create vsg::Billboard transform node
    Implement billboard functionality in standard vertex shader/use in vsg::Builder
    Refactor vsg::tile to use ShaderSet/GraphicsPipelineConfig

    Implement handling of vsg::WriteError/WriteResult

    Code review of ancillary projects

        vsgXchange
        osg2vsg
        vsgImGui
        vsgExamples

        vsgQt
        vsgSDL
        vsgWX
        vsgVR
        vsgPoints

        vsgopenmw
        VulkanPBRT

--

And this is what i've completed:

--COMPLTED ----------------------------

MUST DO:

    Code review of core VSG headers:
        core
            . Allocator.h
                check use std::allocator_affinity_nodes etc.
            . Array2D.h
                is the create(Args... args) required/optimal?
            . Array3D.h
            . Array.h
            . Auxiliary.h
            . compare.h
            . ConstVisitor.h
                Missing ReadError/WriteError
                How to check missing support?
            . Data.h
                Should we support stride for width?
            . Exception.h
            . Export.h
            . External.h
            . Inherit.h
            . Mask.h
            . MemorySlots.h
            . Object.h
            . Objects.h
            . observer_ptr.h
            . ref_ptr.h
            . ScratchMemory.h
            . type_name.h
            . Value.h
            . Version.h
                Need to bump version number for release candidates
            . visit.h
            . Visitor.h

        maths
            . box.h
            . clamp.h
            . color.h
            . mat3.h
            . mat4.h
            . plane.h
            . quat.h
            . sample.h
            . sphere.h
            . transform.h
            . vec2.h
            . vec3.h
            . vec4.h
            . comman.h

        io
            . AsciiInput.h
            . AsciiOutput.h
            . BinaryInput.h
            . BinaryOutput.h
            . convert_utf.h
            . DatabasePager.h
            . FileSystem.h
            . Input.h
            . Logger.h
            . mem_stream.h
            . ObjectFactory.h
            . Options.h
            . Output.h
            . Path.h
            . ReaderWriter.h
            . read.h
            . read_line.h
            . spirv.h
            . stream.h
            . tile.h
            . VSG.h
            . write.h - WriteResult?

        vk
            . AllocationCallbacks.h
            . CommandBuffer.h
            . CommandPool.h
            . Context.h
            . DescriptorPool.h
            . DeviceFeatures.h
            . Device.h
            . DeviceMemory.h
            . Extensions.h
            . Fence.h
            . Framebuffer.h
            . Instance.h
            . MemoryBufferPools.h
            . PhysicalDevice.h
            . Queue.h
            . RenderPass.h
            . ResourceRequirements.h
            . Semaphore.h
            . State.h
            . SubmitCommands.h
            . Surface.h
            . Swapchain.h
            . vk_buffer.h
            . vulkan.h

        state
            . ArrayState.h
            . BindDescriptorSet.h
            . Buffer.h
            . BufferInfo.h
            . BufferView.h
            . ColorBlendState.h
            . ComputePipeline.h
            . DepthStencilState.h
            . DescriptorBuffer.h
            . Descriptor.h
            . DescriptorImage.h
            . DescriptorSet.h
            . DescriptorSetLayout.h
            . DescriptorTexelBufferView.h
            . DynamicState.h
            . GraphicsPipeline.h
            . Image.h
            . ImageInfo.h
            . ImageView.h
            . InputAssemblyState.h
            . material.h
            . MultisampleState.h
            . PipelineLayout.h
            . QueryPool.h
            . RasterizationState.h
            . ResourceHints.h
            . Sampler.h
            . ShaderModule.h
            . ShaderStage.h
            . StateCommand.h
            . StateSwitch.h
            . TessellationState.h
            . VertexInputState.h
            . ViewDependentState.h
            . ViewportState.h
            . PushConstants.h

        commands

            . BeginQuery.h
            . BindIndexBuffer.h
            . BindVertexBuffers.h
            . BlitImage.h
            . ClearAttachments.h
            . Command.h
            . Commands.h
            . CopyAndReleaseBuffer.h
            . CopyAndReleaseImage.h
            . CopyImage.h
            . CopyImageToBuffer.h
            . CopyQueryPoolResults.h
            . Dispatch.h
            . Draw.h
            . DrawIndexed.h
            . DrawIndexedIndirect.h
            . DrawIndirectCommand.h
            . DrawIndirect.h
            . EndQuery.h
            . Event.h
            . ExecuteCommands.h
            . NextSubPass.h
            . PipelineBarrier.h
                - SampleLocations?
                - VulkanInfo?
            . ResetQueryPool.h
            . ResolveImage.h
            . SetDepthBias.h
            . SetLineWidth.h
            . SetScissor.h
            . SetViewport.h
            . WriteTimestamp.h

        nodes
            . AbsoluteTransform.h
            . Bin.h
            . CullGroup.h
            . CullNode.h
            . DepthSorted.h
            . Geometry.h
            . Group.h
            . Light.h
            . LOD.h
            . MatrixTransform.h
            . Node.h
            . PagedLOD.h
            . QuadGroup.h
            . StateGroup.h
            . Switch.h
            . TileDatabase.h
            . Transform.h
            . VertexDraw.h
            . VertexIndexDraw.h

        threading
            . ActivityStatus.h
            . Affinity.h
            . atomics.h
            . Barrier.h
            . FrameBlock.h
            . Latch.h
            . OperationQueue.h
            . OperationThreads.h

        traversal
            . CompileTraversal.h
            . ComputeBounds.h
            . Intersector.h
            . LineSegmentIntersector.h
            . LoadPagedLOD.h
            . RecordTraversal.h

        ui
            . ApplicationEvent.h
            . CollectEvents.h
            . FrameStamp.h
            . KeyEvent.h
            . PlayEvents.h
            . PointerEvent.h
            . PrintEvents.h
            . RecordEvents.h
            . ScrollWheelEvent.h
            . ShiftEventTime.h
            . TouchEvent.h
            . UIEvent.h
            . WindowEvent.h

        utils
            . AnimationPath.h
            . Builder.h
            . CommandLine.h
            . GraphicsPipelineConfig.h
                - Can we find a better name?
            . ShaderCompiler.h
            . ShaderSet.h
            . SharedObjects.h

        viewer

            . Camera.h
            . CloseHandler.h
            . CommandGraph.h
            . CompileManager.h
            . CopyImageViewToWindow.h
                move to commands
                implement as list of internal coomands
            . EllipsoidModel.h
            . Presentation.h
            . ProjectionMatrix.h
            . RecordAndSubmitTask.h
            . RenderGraph.h
            . SecondaryCommandGraph.h
            . Trackball.h
                move to ui?
            . TransferTask.h
            . UpdateOperations.h
            . Viewer.h
            . View.h
            . ViewMatrix.h
            . WindowAdapter.h
            . Window.h
            . WindowResizeHandler.h
            . WindowTraits.h

        raytracing
            . AccelerationGeometry.h
            . AccelerationStructure.h
            . BottomLevelAccelerationStructure.h
            . BuildAccelerationStructureTraversal.h
            . DescriptorAccelerationStructure.h
            . RayTracingPipeline.h
            . RayTracingShaderGroup.h
            . TopLevelAccelerationStructure.h
            . TraceRays.h

        rtx
            . DrawMeshTasks.h
            . DrawMeshTasksIndirectCommand.h
            . DrawMeshTasksIndirectCount.h
            . DrawMeshTasksIndirect.h

        platform
            android/Android_Window.h
            ios/iOS_ViewController.h
            ios/iOS_Window.h
            macos/MacOS_Window.h
            unix/Xcb_Window.h
            win32/Win32_Window.h

        Discuss moving forum to Githib - DONE : Created new github forum, updated links.

could do:

    Move GLSL reader from vsgXchange into core VSG

    Rename assimp.vert and assimp_*.frag standard.vert and standard_*.frag.

[Robert Osfield]

I have just merged the following "Should do" items:

    Move CopyImageViewToWindow from viewer into commands directory:
    https://github.com/vsg-dev/VulkanSceneGraph/pull/551

    Move traversal class into utils & viewer directory:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/553/files

Despite moving all these files from directory to directory all of the ancillary projects are all compiling without changes, so hopefully user applications will be fine too.   There are no functional changes, if you do hit a compile issue it'll just be a case of renaming a directory in the include.

[Robert Osfield]

Hi All,

I decided to tackle the big daddy of directory renaming - change include/vsg/viewer to more general include/vsg/app.  Functionally nothing has changed, it's just a rename to make things a little more logical.  Changes to VSG are:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/554

I have bumped the VSG version to 0.7.0 to signify this change (it's about to get to 1.0 very soon :-)

I had to update vsgQt and vsgImGui to buld against these latest changes, changes we simply a rename:

vsgImGui:    https://github.com/vsg-dev/vsgImGui/commit/92e1ddfc36a02a2a47f65f3c32551fed97bf2a2d

vsgQt:          https://github.com/vsg-dev/vsgQt/pull/14/files

I haven't tested the vsgSDL, vsgVR or vsgWX projects, my expect there is a reasonable chance these changes will break the build, but as for vsgImGui and vsgQt it'll likely require just a simple directory rename.  As have plenty of my own work to complete I'll leave testing/fixes of these to the rest of the community.

I am getting close to be able to create the VulaknSceneGraph-1.0 branch from VSG master.  There are still a few outstanding items so I will probably make that tomorrow.

Cheers,

Robert.

[Ralf Habacker]

Am 27.10.22 um 14:56 schrieb Robert Osfield:

...

If you can help with any of these items please let me know on this thread.

MUST DO:

...

    Code review of CMake build system

The vsgMacros.cmake file was duplicated from the vsg git project to all vsg dependent projects (I suspect this was related to the rebuild of the vsgFramework project some time ago).

My understanding is that vsg and the dependent projects in the vsg-dev namespace can be built either as individual projects one at a time, or in conjunction with vsgFramework as one big cmake project.

In the first case, vsgMacros.cmake is installed by vsg and is thus available to all dependent projects, so no copy is required.

In the second case the macros contained in vsgMacros.cmake are available in the whole cmake project, so that e.g. in vsgxchange no copy of this file has to be included (I commented out the corresponding call in the top level CMakeLists.txt of the vsgxchange project to check this).

Or are there other use cases that would require a copy of this file ?

Ralf

Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.

SPIE Deutschland & Zentraleuropa
www.spie.de

SPIE SAG GmbH
Sitz der Gesellschaft: Ratingen
Geschäftsanschrift: Balcke-Dürr-Allee 7 40882 Ratingen, DE
Eingetragen beim Amtsgericht Düsseldorf, HRB 82030
Geschäftsführung: Markus Holzke, Peter Pfannenstiel, Burkhard Sager
Aufsichtsratsvorsitzender: Gauthier Louette
USt-IdNr.: DE258150700

Unbeschadet der Korrespondenz per E-Mail, sind unsere Erklärungen ausschließlich dann rechtsverbindlich, wenn sie in herkömmlicher Schriftform (mit eigenhändiger Unterschrift des Geschäftsführers oder zweier Gesamtvertretungsberechtigter) oder durch Übermittlung eines solchen Schriftstücks per Telefax erfolgen. Die Datenschutzinformationen nach Art. 13 DSGVO erhalten Sie in unseren B2B-Datenschutzhinweisen .

Please consider the environment before printing this email.

SPIE Deutschland & Zentraleuropa
www.spie.de

SPIE SAG GmbH
Company seat: Ratingen
Business address: Balcke-Dürr-Allee 7 40882 Ratingen, DE
Registered at Amtsgericht Düsseldorf, HRB 82030
Managing Director: Markus Holzke, Peter Pfannenstiel, Burkhard Sager
Chairman of the Supervisory Board: Gauthier Louette
VAT-ID: DE258150700

Notwithstanding the correspondence via e-mail our statements are only legally binding if they are made in writing and are either signed by the Managing Director or duly signed by two authorized representatives or if such document is submitted by facsimile.
Please find our data protection information pursuant to art. 13 GDPR in our B2B Privacy Notice .

[Robert Osfield]

Hi Ralf,

On Wed, 2 Nov 2022 at 15:49, Ralf Habacker <ralf.h...@spie.com> wrote:

The vsgMacros.cmake file was duplicated from the vsg git project to all vsg dependent projects (I suspect this was related to the rebuild of the vsgFramework project some time ago).

Yes, I had to duplicate rather than reuse one installed by VSG to avoid issues caused when using the VSG alongside other VSG related projects with submodule or FetchContent as used in vsgFramework.

 

My understanding is that vsg and the dependent projects in the vsg-dev namespace can be built either as individual projects one at a time, or in conjunction with vsgFramework as one big cmake project.

Yes that is now correct, at least since we addressed the issues that prevented use as a submodule/FetchContent.

 

In the first case, vsgMacros.cmake is installed by vsg and is thus available to all dependent projects, so no copy is required.

I don't think vsgMacros.cmake should ever be installed now.  If your project needs it, it will need keep a local copy.   If it's still being installed we should remove this.

 

In the second case the macros contained in vsgMacros.cmake are available in the whole cmake project, so that e.g. in vsgxchange no copy of this file has to be included (I commented out the corresponding call in the top level CMakeLists.txt of the vsgxchange project to check this).

I am not clear on what you mean here, is a modification of vsgXchange that is required?  Could you share you suggested change?

 

Or are there other use cases that would require a copy of this file ?

Since my decisions to move vsgMacros.cmake directly into the various projects rather than relying upon one installed by the VSG I don't think there should ever be any install related copying of vsgMarcos.cmake.  Installing and need to locate vsgMacros.cmake was one of the problems that tripped up submodule/FetchContent usage so avoiding this addressed this incompatibility.

Cheers,

Robert.

[Robert Osfield]

Hi All,

Today I've addressing the lighting support in the vsg::TileDatabase node (used by the readymap.vsgt and openstreemap.vsgt example data), in ability to customize the shaders and defaults used in the TileDatabase rendering.  As part of this work I reviewed the vsg::Builder's control of material properties and decided to remove the hardwired material used to all applications to set their own default materials used by vsg::Builder.  I also refined the default values used in vsg::PhongMaterial and vsg::PbrMaterial.

Changes to VulkanSceneGraph:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/555/files

Changes to vsgExamples:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/555/files

To see the effects, lets show some screenshots, first the default settings with vsgbuilder rendering just a textured the caocule:

    $ vsgbuilder -i textures/lz.rgb --capsule

Then lets modified the vsg::PhongMaterial setting to set the specular value:

    $ vsgbuilder -i textures/lz.rgb --capsule --specular 0.8.03 0

If you use a specular value of 0 0 0 0 you'll get a matt surface, have a  play with vsgbuilder.

Next up with have the new lighting support available when use the vsg::TileDatabase/vsg::tile render, as a refresher this is what it used to look like:

    $ vsgviewer models/openstreetmap_flat_shaded.vsgt

Now with lighting shader used (viewer by default provides a headlight, but if you set up your own light sources in the scene they'll all be supported as the vsglights example):

 

The same with readmap tiled example, first original flat shaded shader:

    vsgviewer models/readymap_flat_shaded.vsgt

And then with lighting which looks much better with more naturally textured earth model:

    $ vsgviewer models/readymap.vsgt

With the head light used here, and with a static model the lighting isn't as obvious as it will be when use vsg::DirectionLight set up to represent the sun.  I don't have an example for this, but if someone in the community fancies add one go for it :-)

With this work checked into VSG master I have ticked off the last code related TODO item I plan to tackle before making the VulkanSceneGraph-1.0 branch:

    Refactor vsg::tile to use ShaderSet/GraphicsPipelineConfig

I am now going to go back into reviewing CMake build system and looking to spotting any remaining problems in the code base.  This is a great time to check out VSG, vsgXchage... vsgExample masters and make sure everything is working for you before I make the release branch.

Cheers,

Robert.

[Robert Osfield]

In my review of the headers the naming of vsg::GraphicsPipelineConfig has stuck out as awkward.  I couldn't come up with a better name at the time of writing the class but have never been happy with it.

My current thought is to rename it to vsg::GraphicsPipelineConfigurator, and add a using GraphicsPiplineConfig = GraphicsPipelineConfigurator to allow existing app to continue compiling.

Thoughts?

[Robert Osfield]

On Friday, 4 November 2022 at 20:40:06 UTC Robert Osfield wrote:

My current thought is to rename it to vsg::GraphicsPipelineConfigurator, and add a using GraphicsPiplineConfig = GraphicsPipelineConfigurator to allow existing app to continue compiling.

I have made these changes and merged them with VSG, vsgXchange and vsgExamples master.

Changes to VSG:

    https://github.com/vsg-dev/VulkanSceneGraph/pull/556

Changes to vsgXchange:

    https://github.com/vsg-dev/vsgXchange/pull/124/files

Changes to vsgExamples:

    https://github.com/vsg-dev/vsgExamples/pull/181

 

--

I now a possible regression in vsgheadless to investigate and another code review.  Fingers crossed there are no show stoppers lurking.

[Ralf Habacker]

Am 02.11.22 um 17:10 schrieb Robert Osfield:

Hi Ralf,

On Wed, 2 Nov 2022 at 15:49, Ralf Habacker <ralf.h...@spie.com> wrote:

The vsgMacros.cmake file was duplicated from the vsg git project to all vsg dependent projects (I suspect this was related to the rebuild of the vsgFramework project some time ago).

Yes, I had to duplicate rather than reuse one installed by VSG to avoid issues caused when using the VSG alongside other VSG related projects with submodule or FetchContent as used in vsgFramework.

 

My understanding is that vsg and the dependent projects in the vsg-dev namespace can be built either as individual projects one at a time, or in conjunction with vsgFramework as one big cmake project.

Yes that is now correct, at least since we addressed the issues that prevented use as a submodule/FetchContent.

 

In the first case, vsgMacros.cmake is installed by vsg and is thus available to all dependent projects, so no copy is required.

I don't think vsgMacros.cmake should ever be installed now. If your project needs it, it will need keep a local copy.

The drawback is that from now on every package depending on vsg needs a copy and needs to be updated on every change added to that file to keep in sync.

Suppose there is a request to enable cppcheck execution for vsgFramework builds that are currently disabled. This requires patching the associated macro in vsg and synchronizing it with all dependent packages to avoid hard-to-maintain variants of this file.

  If it's still being installed we should remove this.

 

In the second case the macros contained in vsgMacros.cmake are available in the whole cmake project, so that e.g. in vsgxchange no copy of this file has to be included (I commented out the corresponding call in the top level CMakeLists.txt of the vsgxchange project to check this).

I am not clear on what you mean here, is a modification of vsgXchange that is required? 

If you use vsgFramework to build vsgxchange, you can remove this line https://github.com/vsg-dev/vsgXchange/blob/master/CMakeLists.txt#L14 and the following line and the macros provided by vsg are still present, which can be verified by adding these lines temporary. [1]

 
+if(NOT COMMAND vsg_setup_dir_vars)
+    message(FATAL_ERROR "could not find macro 'vsg_setup_dir_vars'")
+endif()

Could you share you suggested change?

I have prepared associated branches with fixes for each vsg related repo

 https://github.com/rhabacker/VulkanSceneGraph/tree/vsgmacros-fixes

 https://github.com/rhabacker/vsgExamples/tree/vsgmacros-fixes

 https://github.com/rhabacker/vsgImGui/tree/vsgmacros-fixes

 https://github.com/rhabacker/vsgQt/tree/vsgmacros-fixes

 https://github.com/rhabacker/vsgXchange/tree/vsgmacros-fixes

which I used to build with vsgFramework without any problem.

I can create pr's from them

Or are there other use cases that would require a copy of this file ?

Since my decisions to move vsgMacros.cmake directly into the various projects rather than relying upon one installed by the VSG I don't think there should ever be any install related copying of vsgMarcos.cmake.

Installing and need to locate vsgMacros.cmake was one of the problems that tripped up submodule/FetchContent usage so avoiding this addressed this incompatibility.

In the single project case described above, this was probably caused by vsgMacros.cmake not being included by default in vsgConfig.cmake.in as is often used in other projects, see a similar example at https://invent.kde.org/frameworks/kdoctools/-/blob/master/KF5DocToolsConfig.cmake.in#L33.

 Applying the following patch to vsg fixes path related problems, since inside vsgConfig.cmake CMAKE_CURRENT_LIST_DIR is on the vsgConfig.cmake directory. Since vsgConfig.cmake is included with the call to find_package(vsg), vsgConfig.cmake is also automatically included and no longer needs to be used in the dependent projects.

diff --git a/src/vsg/vsgConfig.cmake.in b/src/vsg/vsgConfig.cmake.in
index c62a7a2e..9d461c99 100644
--- a/src/vsg/vsgConfig.cmake.in
+++ b/src/vsg/vsgConfig.cmake.in
@@ -18,3 +18,4 @@ else()
 endif()
 
 include("${CMAKE_CURRENT_LIST_DIR}/vsgTargets.cmake")
+include("${CMAKE_CURRENT_LIST_DIR}/vsgMacros.cmake")

Cheers

[Robert Osfield]

Hi Ralf,

Could you generate PR's against the respective projects I can then test and make a decision.

Thanks,

Robert.

[Ralf Habacker]

Am 07.11.22 um 16:48 schrieb Robert Osfield:

Hi Robert,

>
> Could you generate PR's against the respective projects I can then
> test and make a decision.

done.

I'm not quite sure if porting the associated commit back to the
appropriate stable branch with `git cherry-pick -x ....` is the desired way.

Cheers
Ralf

Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail ausdrucken.

SPIE Deutschland & Zentraleuropa

www.spie.de<http://www.spie.de/>

SPIE SAG GmbH
Sitz der Gesellschaft: Ratingen
Geschäftsanschrift: Balcke-Dürr-Allee 7 40882 Ratingen, DE
Eingetragen beim Amtsgericht Düsseldorf, HRB 82030
Geschäftsführung: Markus Holzke, Peter Pfannenstiel, Burkhard Sager
Aufsichtsratsvorsitzender: Gauthier Louette
USt-IdNr.: DE258150700

Unbeschadet der Korrespondenz per E-Mail, sind unsere Erklärungen ausschließlich dann rechtsverbindlich, wenn sie in herkömmlicher Schriftform (mit eigenhändiger Unterschrift des Geschäftsführers oder zweier Gesamtvertretungsberechtigter) oder durch Übermittlung eines solchen Schriftstücks per Telefax erfolgen. Die Datenschutzinformationen nach Art. 13 DSGVO erhalten Sie in unseren B2B-Datenschutzhinweisen <https://spie.de/footer-dt/datenschutzhinweise-fuer-kunden-geschaeftspartner-und-interessenten> .

Please consider the environment before printing this email.

SPIE Deutschland & Zentraleuropa

www.spie.de<http://www.spie.de/>

SPIE SAG GmbH
Company seat: Ratingen
Business address: Balcke-Dürr-Allee 7 40882 Ratingen, DE
Registered at Amtsgericht Düsseldorf, HRB 82030
Managing Director: Markus Holzke, Peter Pfannenstiel, Burkhard Sager
Chairman of the Supervisory Board: Gauthier Louette
VAT-ID: DE258150700


Notwithstanding the correspondence via e-mail our statements are only legally binding if they are made in writing and are either signed by the Managing Director or duly signed by two authorized representatives or if such document is submitted by facsimile.

Please find our data protection information pursuant to art. 13 GDPR in our B2B Privacy Notice <https://spie.de/footer-dt/privacy-notice-for-customers-business-partners-and-interested-parties> .

[Robert Osfield]

On Mon, 7 Nov 2022 at 16:18, Ralf Habacker <ralf.h...@spie.com> wrote:

> Could you generate PR's against the respective projects I can then
> test and make a decision.
done.

I'm not quite sure if porting the associated commit back to the
appropriate stable branch with `git cherry-pick -x ....` is the desired way.

I don;t have time to do the review today, so plan to review the PRs tomorrow I will likely create a branch from VSG master etc. and if it all looks appropriate will merge with respective masters, then make a decision on pull these changes into the 1.0 branches.