using customized PBR ShaderSet with scene-graph loaded via vsgXchange causes validation-errors + crashes

[drywolf]

Hi,

I am loading scenes via vsgXchange, and those scenes might have several mesh-objects, some of which will use Phong-material and others will use PBR-material (in the same scene!)

Now I am trying to use a customized PBR shaderset (while leaving the Phong shaderset untouched)
I added the following uniform buffer to the standard_pbr.frag:

layout(binding = 9) uniform MatFxData
{
    vec4 overlayColor;
} mat_fx;

and I am using overlayColor in the shader code to do a simple color-blending on top of the original PBR shader result.
To be able to use this modified shader, I copied the code from vsgExamples -> flat.cpp and I am using the path to my modified .frag shader file.

Also I added the new uniform-binding to this custom shader-set like so:

  customPBR->addUniformBinding("mat_fx", "", 0, 9, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::MatFxValue::create());

This shader-set instance I then apply to vsg::Options, when I load the scene via vsgXchange:

auto load_options = vsg::Options::create();
load_options->shaderSets["pbr"] = customPBR;

This is all good so far, but here comes the problem.
To make this work, I also need to "patch" the vsg::BindGraphicsPipeline command-nodes and vsg::BindDescriptorSet command-nodes that are used when rendering the PBR shader-set in the command-graph.

In vsgExamples -> vsgstateswitch.cpp I saw that such modifications to an existing command-graph can be done by using a custom vsg::Visitor that will look for the specific node-types and there you can apply the modifications that you need.

So I created such a custom visitor, and started patching the necessary Vulkan state to include binding = 9

• in the visited vsg::DescriptorSetLayout::bindings list I insert an extra VkDescriptorSetLayoutBinding with binding = 9
• in the visited vsg::DescriptorSet::descriptors list I insert an extra uniform-buffer descriptor vsg::DescriptorBuffer::create(mat_fx_data, 9)

This already worked for a simple scene, where I only had a single mesh that was using only the PBR shader.
But when I started doing some tests with more complex scenes, this same code started giving me all kinds of VK validation errors and crashes.

It took a while for me to figure out what was going on.
The problem is happening because:

• the scenes contain meshes that are using both material types (pbr + phong)
• I did not customize the Phong shader/shader-set, so there is no layout(binding = 9) for this material type

(but this is also kind of what I wanted, I only want the PBR material-type to have this color overlay, the Phong material-type should not need to have any changes, just because I decide to change the PBR material)

As said, at this point in time, I still kept getting VK errors and crashes. I quickly realized that I needed to modify the VK/VSG state only for draw-commands related to PBR meshes (and not change any VK/VSG state related to the Phong draw-commands)

So then I tried to modify my code to only insert the extra VkDescriptorSetLayoutBinding and vsg::DescriptorBuffer in cases where the graphics-pipeline is rendering a PBR mesh.
I thought this should be fine now, but I still kept getting the same crashes/validation-errors.

Some more hours of debugging, and I finally realized that vsg::SharedObjects was doing things, I didn't expect it to do.
(and this is actually what this whole thread should be about)

Basically, when vsgXchange loads the scene, any descriptor-set or descriptor-layout that is created, will be automagically de-duplicated by vsg::SharedObjects.
This means that for all draw-commands in the render-graph, if their descriptor-set and/or descriptor-layout are exactly the same (even if it is just by pure coincidence), vsg will silently re-use a single VSG object for all the Vulkan state/commands in the graph.

Now when I do modifications to the render-graph, I can never know if a descriptor-set/descriptor-layout that I am visiting and modifying will only be used for a PBR shader-set drawcall, or also for some Phong shader-set drawcalls 🤯 🙈
Hence when I make the modifications that I intended only for PBR draw-commands, I always also implicitly change the Vulkan state for the Phong draw-commands.

@robertosfield
Maybe I am trying to do this the wrong way, so please let me know if there is a better solution to use a custom PBR shader-set + add the required VK states to the render-graph.
But also on the other hand, this very implicit and hidden state/object sharing really will be quite confusing and might cause lots of spent hours of debugging, if there is not a really solid & somewhat explicit API to handle it.

I also would be really interested to hear some of your thoughts on the current design
(maybe you have already written something about it? I'm sorry if I did miss it)

Thanks

PS: I am really a fan of VSG so far ❤️ and I want to dive deeper into it and learn more.
(I did not intend the above to sound harsh or as unfunded criticism ... I just want to understand how I would achieve the thing that I am trying to do)

[drywolf]

For completeness, this is the log of the typical VK validation-errors that I am getting from this:
(these are just the ones I see before the application crashes)

VUID-VkGraphicsPipelineCreateInfo-layout-07988(ERROR / SPEC): msgNum: 559874765 - Validation Error: [ VUID-VkGraphicsPipelineCreateInfo-layout-07988 ] Object 0: handle = 0x96fbe2000000005e, type = VK_OBJECT_TYPE_SHADER_MODULE; Object 1: handle = 0x130160000000005c, type = VK_OBJECT_TYPE_PIPELINE_LAYOUT; | MessageID = 0x215f02cd | vkCreateGraphicsPipelines(): pCreateInfos[0] Set 0 Binding 9 in shader (VK_SHADER_STAGE_FRAGMENT_BIT) uses descriptor slot (expected `VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC, VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK`) but not declared in pipeline layout The Vulkan spec states: If a resource variables is declared in a shader, a descriptor slot in layout must match the shader stage (https://vulkan.lunarg.com/doc/view/1.3.250.0/windows/1.3-extensions/vkspec.html#VUID-VkGraphicsPipelineCreateInfo-layout-07988)
    Objects: 2
        [0] 0x96fbe2000000005e, type: 15, name: NULL
        [1] 0x130160000000005c, type: 17, name: NULL
VUID-VkGraphicsPipelineCreateInfo-layout-07988(ERROR / SPEC): msgNum: 559874765 - Validation Error: [ VUID-VkGraphicsPipelineCreateInfo-layout-07988 ] Object 0: handle = 0x96fbe2000000005e, type = VK_OBJECT_TYPE_SHADER_MODULE; Object 1: handle = 0x130160000000005c, type = VK_OBJECT_TYPE_PIPELINE_LAYOUT; | MessageID = 0x215f02cd | vkCreateGraphicsPipelines(): pCreateInfos[0] Set 0 Binding 9 in shader (VK_SHADER_STAGE_FRAGMENT_BIT) uses descriptor slot (expected `VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC, VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK`) but not declared in pipeline layout The Vulkan spec states: If a resource variables is declared in a shader, a descriptor slot in layout must match the shader stage (https://vulkan.lunarg.com/doc/view/1.3.250.0/windows/1.3-extensions/vkspec.html#VUID-VkGraphicsPipelineCreateInfo-layout-07988)
    Objects: 2
        [0] 0x96fbe2000000005e, type: 15, name: NULL
        [1] 0x130160000000005c, type: 17, name: NULL
VUID-vkCmdDrawIndexed-None-02699(ERROR / SPEC): msgNum: -1539028524 - Validation Error: [ VUID-vkCmdDrawIndexed-None-02699 ] Object 0: handle = 0x5dbcf90000000065, type = VK_OBJECT_TYPE_DESCRIPTOR_SET; | MessageID = 0xa44449d4 | VkDescriptorSet 0x5dbcf90000000065[] encountered the following validation error at vkCmdDrawIndexed time: Attempting to validate DrawState for binding #9  which is an invalid binding for this descriptor set. The Vulkan spec states: Descriptors in each bound descriptor set, specified via vkCmdBindDescriptorSets, must be valid as described by descriptor validity if they are statically used by a bound shader (https://vulkan.lunarg.com/doc/view/1.3.250.0/windows/1.3-extensions/vkspec.html#VUID-vkCmdDrawIndexed-None-02699)
    Objects: 1
        [0] 0x5dbcf90000000065, type: 23, name: NULL

[timoore]

What you're trying to do is kind of complicated; it's almost easier to write a loader from scratch. You do need to copy the objects that might be shared before you mutate them, but I think that only shallow copies are necessary and the constructors for the state commands should do a reasonable job of that. If not, it's a bug.

If you've passed in your own shader set via the options to vsgXchange, why do you have to modify the BindGraphicsPipeline state command? If you are going to change DescriptorSet commands, then yeah, you need to copy them.

The error message makes it looks like perhaps you didn't call viewer->compile() after making your patches to the scene graph; dunno if that's a possibility.

Another possibility is to use another descriptor set with your new descriptors, so you could potentially bind them with a different state command. The current ShaderSet and GraphicsPipelineConfigurator classes make that awkward. I modified them in vsgCs and need to get off my butt and make a PR for them here.

[drywolf]

hi, thank you for your reply

What you're trying to do is kind of complicated

All I am trying to do is using a very slightly extended version of the existing PBR shader-set to render my meshes in VSG.
I think this should be a pretty natural and typical thing people might want to do.
VSG might not be in a place yet where this can be easily done (just like Robert says in his reply) and I completely understand it. VSG is an evolving project and I think this is really great, so I will help that this usecase can be supported easily.

If you've passed in your own shader set via the options to vsgXchange, why do you have to modify the BindGraphicsPipeline state command?

It is just what I figured by looking at all the places where I could find that references to DescriptorSet and DescriptorSetLayout are being held:

• vsg::BindGraphicsPipeline references a vsg::GraphicsPipeline
• ... the pipeline then references a vsg::PipelineLayout
• ... the pipeline-layout then references a list of vsg::DescriptorSetLayouts
• ... the descriptorset-layout then references a list of vsg::DescriptorSetLayoutBindings
• ... and in this list of bindings I think I also need to make sure that my new layout(binding = 9) is inserted ?!
  The ShaderSet by itself will not do any of this work (as far as I can see it by looking at the code).
  PS: I even tried generating a pbr_ShaderSet_custom.cpp as Robert explained it in this Google Forums Post, but no matter what I tried, I keep getting the VK validation-errors + a crash. So either this is not working as intended, or it is not yet intended to work at this point anyway 🤷‍♂️

PS: this chain of references that I showed is also a good illustration of why I think the current concept of SharedObjects could become somewhat problematic in the future (if left as-is).
You very quickly end up needing to chase references all across the render-graph, and if you want to change something you are forced to make a copy of it.
But you can't only do it for the object that you want to modify! You usually will need to make sure that also ALL OTHER shared objects that are holding references to the object you want to modify will be cloned (and they are pointing to the modified/cloned object instead).

So if you want to change something on any object that is shared, then this change can have a cascading effect (upwards in the graph) and you very quickly can end up needing to chase pointers/references in all kinds of places. And if you happen to miss one spot, then the real fun™ begins.

---

If you are going to change DescriptorSet commands, then yeah, you need to copy them.

Yeah this is what I have been trying already, but without final success. The only thing that helped me so far was to completely disable the usage of vsg::SharedObjects (I even had to comment out some lines in vsgXchange that would auto-create SharedObjects if they weren't provided via options)
When I do it this way, my code is already working, because it allows me to modify only the descriptor-set / descriptor-set-layout that is related to PBR (without SharedObjects getting in the way and affecting the Phong vk DS/DSL)

I will do as Robert suggested and try to make this usecase into a minimal vsgExample. Then we can see what needs to be changed/extended to make this usecase more easily accessible to developers ❤️

[robertosfield]

@drywolf would it be possible to create an example of what you are attempting to do and share this and any necessary data? Ideally in a form that we can add to vsgExamples as a form of unit test/something for us to iterate on to resolve the issue, then later as something others can learn from how to do this type of customization work.

As @timoore mentioned he has some mods to the VSG that he uses to do what he requires in vsgCs once this as submitted I'll review these and either merge them as is or do other refactor work to address the problems his changes address. I'll then wrap this up in a stable release.

This is how the VSG project evolves, new uses come along and push the VSG beyond it's present capabilities, we refactor or add to it to resolve these deficiencies. Using examples as unit tests is one way I like to use through this process - the example can be written to do something that needs to be done, and perhaps is done way more complicated than desired, or not work at all, then changes to the VSG can be tried out to resolve the issues, then finally we end up with a solution and a nature example.

It may of course be that actually it doesn't require any mods to the core VSG, just a little refactor of the example.

[drywolf]

Thanks Robert,
I will create a minimal vsgExample as suggested 👍

[robertosfield]

As a general comment, the vsg::SharedObjects class is used to avoid duplicate state from consuming precious memory and CPU/GPU resources. it's a crucial part of getting the best performance out of graphics applications.

However, sharing data that will need to be indecently varied can be an issue. The question then is how to be declare which parts are appropriate to share and which should be duplicated. In general you can check the referenceCount of an object to see if it's uniquely held and if it's not cloning the type and assigning the clone to the part that you need to vary will be required, this should just be a local change though, other parts of the scene can still reference the original object that you've cloned.

[drywolf]

Yeah, that is all what I figured out the hard way 😅
And the general idea is a really good one, not duplicating CPU/GPU state is an important goal and I am fully behind that.
But the approach how to handle this de-duplication must be really carefully considered I think.
Otherwise, it will very heavily restrict users in what they can do after a scene has been loaded,
or worse, it will lead to bugs/glitches that are exceptionally hard to track down, because the cause of the bug will be completely invisible/unrelated to the site where the bug will manifest itself. (because of this implicit shared state)

PS: Just one idea how it could be done differently and make things better already right now:
I was thinking, what if the calls to vsg::SharedObjects::share() were not done directly while vsgXchange is loading the scene,
but instead the share() was done in a spearate vsg::Visitor after vsgXchange is finished.
Because then, developers can have the ability to make changes to the loaded render-graph, before any shared-state is created,
and after they have made the desired changes, then the vsg::StateSharingVisitor can be called explicitly and will achieve the same thing that is done right now.
But the changes that a developer made to the render-graph will always be respected/included in the render-graph, because point in time when the state-sharing will happen is under explicit control of the developers.

PSS: this only solves the most pressing issue though, and this approach also just slightly improves the issue, there are many more things that people might want to do with render-graphs that would still not be solved by this approach.
This is why I think the concept of de-duplicating/sharing state needs a really careful design consideration to be made.

[drywolf]

I created the minimal vsgExample that shows pretty much exactly what I was trying to do.

https://github.com/drywolf/vsgExamples/tree/vsgcustompbr

So basically all I did is create a copy of the original pbr.cpp shader-set + the related .frag file,
and then I added a new uniform-buffer at layout(binding = 9)

I also included in the repository a FBX test-scene that I was using for testing so far (in data/models/fbx-test.fbx)
The only special thing about this scene is that it uses both material types PBR + Phong.
(as mentioned earlier, afaik this is critical to trigger the issue ... if you have a scene that will only use PBR material, then it might work just fine ?!)

If you compile & run this example, you will see the validation-errors and get the crash that I mentioned.

[drywolf]

My expectation is applications would not generally mix PBR and Phone shader subgraphs in one scene, let alone within one model. I would expect scenes would need to be lit/shaded with the same mathematical models and controlled in one broad way.

PS: I don't think I agree on this ... a scene exported from 3D tools could be any combination of materials/shading modes, it could be as extreme as mixing PBR and Toon-Shading in the same scene, and I would expect VSG to be able to handle this also just fine.

[robertosfield]

I can see the custom_pbr..cpp now so I don't know why I couldn't see it yesterday. Your vsgExamples doesn't build with VSG master at present as I've replaced the WidowTraints::sharedWindow member with WindowTraits::device. I'll revert to VSG v1.0.7 and see how I get on.

Mixing multiple different shader effects in one scene is already/will always be supported, but I wouldn't recommend putting multiple approaches to shaders into a single loaded model as it just makes things more complicated for tasks like when you want to substitute state like you are doing.

[drywolf]

but I wouldn't recommend putting multiple approaches to shaders into a single loaded model as it just makes things more complicated

Yeah I can understand that this leads to increased complexity, but the latest status quo on materials is that potentially each object in your scene could have a unique material with unique shader-code applied to it.
So having two "hard-coded" shaders in the same scene/model should actually be one of the most simple scenarios in 2023 😅
On the other end of the spectrum you have fully user-programmable shading models, and that is much more interesting than just having two pre-defined material models.

To mention just a few examples of technologies that have been pushing this trend in the past couple of years already:

• MaterialX
• Nvidia MDL
• Unreal Engine 4 Material Blueprints
• Unreal Engine 5 Substrate Materials
• Unity Shader Graph
• Blender Shader Nodes

The question for VSG will be just how to properly support these kinds of use-cases. I would be happy to help you advance it in this direction.

[drywolf]

PS: also having two different material models (PBR + Phong) in the same scene was not an intentional choice when I created the model in Blender.
It just so happened as an happy accident™ when I assigned some simple colors to the meshes in Blender, but left the Plane object untouched.
I just explain this, to show how easy it is to create content that could have potentially multiple different material models within a single mesh/scene file.
It is not something that VSG/vsgXchange should try to limit!
Quite to the contrary! The more flexibility VSG/vsgXchange could have to deal with a multitude of material-models / dynamically generated material-shaders, the more interesting VSG could be for integration into some 3D tools, content-creation pipelines, games, etc.

[robertosfield]

The issue with multiple shaders in a single model is the impact it might have on the visuals, it's not the different shaders combinations may be required, that should already supported, but whether they are compatible/coherent. High level shader management such as ShaderSet and all the variants you mention don't fix this problem of compatibility.

PBR and Phong shaders in the same model is something I wouldn't advise, they really are different approaches and not compatible in terms of visuals you'll get. Why a single model has ended up with wanting different ways of doing lighting and using texturing is something I can't answer, but it doesn't seem like a coherent model.

[timoore]

If you've passed in your own shader set via the options to vsgXchange, why do you have to modify the BindGraphicsPipeline state command?

It is just what I figured by looking at all the places where I could find that references to DescriptorSet and DescriptorSetLayout are being held:

• vsg::BindGraphicsPipeline references a vsg::GraphicsPipeline
• ... the pipeline then references a vsg::PipelineLayout
• ... the pipeline-layout then references a list of vsg::DescriptorSetLayouts
• ... the descriptorset-layout then references a list of vsg::DescriptorSetLayoutBindings
• ... and in this list of bindings I think I also need to make sure that my new layout(binding = 9) is inserted ?!
  The ShaderSet by itself will not do any of this work (as far as I can see it by looking at the code).

Sorry, I was confusing my aforementioned patches with the current state of vsg and vsgXchange. It's true that the current assumption in vsgXchange is that all descriptors for a pipeline will be bound "at once," and that the descriptor set layout comes from the descriptors that vsgXchange discovers, not from the shader set.

[robertosfield]

I have been experimenting with the vsgcustompbr example this morning and see the same Vulkan debug errors. I can remove these errors if I make the additional mat_fx an optional setting and this disables the associated code paths in the fragment shader so avoids the error. but this obviously isn't the intent of vsgcutompbr as it is.

The changes I've made include outputting the resulting scene graph so I can reivew the results:

diff --git a/data/shaders/standard_pbr_custom.frag b/data/shaders/standard_pbr_custom.frag
index 5521327..9bed068 100644
--- a/data/shaders/standard_pbr_custom.frag
+++ b/data/shaders/standard_pbr_custom.frag
@@ -1,6 +1,7 @@
 #version 450
 #extension GL_ARB_separate_shader_objects : enable
-#pragma import_defines (VSG_DIFFUSE_MAP, VSG_GREYSACLE_DIFFUSE_MAP, VSG_EMISSIVE_MAP, VSG_LIGHTMAP_MAP, VSG_NORMAL_MAP, VSG_METALLROUGHNESS_MAP, VSG_SPECULAR_MAP, VSG_TWO_SIDED_LIGHTING, VSG_WORKFLOW_SPECGLOSS)
+#pragma import_defines (VSG_DIFFUSE_MAP, VSG_GREYSACLE_DIFFUSE_MAP, VSG_EMISSIVE_MAP, VSG_LIGHTMAP_MAP, VSG_NORMAL_MAP, VSG_METALLROUGHNESS_MAP, VSG_SPECULAR_MAP, VSG_TWO_SIDED_LIGHTING, VSG_WORKFLOW_SPECGLOSS, VSG_MATFXDATA)
+
 
 const float PI = 3.14159265359;
 const float RECIPROCAL_PI = 0.31830988618;
@@ -32,10 +33,12 @@ layout(binding = 4) uniform sampler2D emissiveMap;
 layout(binding = 5) uniform sampler2D specularMap;
 #endif
 
+#ifdef VSG_MATFXDATA
 layout(binding = 9) uniform MatFxData
 {
     vec4 overlayColor;
 } mat_fx;
+#endif
 
 layout(binding = 10) uniform PbrData
 {
@@ -470,6 +473,8 @@ void main()
 
     outColor = LINEARtoSRGB(vec4(color, baseColor.a));
 
+#ifdef VSG_MATFXDATA
     // just perform a simple additive blending of the overlayColor on top of the PBR result
     outColor.rgb += mat_fx.overlayColor.rgb;
+#endif
 }
diff --git a/examples/shading/vsgcustompbr/custom_pbr.cpp b/examples/shading/vsgcustompbr/custom_pbr.cpp
index 5fe1672..ecb4e16 100644
--- a/examples/shading/vsgcustompbr/custom_pbr.cpp
+++ b/examples/shading/vsgcustompbr/custom_pbr.cpp
@@ -71,7 +71,7 @@ vsg::ref_ptr<vsg::ShaderSet> pbr_custom_ShaderSet(vsg::ref_ptr<const vsg::Option
     shaderSet->addUniformBinding("aoMap", "VSG_LIGHTMAP_MAP", 0, 3, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::vec4Array2D::create(1, 1));
     shaderSet->addUniformBinding("emissiveMap", "VSG_EMISSIVE_MAP", 0, 4, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::vec4Array2D::create(1, 1));
     shaderSet->addUniformBinding("specularMap", "VSG_SPECULAR_MAP", 0, 5, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::vec4Array2D::create(1, 1));
-    shaderSet->addUniformBinding("mat_fx", "", 0, 9, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::MaterialFxValue::create());
+    shaderSet->addUniformBinding("mat_fx", "VSG_MATFXDATA", 0, 9, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::MaterialFxValue::create());
     shaderSet->addUniformBinding("material", "", 0, 10, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::PbrMaterialValue::create());
     shaderSet->addUniformBinding("lightData", "", 1, 0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, vsg::vec4Array::create(64));
 
diff --git a/examples/shading/vsgcustompbr/vsgcustompbr.cpp b/examples/shading/vsgcustompbr/vsgcustompbr.cpp
index 56d8542..1c0e81e 100644
--- a/examples/shading/vsgcustompbr/vsgcustompbr.cpp
+++ b/examples/shading/vsgcustompbr/vsgcustompbr.cpp
@@ -79,6 +79,8 @@ int main(int argc, char** argv)
         options->add(vsgXchange::all::create());
 #endif
 
+        auto outputFilename = arguments.value<vsg::Path>("", "-o");
+
         // create an instance of the custom_pbr shader-set
         auto custom_pbr_shaderSet = pbr_custom_ShaderSet(options);
         assert(custom_pbr_shaderSet != nullptr);
@@ -193,6 +195,13 @@ int main(int argc, char** argv)
         else
             vsg_scene = group;
 
+
+        if (outputFilename)
+        {
+            vsg::write(vsg_scene, outputFilename, options);
+            return 1;
+        }
+
         // create the viewer and assign window(s) to it
         auto viewer = vsg::Viewer::create();
         auto window = vsg::Window::create(windowTraits);

Adding the #define this way is how the #pragma(tic) shader composition is meant to work, but... as the vsgXchange::assimp plugin doesn't know that it should assign this custom data it doesn't assign and enable the associated code path.

This lack of knowledge of the data type is also when the vsgXchange::assimp isn't assigned even the prototype or setting up any of the associated DescriptorSetLayout.

My current feeling is the problem lies in how do we inject custom data types/optional shader paths that loaders don't know about. The current vsg::GraphicsPipelineConfigurator will need to evolve further to enable this type of injection, and/or have vsg::Options provide the data to inject.

A first step may be to change the vsg::GraphicsPipelineConfigurator to assign all the non option entries and use the default value provided, so I'll look at this next.

I currently don't think the vsg::SharedObjects usage is related these particular issues, instead it's an orthogonal issue of how should we handle data that has been shared but we want to control multiple instances of the data rather than a single shared instance. There are solutions for this like the ones I mention in a previous reply, as well as vsg::SharedObjects::suitableForSharing visitor that users can override to prevent sharing of particular types of objects.

[drywolf]

I currently don't think the vsg::SharedObjects usage is related these particular issues, instead it's an orthogonal issue of how should we handle data that has been shared but we want to control multiple instances of the data rather than a single shared instance. There are solutions for this like the ones I mention in a previous reply, as well as vsg::SharedObjects::suitableForSharing visitor that users can override to prevent sharing of particular types of objects.

Yeah I completely agree that this is an orthogonal issue, but it was a big part of the confusion/debugging that I had to do when I was trying to get things working, so I wanted to get some of your thoughts on that too.
Thanks for the tip about suitableForSharing, I didn't know about this yet ... will have to try it out sometime. 👍

[timoore]

My current feeling is the problem lies in how do we inject custom data types/optional shader paths that loaders don't know about. The current vsg::GraphicsPipelineConfigurator will need to evolve further to enable this type of injection, and/or have vsg::Options provide the data to inject.

A first step may be to change the vsg::GraphicsPipelineConfigurator to assign all the non option entries and use the default value provided, so I'll look at this next.

You should take a look at what I do in vsgCs. The change from VSG / vsgXchange is that the pipeline layout is created from the ShaderSet and the defines that are part of shaderHints, which these days usually come from DescriptorConfigurator.

I'm preparing a PR which merges the VSG approach into VSG. This should work for the OP if the ShaderSet unconditionally includes the descriptor bindings that they want. I'm not sure what to do if they want to enable it by an option passed to vsgXchange.

[robertosfield]

@timoore OK I will wait for the PR before I dive into tweaking vsg::GraphicsPipelineConfiguration & vsgXchange::assimp to avoid diverging things/changing the same code.

[drywolf]

I will continue to poke at the vsgcustompbr example code and let you know if I come across any ideas/solutions for getting this usecase working properly/easily.

I am also curious to see how much of the solution already would be in the potential PR coming from the vsgCs code. 👍

[robertosfield]

I don't think the PR addresses the issues raised by the injection of additional types so will require a separate fix. I think GraphicsPipelineConfigurator needs to populate the descriptor set layouts based on the all the non optional entries for uniforms and textures, rather than only the ones explicitly enabled by the geometry setup. I will also need to scene graph set up code to assign the non optional uniform/textures even when not enabled explicitly.

This is basically an issue of vsg::ShaderSet being more flexible than the current incarnation of vsg::DescriptorConfigurator and vsg::GraphicsPipelineConfigurator can can with. The problem doesn't lie in vsg::ShaderSet but in the later two classes that use it to help build scene graph state, and usage of them in places like vsg::Builder and vsgXchnage::assimp.

To address these limitations in DescriptorConfigurator and GraphicsPipelineConfigurator and usage of them shouldn't be too complicated and something I plan to look at next week. The PR from Tim addresses other limitations orthogonal to the ones discussed in this thread.

[timoore]

I don't think the PR addresses the issues raised by the injection of additional types so will require a separate fix. I think GraphicsPipelineConfigurator needs to populate the descriptor set layouts based on the all the non optional entries for uniforms and textures, rather than only the ones explicitly enabled by the geometry setup. I will also need to scene graph set up code to assign the non optional uniform/textures even when not enabled explicitly.

I think makePipelineLayout() (called by GraphicsPipelineConfigurator::init()) does what you describe? It includes all bindings that are enabled by a define, or that are not optional, up to the requested descriptor set.

This is basically an issue of vsg::ShaderSet being more flexible than the current incarnation of vsg::DescriptorConfigurator and vsg::GraphicsPipelineConfigurator can can with. The problem doesn't lie in vsg::ShaderSet but in the later two classes that use it to help build scene graph state, and usage of them in places like vsg::Builder and vsgXchnage::assimp.

To address these limitations in DescriptorConfigurator and GraphicsPipelineConfigurator and usage of them shouldn't be too complicated and something I plan to look at next week. The PR from Tim addresses other limitations orthogonal to the ones discussed in this thread.

[robertosfield]

@drywolf Through this week I have been refactoring vsg::DescriptorConfigurator and vsg::GraphicsPipelineConfigurator to support multiple DescriptorSet, custom of DescriptorSet bindings such as support for vsg::ViewDependentState in an extensible way, and finally to automatically assign descriptors that the ShaderSet specifies that have the define=="" so essentially default parameters that should always be set.

All of this work has been carried out in MultiDescriptorSetConfiguration branches of the VSG, vsgXchange, vsgPoints and vsgExamples, and also just wrapped up by getting your vsgcustompbr example working, with just a small change adding in the new ShaderSet::customDescriptorSetBindings functionality, see below for the changes I have made:

diff --git a/examples/shading/vsgcustompbr/custom_pbr.cpp b/examples/shading/vsgcustompbr/custom_pbr.cpp
index 5fe1672..3ac05fc 100644
--- a/examples/shading/vsgcustompbr/custom_pbr.cpp
+++ b/examples/shading/vsgcustompbr/custom_pbr.cpp
@@ -85,5 +85,7 @@ vsg::ref_ptr<vsg::ShaderSet> pbr_custom_ShaderSet(vsg::ref_ptr<const vsg::Option
     shaderSet->definesArrayStates.push_back(vsg::DefinesArrayState{{"VSG_DISPLACEMENT_MAP"}, vsg::DisplacementMapArrayState::create()});
     shaderSet->definesArrayStates.push_back(vsg::DefinesArrayState{{"VSG_BILLBOARD"}, vsg::BillboardArrayState::create()});
 
+    shaderSet->customDescriptorSetBindings.push_back(vsg::ViewDependentStateBinding::create(1));
+
     return shaderSet;
 }

The branches of interest are:

https://github.com/vsg-dev/VulkanSceneGraph/tree/MultiDescriptorSetConfiguration
https://github.com/vsg-dev/vsgXchange/tree/MultiDescriptorSetConfiguration
https://github.com/vsg-dev/vsgPoints/tree/MultiDescriptorSetConfiguration
https://github.com/vsg-dev/vsgExamples/tree/MultiDescriptorSetConfiguration

I still have some work dev work/testing to do on these branches but they should be far enough along for others to start testing. I need to go right now, but over the next few days I'll provide a bit more info about this work.

[drywolf]

This sounds great, thanks @robertosfield for the update.
I will give this new code a try over the weekend 🥳

[robertosfield]

@drywolf How did you get on?

The changes are all part of the release candidates that I tagged on Thursday (#883) so will be part of the VulkanSceneGraph-1.0.8 and other project releases.

[drywolf]

Hi, sorry I didn't find the time to test this just yet.