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

Decouple serialization and reflection #3664

Closed
alice-i-cecile opened this issue Jan 13, 2022 · 3 comments
Closed

Decouple serialization and reflection #3664

alice-i-cecile opened this issue Jan 13, 2022 · 3 comments
Labels
A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change C-Usability A simple 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

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Reflection and serialization (specifically, serde serialization) are tightly coupled right now.

For example, Option<T>'s Reflect implementation has a Serialize trait bound.

This can make it challenging to model types that should be reflected but not serialized (e.g. for fancy ECS tricks like trait queries or editor inspection, see #3580). Additionally, this makes relying on bevy_reflect without using serde (either without any serialization or with an alternate backend) effectively impossible.

What solution would you like?

  1. Lock all of the serialization code behind a serialize feature flag. serde should only be a dependency with the serde flag enabled.
  2. Modify the impl_reflect_value macro to account for whether or not this feature flag is enabled.
  3. Remove the serialize and deserialize trait bounds on storage types like Option and HashSet. We should ensure that these types can still be serialized if and only if the underlying values are serializable.
  4. Remove the serialize method on Reflect. Instead, create a second ReflectSerialization trait that handles this, and does not return an option. This method should return a Box<dyn Serializable>, allowing users to implement this trait with their own serialization backend.

Alternatives considered

We could keep serialize as a method on Reflect. This is less useful, as being able to distinguish between serializable reflected objects and non-serializable ones at the type level is very relevant in end-user code. It also prevents us from hiding all of the serialization functionality behind a feature flag.

Additional context

Issue created for @kabergstrom, who wants to avoid serde due to excessive compilation times.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Jan 13, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Mar 17, 2022

How would this affect serializing/deserializing scenes? I’m assuming scenes will require the serde feature then, right?

@MrGVSV
Copy link
Member

MrGVSV commented Oct 17, 2022

Does #5197 close this out? It made it so that we don't need T: Serialize and things like that, instead users need to manually register the serializable types like app.register_type_data::<Option<String>, ReflectSerialize>().

@alice-i-cecile
Copy link
Member Author

Yep, I'm calling this closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change C-Usability A simple 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
Status: Done
Development

No branches or pull requests

2 participants