Skip to content

Fix vk::TransformMatrixKHR#1036

Closed
0xbeefd1ed wants to merge 2 commits into
ash-rs:masterfrom
0xbeefd1ed:fix-transform-khr
Closed

Fix vk::TransformMatrixKHR#1036
0xbeefd1ed wants to merge 2 commits into
ash-rs:masterfrom
0xbeefd1ed:fix-transform-khr

Conversation

@0xbeefd1ed
Copy link
Copy Markdown

This is a regression caused by #951.

matrix is a 3x4 row-major affine transformation matrix.

So the matrix should have 3 rows, each row with 4 elements.

After #951, to convert a glam::Affine3A into vk::TransformMatrixKHR you'd write

pub fn glam_to_vk_transform(affine: Affine3A) -> vk::TransformMatrixKHR {
    // affine.matrix3 is column-major
    let x = &affine.matrix3.x_axis; // first column
    let y = &affine.matrix3.y_axis; // second column
    let z = &affine.matrix3.z_axis; // third column
    let w = &affine.translation;
    vk::TransformMatrixKHR {
        // row major
        matrix: [[x.x, y.x, z.x], [w.x, x.y, y.y], [z.y, w.y, x.z], [y.z, z.z, w.z]], // This is clearly wrong
    }
}

This is clearly wrong. This PR fixes it by correcting the array dimensions.

  • Alternatively, just keep the flat array structure as before.

@Ralith
Copy link
Copy Markdown
Collaborator

Ralith commented May 6, 2026

Thanks for catching this! I concur that the dimensions are reversed. However, we cannot accept this PR as written because this is generated code; what needs to be fixed is the generator, which is probably misinterpeting nested C array syntax in the XML.

@0xbeefd1ed
Copy link
Copy Markdown
Author

@Ralith thanks for calling that out. I've fixed the generator as well. This part was entirely hand-written.

Copy link
Copy Markdown
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@0xbeefd1ed
Copy link
Copy Markdown
Author

Closing, as the maintainer has fixed this in 69afb6b without a PR.

@0xbeefd1ed 0xbeefd1ed closed this May 6, 2026
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

Successfully merging this pull request may close these issues.

2 participants