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

[Merged by Bors] - make register on TypeRegistry idempotent #6487

Conversation

jakobhellermann
Copy link
Contributor

Objective

  • adding a new .register should not overwrite old type data
  • separate crates should both be able to register the same type

I ran into this while debugging why register::<Handle<T>> removed the ReflectHandle type data from a prior register_asset_reflect.

Solution

  • make register do nothing if called again for the same type
  • I also removed some unnecessary duplicate registrations

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Nov 5, 2022
@jakobhellermann jakobhellermann added C-Bug An unexpected or incorrect behavior and removed C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 5, 2022
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 5, 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.

LGTM!

I think we may eventually want to add methods for overwriting registrations or removing type data so users can opt out of registrations made by other crates, but that can wait.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 5, 2022
# Objective

- adding a new `.register` should not overwrite old type data
- separate crates should both be able to register the same type

I ran into this while debugging why `register::<Handle<T>>` removed the `ReflectHandle` type data from a prior `register_asset_reflect`.


## Solution

- make `register` do nothing if called again for the same type
- I also removed some unnecessary duplicate registrations
@bors bors bot changed the title make register on TypeRegistry idempotent [Merged by Bors] - make register on TypeRegistry idempotent Nov 5, 2022
@bors bors bot closed this Nov 5, 2022
@@ -87,6 +87,10 @@ impl TypeRegistry {

/// Registers the type described by `registration`.
pub fn add_registration(&mut self, registration: TypeRegistration) {
if self.registrations.contains_key(&registration.type_id()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably more intelligently merge the type data of the registration without overwriting existing data/registrations rather than skipping, but for now I'm fine with this.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed here, but that can come later.

@jakobhellermann jakobhellermann deleted the reflect-register-idempotent branch November 5, 2022 17:07
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- adding a new `.register` should not overwrite old type data
- separate crates should both be able to register the same type

I ran into this while debugging why `register::<Handle<T>>` removed the `ReflectHandle` type data from a prior `register_asset_reflect`.


## Solution

- make `register` do nothing if called again for the same type
- I also removed some unnecessary duplicate registrations
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-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants