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.

Sign up for GitHub

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

Jump to bottom

Mesh::merge to return a Result #17475

Merged
merged 6 commits into from
Jan 23, 2025
Merged

Mesh::merge to return a Result #17475

merged 6 commits into from
Jan 23, 2025

Conversation

Vrixyz
Copy link
Member

@Vrixyz Vrixyz commented Jan 21, 2025 •
edited
Loading

Objective

Make Mesh::merge more resilient to use.

Currently, it's difficult to make sure Mesh::merge will not panic (we'd have to check if all attributes are compatible).

Solution

  • Make Mesh::merge return a Result.

Testing

  • It builds

Migration Guide

  • Mesh::merge now returns a Result<(), MeshMergeError>.

@Vrixyz Vrixyz added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 21, 2025
@Vrixyz Vrixyz mentioned this pull request Jan 21, 2025
Merged
greeble-dev
greeble-dev approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@greeble-dev greeble-dev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added some documentation nitpicks.

I feel like the names of the attributes and attributes_data accessors are now a bit surprising? But I guess that's outside the scope of this PR.

crates/bevy_mesh/src/mesh.rs Outdated Show resolved Hide resolved
crates/bevy_mesh/src/mesh.rs Outdated
/// For example, [`VertexAttributeValues::Float32`] is incompatible with [`VertexAttributeValues::Float32x3`].
///
/// In that case, [`self`] is not modified.
Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't seem true if I'm reading the code right? It could append some attributes but then error out on a later one?

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 21, 2025
@alice-i-cecile @greeble-dev
Mention exact error
404bb25 
Co-authored-by: Greeble <166992735+greeble-dev@users.noreply.github.com>
@alice-i-cecile
Copy link
Member

@Vrixyz ping me when this is ready and I'll get this in.

@BenjaminBrienen BenjaminBrienen added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 22, 2025
@Vrixyz Vrixyz removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jan 22, 2025
@Vrixyz
Copy link
Member Author

Vrixyz commented Jan 22, 2025

I removed the wrong comment, thanks reviewers ; @alice-i-cecile here goes your requested ping.

I feel like the names of the attributes and attributes_data accessors are now a bit surprising? But I guess that's outside the scope of this PR.

Indeed. The new accessor is pub(crate) though so as it's not public facing API, so I'd agree that's outside this PR's scope.

@alice-i-cecile
Copy link
Member

I've resolved merge conflicts for you but CI is 100% gonna gripe about that allow statement :)

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 22, 2025
@alice-i-cecile @BenjaminBrienen
Look mom, no same arms!
f94934c 
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 23, 2025
Merged via the queue into bevyengine:main with commit fd2afee Jan 23, 2025
28 checks passed
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@Vrixyz @alice-i-cecile
@greeble-dev @BenjaminBrienen @mrchantey
Mesh::merge to return a Result (bevyengine#17475)
ef9e06b 
# Objective

Make `Mesh::merge` more resilient to use.

Currently, it's difficult to make sure `Mesh::merge` will not panic
(we'd have to check if all attributes are compatible).

- I'd appreciate it for utility function to convert different mesh
representations such as:
dimforge/bevy_rapier#628.

## Solution

- Make `Mesh::merge` return a `Result`.

## Testing

- It builds

## Migration Guide

- `Mesh::merge` now returns a `Result<(), MeshMergeError>`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Greeble <166992735+greeble-dev@users.noreply.github.com>
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBrienen BenjaminBrienen BenjaminBrienen left review comments

@IceSentry IceSentry IceSentry approved these changes

@greeble-dev greeble-dev greeble-dev approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Rendering
Status: Done
Milestone
0.16
Development

Successfully merging this pull request may close these issues.
5 participants
@Vrixyz @alice-i-cecile @IceSentry @BenjaminBrienen @greeble-dev