Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rows and Columns not reflecting properly #131

Open
redorav opened this issue Nov 12, 2021 · 9 comments
Open

Rows and Columns not reflecting properly #131

redorav opened this issue Nov 12, 2021 · 9 comments

Comments

@redorav
Copy link

redorav commented Nov 12, 2021

I'm having a couple of issues with shader reflection for matrices at the moment.

  1. A simple shader with a 3x4 or 4x3 matrix always comes out with the dimensions flipped around. Another example, a 1x4 matrix says it has 4 rows and 1 column, always.

  2. Matrices are also not decorated with SPV_REFLECT_DECORATION_ROW_MAJOR or the COLUMN_MAJOR variant, the decoration flags are always 0.

  3. I'm also not understanding the stride, but it might be consistent with the rows and columns being flipped. The 3x4 matrix has a stride of 64 whereas the 4x3 has a stride of 48 bytes, whereas I would expect the opposite.

The issues all probably come from the fact that this code doesn't seem to account for them. Would it be possible to confirm whether this is indeed a bug? Thanks!

case SpvOpTypeMatrix: {
        p_type->type_flags |= SPV_REFLECT_TYPE_FLAG_MATRIX;
        uint32_t column_type_id = (uint32_t)INVALID_VALUE;
        IF_READU32(result, p_parser, p_node->word_offset + 2, column_type_id);
        IF_READU32(result, p_parser, p_node->word_offset + 3, p_type->traits.numeric.matrix.column_count);
        SpvReflectPrvNode* p_next_node = FindNode(p_parser, column_type_id);
        if (IsNotNull(p_next_node)) {
          result = ParseType(p_parser, p_next_node, NULL, p_module, p_type);
        }
        else {
          result = SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_ID_REFERENCE;
          SPV_REFLECT_ASSERT(false);
        }
        p_type->traits.numeric.matrix.row_count = p_type->traits.numeric.vector.component_count;
        p_type->traits.numeric.matrix.stride = p_node->decorations.matrix_stride;
        // NOTE: Matrix stride is decorated using OpMemberDecoreate - not OpDecoreate.
        if (IsNotNull(p_struct_member_decorations)) {
          p_type->traits.numeric.matrix.stride = p_struct_member_decorations->matrix_stride;
        }
      }
      break;
@chaoticbob
Copy link
Contributor

Hi,

It sounds like SPIRV-Reflect is getting confused row_major and column_major attributes confused. Would you happen to have a simple test case?

Thansks,
Hai

@redorav
Copy link
Author

redorav commented Nov 12, 2021

Yeah something really simple like below should do the trick. I compile it through glslangValidator so that the reflection data isn't optimized out like when using dxc (I use this for reflection). Would that be enough?

struct Instance
{
    row_major float1x4 local2WorldA;
    row_major float3x4 local2WorldB;
};

cbuffer Instance
{
	Instance cb_Instance;
};

float4 main() : SV_Target0
{	
    return float4(0.0, 0.0, 0.0, 0.0);
}

@chaoticbob
Copy link
Contributor

Yes, thanks! Give me a couple of days to find another chunk of time to look at this.

@redorav
Copy link
Author

redorav commented Nov 12, 2021

example.txt

Here's also an example of the spir-v I use to generate the reflection data (it has a bit more data, look for local2World, also rename to .spv) Hopefully we can get to the bottom of it. Thanks

@redorav
Copy link
Author

redorav commented Nov 12, 2021

I also found a couple of links which might be useful. I produce SPIR-V from hlsl, but it looks like SPIR-V has a different idea of what column/row major is.

https://github.com/microsoft/DirectXShaderCompiler/blob/master/docs/SPIR-V.rst#vectors-and-matrices

https://github.com/microsoft/DirectXShaderCompiler/blob/master/docs/SPIR-V.rst#appendix-a-matrix-representation

SPIRV-Reflect might be correct in the way it's interpreting rows and columns (so I would have to flip them), and perhaps the only bug here is that they aren't tagged as ColMajor or RowMajor. Let me know your thoughts

@chaoticbob
Copy link
Contributor

Thanks so much for digging into this. Apologies for follow up. I talked to one of my colleagues about this and they reminded me that there was a bit of historical confusion between what the user may perceives as the rows / cols and what's in the SPIR-V. I'll take a look at the decorations.

@chaoticbob
Copy link
Contributor

Hopefuly I didn't misunderstand something, but I checked the row and major decorations in the block variables and it looks like the decorations have row and column major bit set where appropriate.

As mentioned in the links you provided, the row/colum major ordering is flipped between the HLSL source and the SPIR-V if there is an explicit row_major or column_major applied - otherwise it will default to row_major .

I added two test cases for this:
https://github.com/KhronosGroup/SPIRV-Reflect/blob/master/tests/glsl/matrix_major_order_glsl.glsl
https://github.com/KhronosGroup/SPIRV-Reflect/blob/master/tests/hlsl/matrix_major_order_hlsl.hlsl

The SPIR-V for local2World shows 2 member decorate for ColMajor:

               OpMemberDecorate %33 0 ColMajor
               OpMemberDecorate %33 0 Offset 0
               OpMemberDecorate %33 0 MatrixStride 16
               OpMemberDecorate %33 1 ColMajor   // local2World
               OpMemberDecorate %33 1 Offset 16
               OpMemberDecorate %33 1 MatrixStride 16
               OpMemberDecorate %33 2 ColMajor
               OpMemberDecorate %33 2 Offset 80
               OpMemberDecorate %33 2 MatrixStride 16
               OpMemberDecorate %34 0 Offset 0

@redorav
Copy link
Author

redorav commented Jan 10, 2022

Hi @chaoticbob that they are flipped is the conclusion I got from reading that documentation, I wanted your confirmation as well. In terms of the decoration_flags I don't really see them when I check something simple like:

if (type.type_flags & SPV_REFLECT_TYPE_FLAG_MATRIX)
{
	bool isRowMajor = type.decoration_flags & SPV_REFLECT_DECORATION_ROW_MAJOR;
	bool isColMajor = type.decoration_flags & SPV_REFLECT_DECORATION_COLUMN_MAJOR;
}

Both are false. Maybe I need a new version of the library? I thought I was relatively up to date. I'll try a new one and get back to you.

@redorav
Copy link
Author

redorav commented Jan 10, 2022

I updated and still not able to see the decorations. This is what the type looks like:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants