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.

Sign up for GitHub

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

Jump to bottom

Merge _Reflect and _WithRegistry commands #9680

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

Merge _Reflect and _WithRegistry commands #9680

wants to merge 1 commit into from
+116 −144

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 3, 2023

Objective

While code was already effectively deduplicated, this further deduplicates it, by removing InsertReflect and RemoveReflect, since they were just specialized version of their _WithRegistry counterparts.

Solution

  • Add ReadTypeRegistry trait, implemented for AsRef<TypeRegistry> and AppTypeRegistry
  • Add default impl for ReflectCommandExt insert_reflect and remove_reflect methods
  • Remove InsertReflect and RemoveReflect in favor of InsertReflectWithRegistry<AppTypeRegistry> and equivalent.

@nicopap nicopap added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Sep 3, 2023
SkiFire13
SkiFire13 reviewed Sep 3, 2023
View reviewed changes
crates/bevy_ecs/src/reflect/entity_commands.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/entity_commands.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/entity_commands.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/entity_commands.rs Outdated Show resolved Hide resolved
alice-i-cecile
alice-i-cecile approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's good to collapse these :) The AsDeref + ReadTypeRegistry gymnastics don't seem particularly worth it to me without more context, but I won't block over that.

@nicopap
Copy link
Contributor Author

nicopap commented Sep 10, 2023

I would have preferred a solution where we implement AsRef<TypeRegistry> or Deref<Target=TypeRegistry> for AppTypeRegistry, but because of the lock guard from RwLock::read, it's not possible to do. I've had several attempts, but finally concluded it wasn't possible. So I created the ReadTypeRegistry trait. The GAT lifetime is because of this.

@SkiFire13
Copy link
Contributor

Note that TypeRegistry itself does not implement AsRef<TypeRegistry> (in general AsRef<T> is not automatically implemented for T, but Borrow<T> is)

@nicopap nicopap force-pushed the entity-commands-cleanup branch from 6f49d32 to 3d42430 Compare September 10, 2023 17:43
@nicopap
Merge InsertReflect and InsertReflectWithRegistry
3baade9 
While code was already effectively deduplicated, this further
deduplicates it, by removing InsertReflect and RemoveReflect, since they
were just specialized version of their _WithRegistry counterparts.

- Add ReadTypeRegistry trait, implemented for AsRef<TypeRegistry>
  and AppTypeRegistry
- Add default impl for ReflectCommandExt insert_reflect and
  remove_reflect methods
- Remove InsertReflect and RemoveReflect in favor of
  InsertReflectWithRegistry<AppTypeRegistry> and equivalent.
@nicopap nicopap force-pushed the entity-commands-cleanup branch from 3d42430 to 3baade9 Compare September 26, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SkiFire13 SkiFire13 SkiFire13 left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicopap @SkiFire13 @alice-i-cecile