vsg-dev / vsgXchange

Utility library for converting data+materials to/from VulkanSceneGraph
MIT License
65 stars 39 forks source link

Don't share descriptors or descriptor sets #174

Closed timoore closed 6 months ago

timoore commented 11 months ago

Calling sharedObjects->share() on descriptor sets was resulting in shared structure that would royally screw up the later calls to vsg::DescriptorConfigurator::assignDefaults().

robertosfield commented 11 months ago

This removes an important performance optimization so we'll need to figure out exactly what the problem is before taking a sledgehammer to the problem.

Could you provide a guide to how to recreate problems this code causes?

timoore commented 11 months ago

This removes an important performance optimization so we'll need to figure out exactly what the problem is before taking a sledgehammer to the problem.

Could you provide a guide to how to recreate problems this code causes?

This comes about when passing a custom shader set via vsg::Options, and using it to create the material descriptor set on a set number > 0, in my case 3. The DescriptorConfigurator adds empty descriptor sets to the lower-numbered entries for sets as it accumulates bindings. If these are still empty at the end if material conversion, then share() will replace them all with the same descriptor set object. Later, when DescriptorConfigurator and GraphicsPipelineConfigurator assigns default descriptor bindings, chaos ensues as defaults from different descriptor sets are all jumbled up.

I know that share() is considered an important optimization, but GraphicsPipelineConfigurator::copyTo() calls share() again on the descriptor sets and the graphics pipeline. I'm not clear on any recursive behavior of share(). If in assimp.cpp you wanted to call share() on the descriptor objects, but not the descriptor set objects, then I think that fix the bug too.

robertosfield commented 8 months ago

@timoore I'm just looking over old PR's, what is the status of this PR now? Is it still needed?