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

Alternative fix for mesh2d_manual example #15883

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Oct 13, 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

@rparrett rparrett changed the title Fix mesh2d_manual example Alternative fix for mesh2d_manual example Oct 13, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 13, 2024
@alice-i-cecile alice-i-cecile added the A-Rendering Drawing game state to the screen label Oct 13, 2024
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

I like this more than the alternative,
can confirm the example works

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

I think it makes sense to demonstrate something actually custom. Thanks!

@IceSentry
Copy link
Contributor

Why does the current code not work? AFAIK attributes haven't changed at all for a long time?

@rparrett
Copy link
Contributor Author

Why does the current code not work? AFAIK attributes haven't changed at all for a long time?

@IceSentry It was mistake during the migration in #15862. During the process of figuring out why mesh2d_manual wasn't working, it was tidied up to not use the built-in color vertex attribute instead of the custom one. But the shader code wasn't updated. The underlying problem with the example was fixed in another way.

So the question right now is just "do we want to use a custom vertex attribute in mesh2d_manual or the built-in one?".

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Okay, thank you for the context, in that case, yeah I'm happy to use a custom attribute here.

@IceSentry IceSentry added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 14, 2024
Merged via the queue into bevyengine:main with commit 9dd6f42 Oct 14, 2024
34 checks passed
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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

mesh2d_manual example doesn't render correctly
6 participants