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

fix mesh2d_manual example #15862

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

awtterpip
Copy link
Contributor

Objective

Solution

  • Change mesh2d_manual to use the proper vertex format for color.

Testing

  • The mesh2d_manual example now works for me, tested on linux

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 12, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 12, 2024
@rparrett rparrett requested a review from Jondolf October 12, 2024 00:29
@@ -264,7 +264,7 @@ fn vertex(vertex: Vertex) -> VertexOutput {
let model = mesh2d_functions::get_world_from_local(vertex.instance_index);
out.clip_position = mesh2d_functions::mesh2d_position_local_to_clip(model, vec4<f32>(vertex.position, 1.0));
// Unpack the `u32` from the vertex buffer into the `vec4<f32>` used by the fragment shader
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is no longer needed

@rparrett
Copy link
Contributor

rparrett commented Oct 12, 2024

This looks good, but I'm curious about why we changed the vertex attribute format in that migration. I had the impression that we were intentionally using a custom vertex attribute in that example, but I could very easily be mistaken.

@rparrett
Copy link
Contributor

rparrett commented Oct 12, 2024

Would like to know if the change away from the custom vertex attribute in #15847 was intentional or desired before approving.

This fixes the issue, but it could also be fixed by switching back to the custom u32 based color attribute, probably. And I think that example may have been doing that intentionally.

@Jondolf
Copy link
Contributor

Jondolf commented Oct 12, 2024

@rparrett It was half-intentional, but can be reverted. I originally changed it to fix a crash caused by conflicting vertex color formats with default mesh materials (see this) but then fixed it in a different way as per Cart's suggestion. I forgot to change it back and thought it was cleaner like that anyway, since it's the default format that Mesh::ATTRIBUTE_COLOR uses.

My bad though! Didn't anticipate that it would break things or be particularly undesirable (I recall testing it and it worked like normal...)

@alice-i-cecile alice-i-cecile added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

mesh2d_manual example doesn't render correctly
4 participants