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

Implement Reflect for TypeId #4976

Closed
wants to merge 0 commits into from

Conversation

PROMETHIA-27
Copy link
Contributor

Objective

Allows reflection and serialization of TypeIds. This is helpful for serializing and deserializing certain types that use TypeIds and are otherwise reflectable/serializable.

Solution

  • Implement TypeIds as reflect values with Hash and PartialEq
  • Hardcode TypeId serialization as an alternative to Value serialization, since TypeIds need access to the TypeRegistry to be serialized/deserialized and otherwise cannot access it. TypeIds are de/serialized as their corresponding type's name.

Issues:

  • Hardcoding a new serialization method for one type is gross, especially when it's entirely possible other types might end up also needing a TypeRegistry for serialization.
  • Serialization format can be simplified to TypeId("{type name here}") once 4561 hits

Alternative solutions:

  • Don't implement reflect for TypeIds
    • Would entirely prevent behavior which is probably logically sound and useful
  • Implement reflect without serialization
    • It should be sound to allow TypeIds to be serialized to a type name and back, so this doesn't seem like a good approach
  • Allow any serialized types to access the TypeRegistry during de/serialization
    • Add it to thread local storage?
    • Pass it as a parameter?

@PROMETHIA-27 PROMETHIA-27 requested a review from MrGVSV June 9, 2022 16:27
crates/bevy_reflect/src/serde/ser.rs Outdated Show resolved Hide resolved
@Weibye Weibye added A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 9, 2022
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Looks good to me!

crates/bevy_reflect/src/serde/de.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/serde/ser.rs Outdated Show resolved Hide resolved
@MrGVSV
Copy link
Member

MrGVSV commented Jun 9, 2022

Allow any serialized types to access the TypeRegistry during de/serialization

  • Add it to thread local storage?
  • Pass it as a parameter?

Reflecting on this more, I think you're right. It's probably best to allow the serialization method to access the registry (even if #4782 lands) as this gives the best control and flexibility for types going forward.

We could probably add that functionality to #4782 itself or create a new PR for it (I don't think it should be included as part of this one).

@PROMETHIA-27
Copy link
Contributor Author

Agreed, that's a big change beyond the scope of this PR. I think it probably belongs in a new PR. I'm also not certain on how it should be done.

@MrGVSV
Copy link
Member

MrGVSV commented Jun 9, 2022

I'm also not certain on how it should be done.

I’m in favor of the parameter method. The TLS solution will probably be a lot more controversial and a bigger change.

So just adding it as a parameter is probably simplest and most likely to be approved for now

@PROMETHIA-27
Copy link
Contributor Author

I'll investigate that and open a PR if I can get it working

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

@PROMETHIA-27, once this is rebased and the serialization fixed (remember to fix the PR description) I'm happy to merge this.

Other code LGTM.

@PROMETHIA-27
Copy link
Contributor Author

Making a new PR for this soon:tm:

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 C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants