Skip to content

fix mesh2d_manual example#15862

Closed
awtterpip wants to merge 2 commits intobevyengine:mainfrom
awtterpip:fix-mesh2d-manual
Closed

fix mesh2d_manual example#15862
awtterpip wants to merge 2 commits intobevyengine:mainfrom
awtterpip:fix-mesh2d-manual

Conversation

@awtterpip
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 rparrett requested a review from IceSentry October 12, 2024 00:32
@rparrett
Copy link
Copy Markdown
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
Copy Markdown
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
@BenjaminBrienen BenjaminBrienen added the C-Bug An unexpected or incorrect behavior label Oct 13, 2024
@alice-i-cecile
Copy link
Copy Markdown
Member

#15883 was selected over this PR because it better preserves the intent of the example.

github-merge-queue bot pushed a commit that referenced this pull request Oct 14, 2024
# Objective

Fixes #15847

Alternative to #15862. Would appreciate a rendering person signaling
preference for one or the other.

## Solution

Partially revert the changes made to this example in #15524.

Add comment explaining that the non-usage of the built-in color vertex
attribute is intentional.

## Testing

`cargo run --example mesh2d_manual`
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-Bug An unexpected or incorrect behavior 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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

mesh2d_manual example doesn't render correctly

5 participants