Skip to content

Added optional serde derives for rendering types. #20396

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 3 commits into
base: main
Choose a base branch
from

Conversation

mintlu8
Copy link
Contributor

@mintlu8 mintlu8 commented Aug 3, 2025

Objective

Certain types in bevy_render, bevy_pbr, and bevy_sprite have trivial serde implementations but currently doesn't. Adding them can benefit developers of prefab crates.

Solution

Added serde implementations for these types.

Testing

Cargo build with the serialize feature.

@janhohenheim
Copy link
Member

Some notes:

  • The vibe regarding this in general is that rendering types should not be serialized. Hence why e.g. SerializedMesh exists.
  • Still, I think it's alright to implement them on these types. I certainly wouldn't veto it, though I wonder if using a serliaez impl on these types is pointing to underlying issues in the prefab impl
  • What does ExtendedMaterial do when its generics are not Serialize or Deserialize?
  • If we merge this, I would want to also conditionally #[cfg_attr(feature="serialize", reflect(Serialize, Deserialize))

@janhohenheim janhohenheim added A-Rendering Drawing game state to the screen X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Aug 3, 2025
@mintlu8
Copy link
Contributor Author

mintlu8 commented Aug 3, 2025

Some notes:

  • The vibe regarding this in general is that rendering types should not be serialized. Hence why e.g. SerializedMesh exists.

Rendering types cannot be serialized because Handle exists. Types that doesn't contain Handle can be easily serialized. (We could do some thread local shenanigans but thats way out of scope for this.)

  • Still, I think it's alright to implement them on these types. I certainly wouldn't veto it, though I wonder if using a serliaez impl on these types is pointing to underlying issues in the prefab impl

It's more if the prefab crate want to express something like AlphaMode it doesn't have to add a custom type.

  • What does ExtendedMaterial do when its generics are not Serialize or Deserialize?
  • If we merge this, I would want to also conditionally #[cfg_attr(feature="serialize", reflect(Serialize, Deserialize))

Added Reflect impls, although for ExtendedMaterial I'm not sure since the type bounds do not match.

@janhohenheim
Copy link
Member

It's less about Handle and more about not wanting people to depend on the format of various render types like Mesh.
It's probably fine in this case? Pinging @tychedelia

@tychedelia
Copy link
Member

Having a conversation with some of the other rendering folks about how we want to approach this as it's coming up a lot recently.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Aug 4, 2025
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants