Skip to content

Mesh serialization #19545

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

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

Mesh serialization #19545

wants to merge 6 commits into from

Conversation

yrns
Copy link
Contributor

@yrns yrns commented Jun 9, 2025

Objective

  • Make Mesh serializable.

Solution

  • Implement Serialize for Mesh and Handle (as an AssetPath).
  • Note Mesh does not impl Deserialize because it contains a handle which aren't directly deserializable from asset paths. But it can be deserialized via reflection. I have an example of round-trip saving and loading meshes with an asset loader and reflect deserialization processor that I can submit in a following PR.
  • This is behind a non-default feature ("serialize").

Testing

  • cargo test -p bevy_mesh --features serialize

@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Jun 9, 2025
@IceSentry IceSentry added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 9, 2025
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jun 9, 2025
@alice-i-cecile alice-i-cecile requested review from andriyDev and cart June 9, 2025 05:04
@alice-i-cecile
Copy link
Member

I like the idea (and this is very useful), but I'm not sure what our plans are for handling Handle serialization. Pinging some experts for review.

match self.path() {
Some(asset_path) => asset_path.serialize(serializer),
// Maybe this should be an error?
None => AssetPath::default().serialize(serializer),
Copy link
Member

Choose a reason for hiding this comment

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

This definitely feels like it should be an error case: this will cause serious bugs if people don't catch this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. I think we can return an error with S::Error::custom("blah").

Copy link
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

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

I don't think I'm substantially against serializing handles, though the fact that we don't have a way to deserialize them is quite sad and could lead to users asking for that (which just isn't possible in the current design).

I'm not fully clear on whether Mesh should be Serialize at all. Makes me a little uncomfy for some reason, but I won't block on that.

As an aside, I have been dreaming of doing the cursed thing and moving our asset loading stuff to a static to resolve a bunch of our woes (but probably introduce a few more). Could be worth exploring!

match self.path() {
Some(asset_path) => asset_path.serialize(serializer),
// Maybe this should be an error?
None => AssetPath::default().serialize(serializer),
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. I think we can return an error with S::Error::custom("blah").

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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants