-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Component Lifecycle Hooks and a Deferred World #10756
Component Lifecycle Hooks and a Deferred World #10756
Conversation
The generated |
1 similar comment
The generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a fantastic change that would make a lot of change-detection things much better. I had a play with this and made a couple of changes if you'd be interested in incorporating them. I still think this would be amazing as-is, but if you could include those couple of extras, it'd be really helpful.
@@ -201,11 +203,23 @@ pub enum StorageType { | |||
SparseSet, | |||
} | |||
|
|||
/// The type used for [`Component`] lifecycle hooks such as `on_add`, `on_insert` or `on_remove` | |||
pub type ComponentHook = for<'w> fn(DeferredWorld<'w>, Entity, ComponentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was changed to &mut DeferredWorld
you avoid needing to add mut
in the hook closure definition, and can avoid repeated instantiation of DeferredWorld
when triggering the hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The presence of mut world
was not something I had considered but there is no cost in constructing DeferredWorld
as it is just a wrapped world pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I doubt it has any performance implications, moreso just the ergonomics. Conceptually as well, it matches how things like resource_scope
work (providing a world reference). Obviously DeferredWorld
is a reference, but it would help with first-glance clarity IMO.
+1 For Edit: Oof, I should have looked at the code first. |
It's called |
About finding a better name for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extremely useful, well-engineered. There will be some interesting challenges around how to teach this feature (when should you use this versus other methods? What are the perf characteristics?) but I think this should exist (both for relations and other use cases), and I think this is at a level of code quality that's acceptable to merge.
I'm quite curious about how this might interact with #1481's design space in the future, but that's a separate conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing &mut ComponentInfo
is my biggest concern here, as the World's metadata is safety critical, and any possibility of invalidating those metadata stores is an opportunity for unsoundness. I don't like merging this as is, but solving this is a problem bigger than the scope of this PR.
# Objective Following #10756, we're now using raw pointers in BundleInserter and BundleSpawner. This is primarily to get around the need to split the borrow on the World, but it leaves a lot to be desired in terms of safety guarantees. There's no type level guarantee the code can't dereference a null pointer, and it's restoring them to borrows fairly liberally within the associated functions. ## Solution * Replace the pointers with `NonNull` and a new `bevy_ptr::ConstNonNull` that only allows conversion back to read-only borrows * Remove the closure to avoid potentially aliasing through the closure by restructuring the match expression. * Move all conversions back into borrows as far up as possible to ensure that the borrow checker is at least locally followed.
# Objective - Provide a reliable and performant mechanism to allows users to keep components synchronized with external sources: closing/opening sockets, updating indexes, debugging etc. - Implement a generic mechanism to provide mutable access to the world without allowing structural changes; this will not only be used here but is a foundational piece for observers, which are key for a performant implementation of relations. ## Solution - Implement a new type `DeferredWorld` (naming is not important, `StaticWorld` is also suitable) that wraps a world pointer and prevents user code from making any structural changes to the ECS; spawning entities, creating components, initializing resources etc. - Add component lifecycle hooks `on_add`, `on_insert` and `on_remove` that can be assigned callbacks in user code. --- ## Changelog - Add new `DeferredWorld` type. - Add new world methods: `register_component::<T>` and `register_component_with_descriptor`. These differ from `init_component` in that they provide mutable access to the created `ComponentInfo` but will panic if the component is already in any archetypes. These restrictions serve two purposes: 1. Prevent users from defining hooks for components that may already have associated hooks provided in another plugin. (a use case better served by observers) 2. Ensure that when an `Archetype` is created it gets the appropriate flags to early-out when triggering hooks. - Add methods to `ComponentInfo`: `on_add`, `on_insert` and `on_remove` to be used to register hooks of the form `fn(DeferredWorld, Entity, ComponentId)` - Modify `BundleInserter`, `BundleSpawner` and `EntityWorldMut` to trigger component hooks when appropriate. - Add bit flags to `Archetype` indicating whether or not any contained components have each type of hook, this can be expanded for other flags as needed. - Add `component_hooks` example to illustrate usage. Try it out! It's fun to mash keys. ## Safety The changes to component insertion, removal and deletion involve a large amount of unsafe code and it's fair for that to raise some concern. I have attempted to document it as clearly as possible and have confirmed that all the hooks examples are accepted by `cargo miri` as not causing any undefined behavior. The largest issue is in ensuring there are no outstanding references when passing a `DeferredWorld` to the hooks which requires some use of raw pointers (as was already happening to some degree in those places) and I have taken some time to ensure that is the case but feel free to let me know if I've missed anything. ## Performance These changes come with a small but measurable performance cost of between 1-5% on `add_remove` benchmarks and between 1-3% on `insert` benchmarks. One consideration to be made is the existence of the current `RemovedComponents` which is on average more costly than the addition of `on_remove` hooks due to the early-out, however hooks doesn't completely remove the need for `RemovedComponents` as there is a chance you want to respond to the removal of a component that already has an `on_remove` hook defined in another plugin, so I have not removed it here. I do intend to deprecate it with the introduction of observers in a follow up PR. ## Discussion Questions - Currently `DeferredWorld` implements `Deref` to `&World` which makes sense conceptually, however it does cause some issues with rust-analyzer providing autocomplete for `&mut World` references which is annoying. There are alternative implementations that may address this but involve more code churn so I have attempted them here. The other alternative is to not implement `Deref` at all but that leads to a large amount of API duplication. - `DeferredWorld`, `StaticWorld`, something else? - In adding support for hooks to `EntityWorldMut` I encountered some unfortunate difficulties with my desired API. If commands are flushed after each call i.e. `world.spawn() // flush commands .insert(A) // flush commands` the entity may be despawned while `EntityWorldMut` still exists which is invalid. An alternative was then to add `self.world.flush_commands()` to the drop implementation for `EntityWorldMut` but that runs into other problems for implementing functions like `into_unsafe_entity_cell`. For now I have implemented a `.flush()` which will flush the commands and consume `EntityWorldMut` or users can manually run `world.flush_commands()` after using `EntityWorldMut`. - In order to allowing querying on a deferred world we need implementations of `WorldQuery` to not break our guarantees of no structural changes through their `UnsafeWorldCell`. All our implementations do this, but there isn't currently any safety documentation specifying what is or isn't allowed for an implementation, just for the caller, (they also shouldn't be aliasing components they didn't specify access for etc.) is that something we should start doing? (see 10752) Please check out the example `component_hooks` or the tests in `bundle.rs` for usage examples. I will continue to expand this description as I go. See bevyengine#10839 for a more ergonomic API built on top of this one that isn't subject to the same restrictions and supports `SystemParam` dependency injection.
# Objective Following bevyengine#10756, we're now using raw pointers in BundleInserter and BundleSpawner. This is primarily to get around the need to split the borrow on the World, but it leaves a lot to be desired in terms of safety guarantees. There's no type level guarantee the code can't dereference a null pointer, and it's restoring them to borrows fairly liberally within the associated functions. ## Solution * Replace the pointers with `NonNull` and a new `bevy_ptr::ConstNonNull` that only allows conversion back to read-only borrows * Remove the closure to avoid potentially aliasing through the closure by restructuring the match expression. * Move all conversions back into borrows as far up as possible to ensure that the borrow checker is at least locally followed.
# Objective Oftentimes I find myself reading through a PR and not quite understanding what's going on. Even if it's super detailed, it can sometimes be difficult to imagine what the end result of the PR might look like. For example, #10756 clearly communicates its goals and contains a descriptive Changelog. However, I was still a bit lost as to what a user might see from the change until I saw the dedicated example in the diff. ## Solution At the risk of giving contributors more work, I think a dedicated `Showcase` section could be really nice. Along with providing reviewers stumbling on the PR with a "tangible summary" of the change, it should also help out when working on the release post. Sometimes someone other than the PR's author has to write up a blog section on the PR. This can be somewhat daunting to people wanting to contribute in that effort as they have to rely on the Migration Guide giving a decent example (assuming it's a breaking change), piecing together the other sections into a sensible example themselves, or manually reading through the diff. Theoretically, this new `Showcase` section would be more of an encouragement than a strict requirement. And it's probably only going to be useful where there is something to showcase (e.g. visual changes, API changes, new features, etc.). ### Bikeshedding - **Naming.** I also considered `Demo` and `Example`, but there may be others we prefer. I chose `Showcase` to communicate the feeling of fun and appreciation for the work contributors put in. - **Position.** I placed the section right above the `Changelog` section since I felt it made sense to move from the details in `Solution` to a brief example in `Showcase` to a tl;dr of the changes in `Changelog` - **Phrasing.** We can also bikeshed the bullet points and phrasing of each as well.
Objective
Solution
DeferredWorld
(naming is not important,StaticWorld
is also suitable) that wraps a world pointer and prevents user code from making any structural changes to the ECS; spawning entities, creating components, initializing resources etc.on_add
,on_insert
andon_remove
that can be assigned callbacks in user code.Changelog
DeferredWorld
type.register_component::<T>
andregister_component_with_descriptor
. These differ frominit_component
in that they provide mutable access to the createdComponentInfo
but will panic if the component is already in any archetypes. These restrictions serve two purposes:Archetype
is created it gets the appropriate flags to early-out when triggering hooks.ComponentInfo
:on_add
,on_insert
andon_remove
to be used to register hooks of the formfn(DeferredWorld, Entity, ComponentId)
BundleInserter
,BundleSpawner
andEntityWorldMut
to trigger component hooks when appropriate.Archetype
indicating whether or not any contained components have each type of hook, this can be expanded for other flags as needed.component_hooks
example to illustrate usage. Try it out! It's fun to mash keys.Safety
The changes to component insertion, removal and deletion involve a large amount of unsafe code and it's fair for that to raise some concern. I have attempted to document it as clearly as possible and have confirmed that all the hooks examples are accepted by
cargo miri
as not causing any undefined behavior. The largest issue is in ensuring there are no outstanding references when passing aDeferredWorld
to the hooks which requires some use of raw pointers (as was already happening to some degree in those places) and I have taken some time to ensure that is the case but feel free to let me know if I've missed anything.Performance
These changes come with a small but measurable performance cost of between 1-5% on
add_remove
benchmarks and between 1-3% oninsert
benchmarks. One consideration to be made is the existence of the currentRemovedComponents
which is on average more costly than the addition ofon_remove
hooks due to the early-out, however hooks doesn't completely remove the need forRemovedComponents
as there is a chance you want to respond to the removal of a component that already has anon_remove
hook defined in another plugin, so I have not removed it here. I do intend to deprecate it with the introduction of observers in a follow up PR.Discussion Questions
DeferredWorld
implementsDeref
to&World
which makes sense conceptually, however it does cause some issues with rust-analyzer providing autocomplete for&mut World
references which is annoying. There are alternative implementations that may address this but involve more code churn so I have attempted them here. The other alternative is to not implementDeref
at all but that leads to a large amount of API duplication.DeferredWorld
,StaticWorld
, something else?EntityWorldMut
I encountered some unfortunate difficulties with my desired API. If commands are flushed after each call i.e.world.spawn() // flush commands .insert(A) // flush commands
the entity may be despawned whileEntityWorldMut
still exists which is invalid. An alternative was then to addself.world.flush_commands()
to the drop implementation forEntityWorldMut
but that runs into other problems for implementing functions likeinto_unsafe_entity_cell
. For now I have implemented a.flush()
which will flush the commands and consumeEntityWorldMut
or users can manually runworld.flush_commands()
after usingEntityWorldMut
.WorldQuery
to not break our guarantees of no structural changes through theirUnsafeWorldCell
. All our implementations do this, but there isn't currently any safety documentation specifying what is or isn't allowed for an implementation, just for the caller, (they also shouldn't be aliasing components they didn't specify access for etc.) is that something we should start doing? (see 10752)Please check out the example
component_hooks
or the tests inbundle.rs
for usage examples. I will continue to expand this description as I go.See #10839 for a more ergonomic API built on top of this one that isn't subject to the same restrictions and supports
SystemParam
dependency injection.