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

Expand MergeMeshError to include IncompatiblePrimitiveTopology variant #18561

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

Conversation

Pnoenix
Copy link

@Pnoenix Pnoenix commented Mar 26, 2025

Objective

Fix #18546 by adding a variant to MergeMeshError, for incompatible primitive topologies.

Solution

Made MergeMeshError into an enum with two variants; IncompatibleVertexAttributes and IncompatiblePrimitiveTopology.
Added an if statement in Mesh::merge to check if the primitive_topology field of self matches other.
Also renamed MergeMeshError to MeshMergeError to align with the two other MeshSomethingError's.

Testing

Didn't do any.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Pnoenix added 2 commits March 26, 2025 18:03
Pnoenix added 3 commits March 26, 2025 19:33
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 26, 2025
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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 Mar 26, 2025
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@Pnoenix
Copy link
Author

Pnoenix commented Mar 27, 2025

@alice-i-cecile I added a migration guide file, does it look fine?

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Mar 28, 2025
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Could you use a shorter snake case name for the migration guide file? Like merge_mesh_errors.md? @alice-i-cecile do you have preferences about the format?

@alice-i-cecile
Copy link
Member

Agreed; short snake_case name that mentions the type that was changed please.

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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: No status
Development

Successfully merging this pull request may close these issues.

Merging meshes with different primitive topologies leads to rendering panics rather than returning an error
4 participants