implement insert and remove reflected entity commands #8895

NoahShomette · 2023-06-20T00:56:46Z

Objective

To enable non exclusive system usage of reflected components and make reflection more ergonomic to use by making it more in line with standard entity commands.

Solution

Implements a new EntityCommands extension trait for reflection related functions in the reflect module of bevy_ecs.
Implements 4 new commands, insert_reflect, insert_reflect_with_registry, remove_reflect, and remove_reflect_with_registry. Both insert commands take a Box<dyn Reflect> component while the remove commands take the component type name.
Made EntityCommands fields pub(crate) to allow access in the reflect module. (Might be worth making these just public to enable user end custom entity commands in a different pr)
Added basic tests to ensure the commands are actually working.
Documentation of functions.

Changelog

Added:

Implements 4 new commands on the new entity commands extension.
insert_reflect
remove_reflect
insert_reflect_with_registry
remove_reflect_with_registry

The commands operate the same except the with_registry commands take a generic parameter for a resource that implements AsRef<TypeRegistry>. Otherwise the default commands use the AppTypeRegistry for reflection data.

Changed:

Made EntityCommands fields pub(crate) to allow access in the reflect module.

Hopefully this time it works. Please don't make me rebase again ☹

MrGVSV

Looking good. Along with my general nitpicks and suggestions, I left a few comments open for discussion for other reviewers.

Overall, I'm still on the fence about this change. On the one hand, it makes reflection more immediately useful to users without having to understand the complexities of ReflectComponent. On the other hand, the actual command is pretty straightforward if someone wanted to just write it themselves with a custom command or closure command (not trying to downplay this change btw, just want us to be mindful of what we put on a very core API).

It'd be nice to get some input from someone more on the ECS side of things (since for me, all the reflection stuff looks great!).

crates/bevy_reflect/src/type_registry.rs

MrGVSV · 2023-06-20T04:16:21Z

crates/bevy_ecs/src/reflect/entity_commands.rs

+/// An extension trait for [`EntityCommands`] for reflection related functions
+pub trait EntityCommandsReflectExtension {


Discussion: This is based on my suggestion to use an extension trait in order to help create a separation from reflection-based behaviors. My original reasoning was that we don't want beginner users reaching for this option right out of the gate (especially since reflection still has some quirks that make it not very beginner friendly). And hopefully we can bring this in fully once reflection/scenes are a little more fleshed out.

However,— to argue against myself for a moment,— it's a seemingly "unnecessary" separation from a user perspective, especially since it requires a completely separate import. On top of this, it makes finding this functionality much more difficult for users who do need it.

I'd like to hear feedback from others (including you, @NoahShomette haha) on opinions here. Should we just make these proper methods on EntityCommands?

I think the separation makes logical sense and I do like it being separated because at least from a code perspective it keeps all the more special purpose reflection functions away from the general functions which should be much more commonly used. However it could go either way and I don't really have a strong opinion on it. I do think if we added more functions then the division might get more useful just from a code base perspective since the actual EntityCommands file is getting rather big already. It also depends on how "public" reflection related things are supposed to be which I don't have a great gauge on so I'll defer to others on that point.

Additionally I know at least for me I use an IDE and I very rarely purposefully import things. I might be an outlier but I would assume most people use something that will autocomplete and suggest related functions even if they aren't already imported which would make the discoverability point much less damning.

I'd overall be in favor of keeping the extension trait.

crates/bevy_ecs/src/reflect/entity_commands.rs

NoahShomette · 2023-06-20T23:56:08Z

Looking good. Along with my general nitpicks and suggestions, I left a few comments open for discussion for other reviewers.

Overall, I'm still on the fence about this change. On the one hand, it makes reflection more immediately useful to users without having to understand the complexities of ReflectComponent. On the other hand, the actual command is pretty straightforward if someone wanted to just write it themselves with a custom command or closure command (not trying to downplay this change btw, just want us to be mindful of what we put on a very core API).

It'd be nice to get some input from someone more on the ECS side of things (since for me, all the reflection stuff looks great!).

I definitely agree its a small change and easily implemented by the end user (although EntityCommands needs public fields for that technically first), however it was a bit disconcerting to me that there wasn't any way to deferred insert reflect components when I ran into it. I think Bevy focuses enough on this and its a big part of end user actions, specifically interacting with commands and thinking deferred and parallel, that it makes sense to me to have first party support for this kind of thing. Its a little thing but it helps to make reflect more usable and more in line with the rest of the engine imo.

# Objective - Use `AppTypeRegistry` on API defined in `bevy_ecs` (#8895 (comment)) A lot of the API on `Reflect` depends on a registry. When it comes to the ECS. We should use `AppTypeRegistry` in the general case. This is however impossible in `bevy_ecs`, since `AppTypeRegistry` is defined in `bevy_app`. ## Solution - Move `AppTypeRegistry` resource definition from `bevy_app` to `bevy_ecs` - Still add the resource in the `App` plugin, since bevy_ecs itself doesn't know of plugins Note that `bevy_ecs` is a dependency of `bevy_app`, so nothing revolutionary happens. ## Alternative - Define the API as a trait in `bevy_app` over `bevy_ecs`. (though this prevents us from using bevy_ecs internals) - Do not rely on `AppTypeRegistry` for the API in question, requring users to extract themselves the resource and pass it to the API methods. --- ## Changelog - Moved `AppTypeRegistry` resource definition from `bevy_app` to `bevy_ecs` ## Migration Guide - If you were **not** using a `prelude::*` to import `AppTypeRegistry`, you should update your imports: ```diff - use bevy::app::AppTypeRegistry; + use bevy::ecs::reflect::AppTypeRegistry ```

nicopap · 2023-06-23T07:37:44Z

With #8901, it's now possible to use AppTypeRegistry in bevy_ecs. I think it might make some of the implementation simpler.

nicopap · 2023-07-09T06:49:32Z

FYI if you want this to go forward, you need to address the comments reviewers made.

…_reflected_with_registry

…flectExtension

NoahShomette · 2023-07-12T06:05:28Z

Apologies for the absolutely horrible PR timeline now, but I think it should be fixed from a commit point of view and all of the conversations should be resolved.

I added additional insert_reflected_with_registry and remove_reflected_with_registry commands as well and made the standard commands insert_reflected and remove_reflected use the AppTypeRegistry for their reflection data.

Also apologies for the delay in updating this but I fell ill and just got super busy :(

NoahShomette · 2023-07-12T06:14:20Z

I'm not sure what the standard is for commands, but would something like this be possible for maximum flexibility?

We do have some operations in the actual command call but it allows any TypeRegistry to be used rather than just resources and it only involves cloning a ReflectComponent (Which I dont believe would be a huge problem)

    fn remove_reflected_with_registry(
        &mut self,
        component_type_name: String,
       type_registry: &TypeRegistry
    ) -> &mut Self {
       let type_info = self.component.type_name();
       let Some(type_registration) = registry.read().get_with_name(type_info) else {
                panic!("Could not get type registration (for component type {}) because it doesn't exist in the TypeRegistry.", component.type_name());
            }
      let Some(reflect_component) = type_registration.data::<ReflectComponent>() else {
                    panic!("Could not get ReflectComponent data (for component type {}) because it doesn't exist in this TypeRegistration.", component.type_name());
                }

        self.commands.add(RemoveReflectedWithRegistry {
            entity: self.entity,
            reflect_component: reflect_component.clone(),
        });
        self
    }

nicopap

This is the first half of my review. I'm giving feedback on the actual code next.

crates/bevy_ecs/src/reflect/entity_commands.rs

nicopap · 2023-07-27T08:47:34Z

crates/bevy_ecs/src/reflect/entity_commands.rs

+    /// }
+    ///
+    /// ```
+    fn remove_reflected(&mut self, component_type_name: String) -> &mut Self;


I'd suggest using a impl Into<Cow<'static, str>> here instead of String so that it's possible to use the API without allocation.

I added this, I believe I put it in correctly but I would appreciate a double check as I wasn't able to get it to work without having to call .to_owned() when passing stuff into the function and I'm not sure if thats an issue with it now. I was getting lifetime bounds errors which you can replicate by just removing the .to_owned() from the tests

crates/bevy_ecs/src/reflect/entity_commands.rs

nicopap

Looks excellent! I'd require just some minor adjustments.

crates/bevy_ecs/src/reflect/entity_commands.rs

NoahShomette · 2023-08-17T16:46:21Z

Looks excellent! I'd require just some minor adjustments.

Awesome thank you so much! I implemented all your feedback so it should be good to go from that regards!

github-actions · 2023-08-17T16:55:31Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

nicopap

Great job! LGTM

nicopap · 2023-08-17T20:28:19Z

crates/bevy_ecs/src/reflect/entity_commands.rs

+        return;
+
+    };


Suggested change

return;

};

return;

};

nicopap · 2023-08-17T20:28:27Z

crates/bevy_ecs/src/reflect/entity_commands.rs

+        return;
+
+    };


Suggested change

return;

};

return;

};

crates/bevy_ecs/src/reflect/entity_commands.rs

nicopap · 2023-08-17T20:31:35Z

Just don't worry about CI, it's broken. Also don't worry about formatting within let else it's also broken, but both are going to get fixed soon(-ish)!

NoahShomette · 2023-08-18T03:11:23Z

Just don't worry about CI, it's broken. Also don't worry about formatting within let else it's also broken, but both are going to get fixed soon(-ish)!

Haha that works for me! Thank you very much for reviewing and helping me out!

NoahShomette · 2023-08-26T03:50:09Z

I realized that the two with_registry commands were removing the registry resource but were not returning it afterwards. Updated both functions to use a resource_scope so that should be fixed now.

alice-i-cecile · 2023-08-28T16:40:27Z

@NoahShomette this needs to be formatted, but once that's done I'll merge it!

NoahShomette · 2023-08-28T16:45:41Z

@NoahShomette this needs to be formatted, but once that's done I'll merge it!

Done!

# Objective To enable non exclusive system usage of reflected components and make reflection more ergonomic to use by making it more in line with standard entity commands. ## Solution - Implements a new `EntityCommands` extension trait for reflection related functions in the reflect module of bevy_ecs. - Implements 4 new commands, `insert_reflect`, `insert_reflect_with_registry`, `remove_reflect`, and `remove_reflect_with_registry`. Both insert commands take a `Box<dyn Reflect>` component while the remove commands take the component type name. - Made `EntityCommands` fields pub(crate) to allow access in the reflect module. (Might be worth making these just public to enable user end custom entity commands in a different pr) - Added basic tests to ensure the commands are actually working. - Documentation of functions. --- ## Changelog Added: - Implements 4 new commands on the new entity commands extension. - `insert_reflect` - `remove_reflect` - `insert_reflect_with_registry` - `remove_reflect_with_registry` The commands operate the same except the with_registry commands take a generic parameter for a resource that implements `AsRef<TypeRegistry>`. Otherwise the default commands use the `AppTypeRegistry` for reflection data. Changed: - Made `EntityCommands` fields pub(crate) to allow access in the reflect module. > Hopefully this time it works. Please don't make me rebase again ☹

MrGVSV added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jun 20, 2023

MrGVSV self-requested a review June 20, 2023 02:03

nicopap self-requested a review June 20, 2023 04:30

MrGVSV reviewed Jun 20, 2023

View reviewed changes

nicopap mentioned this pull request Jun 20, 2023

Move AppTypeRegistry to bevy_ecs #8901

Merged

NoahShomette added 2 commits July 12, 2023 01:56

Implement entity commands reflect extension

5bd3bd0

Implement final changes and insert_reflected_with_registry and remove…

a677efc

…_reflected_with_registry

NoahShomette force-pushed the reflect_entity_commands branch from 1192171 to a677efc Compare July 12, 2023 05:57

NoahShomette added 2 commits July 12, 2023 02:02

Make EntityCommands fields pub(crate)

61a59d0

add entity_commands module and pub use statement for EntityCommandsRe…

760bb6c

…flectExtension

NoahShomette added 4 commits July 12, 2023 02:14

cargo fmt

e1762aa

Documentation fixes

666fd76

cargo fmt entity_commands.rs

dde9e27

remove bom to pass ci

e3d1d6a

nicopap reviewed Jul 27, 2023

View reviewed changes

nicopap mentioned this pull request Jul 31, 2023

ReflectComponent Commands #5541

Closed

Impl feedback

8f91312

nicopap self-requested a review August 17, 2023 06:19

nicopap added this to the 0.12 milestone Aug 17, 2023

nicopap suggested changes Aug 17, 2023

View reviewed changes

impl feedback

0a9cd4b

nicopap approved these changes Aug 17, 2023

View reviewed changes

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 Aug 20, 2023

Fix with_registry commands removing the resource and not returning it

a5355c8

alice-i-cecile enabled auto-merge August 28, 2023 16:40

alice-i-cecile disabled auto-merge August 28, 2023 16:40

Format

c6b2b2a

alice-i-cecile added this pull request to the merge queue Aug 28, 2023

Merged via the queue into bevyengine:main with commit 72fc63e Aug 28, 2023
20 checks passed

NoahShomette deleted the reflect_entity_commands branch August 29, 2023 01:23

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement insert and remove reflected entity commands #8895

implement insert and remove reflected entity commands #8895

NoahShomette commented Jun 20, 2023 •

edited

MrGVSV left a comment

MrGVSV Jun 20, 2023

NoahShomette Jun 20, 2023

NoahShomette commented Jun 20, 2023

nicopap commented Jun 23, 2023

nicopap commented Jul 9, 2023

NoahShomette commented Jul 12, 2023 •

edited

NoahShomette commented Jul 12, 2023 •

edited

nicopap left a comment

nicopap Jul 27, 2023

NoahShomette Aug 10, 2023

nicopap left a comment

NoahShomette commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

nicopap left a comment

nicopap Aug 17, 2023

nicopap Aug 17, 2023

nicopap commented Aug 17, 2023

NoahShomette commented Aug 18, 2023

NoahShomette commented Aug 26, 2023

alice-i-cecile commented Aug 28, 2023

NoahShomette commented Aug 28, 2023

		/// An extension trait for [`EntityCommands`] for reflection related functions
		pub trait EntityCommandsReflectExtension {

-        return;
-    };
+        return;
+    };

implement insert and remove reflected entity commands #8895

implement insert and remove reflected entity commands #8895

Conversation

NoahShomette commented Jun 20, 2023 • edited

Objective

Solution

Changelog

MrGVSV left a comment

Choose a reason for hiding this comment

MrGVSV Jun 20, 2023

Choose a reason for hiding this comment

NoahShomette Jun 20, 2023

Choose a reason for hiding this comment

NoahShomette commented Jun 20, 2023

nicopap commented Jun 23, 2023

nicopap commented Jul 9, 2023

NoahShomette commented Jul 12, 2023 • edited

NoahShomette commented Jul 12, 2023 • edited

nicopap left a comment

Choose a reason for hiding this comment

nicopap Jul 27, 2023

Choose a reason for hiding this comment

NoahShomette Aug 10, 2023

Choose a reason for hiding this comment

nicopap left a comment

Choose a reason for hiding this comment

NoahShomette commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

nicopap left a comment

Choose a reason for hiding this comment

nicopap Aug 17, 2023

Choose a reason for hiding this comment

nicopap Aug 17, 2023

Choose a reason for hiding this comment

nicopap commented Aug 17, 2023

NoahShomette commented Aug 18, 2023

NoahShomette commented Aug 26, 2023

alice-i-cecile commented Aug 28, 2023

NoahShomette commented Aug 28, 2023

NoahShomette commented Jun 20, 2023 •

edited

NoahShomette commented Jul 12, 2023 •

edited

NoahShomette commented Jul 12, 2023 •

edited