Fix `set_push_constants` for render bundles

[fyellin]

Fixes https://github.com/gfx-rs/wgpu/issues/6532. Fixes https://github.com/gfx-rs/wgpu/issues/2683.

The use of RenderBundleEncoder.SetPushConstants would cause a panic on the subsequent call to RenderPass.ExecuteBundles(). The .finish() method was failing to copy its push_constant_data from the render bundle encoder to the render bundle.

Currently string_data is unused, but it seemed safest to copy it to prevent future bugs.

I have confirmed with a toy program that the old code panics and the new code works (and correctly accesses the pushed constants).

[fyellin]

self is immutable in the function finish() [see line 345], so I cannot take these vectors from self. Since this is my first time modifying wgpu-core, I don't feel confident enough to change the signature of the method and make self mutable.

I also just verified that I get a compiler error when I change self to &mut self because at least one of the callers to finish wasn't expecting it to be mutable.

I'll rely on your judgment on how to proceed.

On Wed, Nov 13, 2024 at 12:34 PM Nicolas Silva @.***> wrote:

@.**** commented on this pull request.

In wgpu-core/src/command/bundle.rs https://github.com/gfx-rs/wgpu/pull/6540#discussion_r1841124145:

@@ -543,8 +543,8 @@ impl RenderBundleEncoder { label: desc.label.as_ref().map(|cow| cow.to_string()), commands, dynamic_offsets: flat_dynamic_offsets,

• string_data: Vec::new(),
• push_constant_data: Vec::new(),
• string_data: self.base.string_data.clone(),

I think that this should be std::mem::take(&mut self.base.string_data) (and same thing for the push constant data.

— Reply to this email directly, view it on GitHub https://github.com/gfx-rs/wgpu/pull/6540#pullrequestreview-2434355906, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABC2LIIULVTK24UFS5JWZVD2AOZT5AVCNFSM6AAAAABRXDWEIKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZUGM2TKOJQGY . You are receiving this because you authored the thread.Message ID: @.***>

[fyellin]

I realized the argument is self. So we own it. I can just use self.base.<field>. Also added tests to show that things are working now.

[fyellin]

My changes fixed one bug, but it seems there is another bug in the Vulkan and DX12 implementation. For now, I can fix the code so that it works on everything else, but I don't know enough about the internal implementations of Vulkan or DX12. For Vulcan there is an error message that makes no sense. For DX12, naga seems to produce bad code, and there is a compiler failure.

My code in the test suite is just a basic use of SetPushConstant on a RenderPassEncoder and a RenderBundleEncoder. The fact that the code is now running successfully on RenderBundleEncoders means that I've fixed the generic bug. Someone else is going to need to fix the implementation specific bugs, since I don't know about the internals.

[teoxoy]

The DX12 issue is https://github.com/gfx-rs/wgpu/issues/5683 / https://github.com/gfx-rs/wgpu/issues/5571, can you wrap the push constants in the shader in a struct? That should get things working.

I think the Vulkan validation error is https://github.com/gfx-rs/wgpu/issues/4502. Can you change the test to not use non-0 start ranges? This will most likely become a validation error emitted by us in the future as the WebGPU spec proposal also doesn't allow users to specify non-0 start ranges.

[fyellin]

I've converted the code to use a struct. The naga compiler does the right thing now.

Since push_constants no longer exists in WGPU or WebGPU, I was using the Vulkan documentation as my guide. According to those specs, one can separate the vertex shader constants from the fragment shader constants, and specify that a certain byte-range of the constants are are only for the vertex shader or only for the fragment shader. When doing this, you are also required to call set_push_constants separately for each of the byte-ranges.

So I performed multiple tests.

1. Separate constants for the vertex shader and fragment shader.

   • Both Linux/Vulkan and Windows/DX12 failed. Linux gave the message that the "set_push_constant" was out of range. Windows gave the wrong answer.

2. Create a single constant pool shared by both shaders. This seems to be the intent of the spec proposal you mentioned above.

   • Linux and Metal passed. Windows/DX12 ran, but gave the wrong answer.

3. Create a single constant pool, but use two separate set_push_constant operations to separately push the constants intended for the vertex shader and those intended for the fragment shader.

   • The results were the same as previously. Windows/DX12 ran, but gave the wrong answer.

It seems that there are two possibly new bugs:

• Linux/Vulkan cannot handle having the constants separated by stages, as per the Vulkan spec.
• Windows/DX12 is just broken. It also seems that there is no problem with using a non-zero offset. Vulkan's problem is dealing with multiple stages.

I will resubmit my PR using #2 above, and exclude DX12.

[fyellin]

I have opened two additional bugs:

BUG #6558. SetPushConstant on Dx12 gives wrong results BUG #6562. Unable to do SetPushConstant on Vulcan with separate Vertex and Fragment Stages

This PR fixes yet a third bug and adds a test that succeeds on GL and Metal and actually tests SetPushConstant. BUG #6532. SetPushConstant always crashes.

How do I proceed? Right now, the test suite isn't even running.

[teoxoy]

Thanks for looking into these issues!

I left a comment in https://github.com/gfx-rs/wgpu/issues/6558#issuecomment-2482643043, could you try seeing if avoiding arrays gets things working on DX12?