vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.3k stars 209 forks source link

Command buffer's old _currentPipelineLayout which has been destoryed using in next frame casue vkCmdPushConstants crash. #476

Closed simwangrui closed 2 years ago

simwangrui commented 2 years ago

Hi, In my project I need dynamic creation and destruction of my RTT RenerGraph.In some case,after clear previous RenderGraph,rebuild a new RenderGraph will casue program crash and vulkan report the error:

VUID-vkCmdPushConstants-layout-parameter(ERROR / SPEC): msgNum: -187369612 - Validation Error: [ VUID-vkCmdPushConstants-layout-parameter ] Object 0: handle = 0x2bbebbb6270, type = VK_OBJECT_TYPE_INSTANCE; | MessageID = 0xf4d4f774 | Invalid VkPipelineLayout Object 0xcfcda0000000001e. The Vulkan spec states: layout must be a valid VkPipelineLayout handle (https://vulkan.lunarg.com/doc/view/1.3.211.0/windows/1.3-extensions/vkspec.html#VUID-vkCmdPushConstants-layout-parameter) Objects: 1 [0] 0x2bbebbb6270, type: 1, name: NULL

I found that in my scene tree if there is a vsg::Command node before all vsg::BindGraphicsPipeline state will crash in vkCmdPushConstants when rebuild a new RenderGraph.For example, the following scene tree:

`auto view_scene_root = vsg::Group::create();

auto set_line_width = vsg::SetLineWidth::create(1.0); #may be other vsg::Command view_scene_root->addChild(set_line_width);

auto real_scene = create_real_scene(); #the scene tree contains vsg::BindGraphicsPipeline view_scene_root->addChild(real_scene);

renderGraph->addChild(view);`

I track the code,found that:command buffer's old _currentPipelineLayout which has been destoryed using in next frame casue vkCmdPushConstants crash.

here is my test code: vsg_dynamic_rendergraph2.zip

robertosfield commented 2 years ago

Hi,

I haven't had a chance to look at your example yet, but my first pass guess is that vkPushConstant requires a vkPipelineLayout and for the VSG implementation osg vsg::PushConstant I chose to leave this something bound during the record traversal using which ever PipelineLayout is current during traversal. However, this design relies upon a BindGraphicsPipeline appearing above it in the scene graph.

This design/implementation choice is broken by your usage case. A quick fix on the VSG side would be to check against the pipeline layout pointer before applying, however, this would stop the crash but leave the push constants unapplied.

A more full proof fix would be to change vsg::PushConsant so that you have to assign a vsg::PipelineLayout, however, that would leave the question of what happens if you independently create and destroy new subgraphs that have their own local vsg::PipelineLayout. If you make sure you share the same PipelineLayout between each subgraph then things would be safe. It's also possible that Vulkan drivers will see that pipeline layouts are compatible and still retain their relevance, but this would likely be sketchy to rely upon.

The other solution is to put the PushConstant directly within the subgraphs that you are creating and destroying.

Cheers, Robert.

simwangrui commented 2 years ago

Hi, Actually, my scene tree is a RenderGraph with three subpasses to rendering Order Independent Transparency.The first pass rendering opaque scene,the second pass rendering transparent scene,the third pass synthesis full scene.some time has no opaque scene.The scene tree is as follow:

scene_tree

image

Because renderpass has three sub passes, I have to use the nextsubpass command as place holder, even if there is no opaque scene.In this case there is no vsg::PipelineLayout above the nextsubpass command,when RecordTraversal arrived nextsubpass, it first do _state->record(), which call vkCmdPushConstants vulkan function,if command buffer try using old _currentPipelineLayout which has been destroyed will cause crashing.

void RecordTraversal::apply(const Command& command) { _state->record(); command.record(*(_state->_commandBuffer)); }

inline void record(CommandBuffer& commandBuffer) { if (dirty) { auto pipeline = commandBuffer.getCurrentPipelineLayout(); auto stageFlags = commandBuffer.getCurrentPushConstantStageFlags();

            // don't attempt to push matrices if no pipeline is current or no stages are enabled for push constants
            if (pipeline == 0 || stageFlags == 0)
            {
                return;
            }

            // make sure matrix is a float matrix.
            mat4 newmatrix(matrixStack.top());
            vkCmdPushConstants(commandBuffer, pipeline, stageFlags, offset, sizeof(newmatrix), newmatrix.data());
            dirty = false;
   }

}

I try to clear command buffer's _currentPipelineLayout to null in the begining of every frame.

void CommandGraph::reset() { for (auto& ec : _executeCommands) ec->reset();

//my added test code
for (auto commandBuffer : _commandBuffers)
{
    commandBuffer->setCurrentPipelineLayout(0);
}

}

It was working ok,But I don't konw whether it is a right way.

Best wishes

robertosfield commented 2 years ago

Thanks for the explanation. Your investigation looks to have settled upon a similar place that I ended up. I have added a vsg::CommandBuffer::reset() method that is implemented as:

void CommandBuffer::reset() { _currentPipelineLayout = VK_NULL_HANDLE; _currentPushConstantStageFlags = 0;

_commandPool->reset();

}

This is called from CommandGraph::record(..) in place of the original commandBuffer->getCommandPool()->reset(); call.

Thanks changed are now merged with VSG master: #478

You example now longer throws out Vulkan debug warnings about vkPushConstant, which is progress. I see lots of other errors that I think are probably down to problems in setup of the testbed example rather than a bug in the VSG.

Could you check out VSG master and let me know whether this resolves the issue for you.

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

simwangrui commented 2 years ago

Hi, I pull the master,now it works well. I track the code,after rebuild the scene graph,when RecordTraversal arrived on my NextSubPass command,the command buffer's _currentPipelineLayout is VK_NULL_HANDLE and _currentPushConstantStageFlags is 0 as expect. VSG is an excellent job, and I can learn a lot from it. I am very happy using VSG to develop my project,Thank you very much.

Best wishes

robertosfield commented 2 years ago

Good to hear that it's working now, and that the VSG is working well for your project.