mellinoe / veldrid-samples

Sample projects for Veldrid
https://mellinoe.github.io/veldrid-docs/
121 stars 49 forks source link

Command line argument parser. TexuteCube now illustrates dynamic binding #17

Open gleblebedev opened 5 years ago

gleblebedev commented 5 years ago

Dynamic binding seems to be broken in Direct3D11 and works fine in OpenGL and Vulkan.

Please run the TexturedCube.Desktop sample with one of the following arguments to test the issue:

-g Direct3D11 -g Vulkan -g OpenGL

mellinoe commented 5 years ago

The sample is working fine on D3D11 for me. What issue are you hitting?

gleblebedev commented 5 years ago

Do you see 3 boxes or just one?

gleblebedev commented 5 years ago

I've changed the sample to use dynamic binding

mellinoe commented 5 years ago

I see 3 boxes -- same as the other backends.

gleblebedev commented 5 years ago

Now that becomes interesting. Are you on Nvidia or AMD hardware? Mine is AMD.

mellinoe commented 5 years ago

I'm running on Nvidia right now. I can test AMD later today to see if that exhibits any issue. I'm assuming the problem is that only one cube is rendered?

gleblebedev commented 5 years ago

Yes. It looks like all use the same matrix for me as if offset isn't applied. Where is the method that applies the offset? I'll try to debug it now.

mellinoe commented 5 years ago

I can confirm that it's not working correctly on my AMD machine, so there's some problem there. I don't have all of my dev tools installed on that machine, so I'll need to set some of that up to get more info.

mellinoe commented 5 years ago

The code that actually applies the offset would be here: https://github.com/mellinoe/veldrid/blob/master/src/Veldrid/D3D11/D3D11CommandList.cs#L880

gleblebedev commented 5 years ago

In renderdoc I see vssetconstantbuffers1 is used to set the offset. The offset is 16 and 32 which is correct (16*16 = 256).

gleblebedev commented 5 years ago

What tool would you recommend to use to debug it?

gleblebedev commented 5 years ago

This looks strange. For the first box: image Then the second one: image And the third: image

gleblebedev commented 5 years ago

It looks like pNumConstants is incorrect - it should be 16 or at least 48-firstContant position. Otherwise the end of the buffer exceeds the size of the buffer.

gleblebedev commented 5 years ago

Also the first box is binding to the whole buffer. Should it use VSSetConstantBuffers1 anyway, even with offset = 0?

public bool IsFullRange => Offset == 0 && Size == Buffer.SizeInBytes;

How Buffer.SizeInBytes is set? I don't see a way to supply size of the buffer.

gleblebedev commented 5 years ago

I see if I set DeviceBufferRange(_worldBuffer, 0, 64) to resource set it has now correct size and use VSSetConstantBuffers1 for the first box. But the result is still the same - 1 box on the screen :(

gleblebedev commented 5 years ago

If I set new DeviceBufferRange(_worldBuffer, 256, 64) is uses the second matrix as expected... Kinda...

gleblebedev commented 5 years ago

I found one more interesting thing. If i change first value in the render loop for (int i = "THISVALUE"; i < _worldMatrixOffsets.Length; ++i) then the corresponding box becomes visible....

gleblebedev commented 5 years ago

this looks like something related :)

https://docs.microsoft.com/de-de/windows/win32/api/d3d11_1/nf-d3d11_1-id3d11devicecontext1-vssetconstantbuffers1#calling-vssetconstantbuffers1-with-command-list-emulation

gleblebedev commented 5 years ago

I don't think I can simulate this without changing the Veldrid Direct3D11 code. I hope setting constant buffer to null will work :) May I ask you to test it?

gleblebedev commented 5 years ago

Starting the command list for every box definitely helps. So I'm 99% sure that's the offset not been reset for emulated command list.

image

mellinoe commented 5 years ago

Thanks for that MSDN link. Adding that workaround seems to resolve the issue on my AMD machine. Preview version 4.7.0-ge6647cf4a7 includes the fix for you to test locally. I will push out a stable version of 4.7.0 sometime soon, since that includes a number of other misc. fixes.

mellinoe commented 5 years ago

4.7.0 is uploaded now, so you can update this branch and test that out.

gleblebedev commented 5 years ago

Thanks a million!

gleblebedev commented 5 years ago

I can confirm - it works now. I've updated the PR.

gleblebedev commented 5 years ago

Ok, now the next issue. OpenGL uniform binding is broken at Veldrid.SPIRV version 1.0.12. I've updated the version to illustrate the problem.

It looks very much connected to https://github.com/KhronosGroup/SPIRV-Cross/issues/1107

Uniform binding can't find a uniform: image

Generated OpenGL shader code:

version 330

layout(std140) uniform vdspv_1_0 { mat4 World; } _14;

layout(std140) uniform vdspv_0_1 { mat4 View; } _33;

layout(std140) uniform vdspv_0_0 { mat4 Projection; } _41;

layout(location = 0) in vec3 Position; out vec2 vdspv_fsin0; layout(location = 1) in vec2 TexCoords;

void main() { vec4 worldPosition = _14.World vec4(Position, 1.0); vec4 viewPosition = _33.View worldPosition; vec4 clipPosition = _41.Projection * viewPosition; gl_Position = clipPosition; vdspv_fsin0 = TexCoords; }

As you can see the names are broken now.

gleblebedev commented 5 years ago

I guess that's the reason for the regression: https://github.com/mellinoe/veldrid-spirv/commit/05ac7865f12f6fb345e96bcadc8b4617b4c7c442

gleblebedev commented 5 years ago

I guess there suppose to be code for OpenGL that binds uniforms based on location but it's missing in 4.7 release :(