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

Allow SceneMapSerializer to accept Box<dyn Reflect>s as well #17622

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aecsocket
Copy link
Contributor

Objective

NOTE: This is NOT a working PR! See below.

Currently, SceneMapSerializer looks like this:

pub struct SceneMapSerializer<'a> {
    /// List of boxed values of unique type to serialize.
    pub entries: &'a [Box<dyn PartialReflect>],
// ...

Which works fine for the use-case of SceneSerializer and EntitySerializer, since components there are PartialReflects. However, I want to serialize a Vec<Box<dyn Reflect>> using this serializer, which is currently impossible without cloning the Box<dyn Reflect>s first.

Solution

This PR attempts to solve this issue by changing the signature to:

pub struct SceneMapSerializer<'a, Entry: AsRef<dyn PartialReflect>> {
    /// List of values of unique type to serialize.
    pub entries: &'a [Entry],
// ...

Since the serializer logic doesn't actually need a Box<dyn PartialReflect>, it just needs some way to get a &dyn PartialReflect from the entry. This means that Vec<Box<dyn PartialReflect>>, Vec<&dyn PartialReflect>, and Vec<Arc<dyn PartialReflect>> can all be serialized now.

However, this doesn't solve the original issue of serializing Box<dyn Reflect>s since that doesn't impl AsRef<dyn PartialReflect>. I'm not sure how to solve this, so I'd like to open this PR and see if anyone else has a good idea on how to achieve this.

Current ideas:

  • Use CastPartialReflect from bevy_reflect: Add casting traits to support Box and other wrapper types #15532
    • Would be clean, since a Vec<&dyn PartialReflect> and Vec<&dyn Reflect> could be accepted into the same interface
  • Make entries a impl Iterator<Item = impl AsRef<dyn PartialReflect>>, and manually entries.iter().map(Reflect::as_partial_reflect) when constructing the SceneMapSerializer
    • Most flexible solution, and it also allows users to apply extra logic like filtering, mapping beforehand
    • Does not work with serde::Serialize since fn serialize(&self) takes a shared ref to self, but we would need to mutate the iterator to use it (unless we mutex, but like, really?)
      • Is there some alternate trait to serde::Serialize we can use?
    • You could pass in a Vec<&dyn Reflect> but you need to do that iterator mapping trick, so it's not quite as clean as the first solution
  • Some combination of the 2? impl Iterator<Item = impl CastPartialReflect>?
  • Are we ok with the extra generic in SceneMapSerializer?

Testing

Added a unit test to serde.rs which should be used to validate the outcome of this PR. It checks that passing in different combinations of Box<..>, Arc<..>, PartialReflect and Reflect all compile.

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
  • Simply adding new functionality is not a breaking change.
  • Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Scenes Serialized ECS data stored on the disk S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Design This issue requires design work to think about how it would best be accomplished C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible labels Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants