Entity scope v1: component scope

[janis-bhm]

Objective

related: #13128 With Resources-as-Entities, and possibly more *-as-Entities on the horizon, simultaneous mutable access to an entity and the world has and will come up more often: resource_scope, for example, needs mutable access to the resource component on the resource entity, and doesn't want the entity to be available for it's scope.

Solution

I propose the following solution: Entities can be 'hard' disabled by swapping them into a region at the beginning of their archetype/table and their meta is updated to contain no location. These regions are skipped by queries and not dropped when the table is removed. A component_scope ptr::reads a component onto the stack, then disables the entity and calls the closure with a mutable reference to the stack copy of the component. After the closure finishes, the stack component is ptr::writen back into storage and the entity is enabled: during the scope the component in storage must not be dropped because the scope closure may decide to drop it itself.

A component_scope limited to a single component is the simplest and easiest functionality to build here, because we cannot ensure that the table doesn't move the disabled components around or that the table even remains live for the duration of the scope, so we can't give out references to the ECS storage, and can't use WorldQuery and related traits to easily retrieve multiple components: this will require some more trait magic ontop of WorldQuery, but I think it shouldn't be excessively complicated or unsound.

Remaining Questions

• Relationships: the relationship hooks unwrap the target entity, which isn't available when disabled, causing a panic. The component_scope closure could be passed Commands of some shape onto which to record operations related to the disabled entity.
• Is this actually worth it/cheaper than an archetype move? This still moves components around in the tables, which is most of what an archetype move is. I haven't benchmarked this (and I would need some help properly benchmarking it). Nested component_scopes should not require a swap when re-enabling the entity, but different functionality build with disabling/enabling might.
• Things I've forgotten or overlooked.

Testing

there are still lots of TODOs in this PR:

• Tests
• Safety comments
• some polishing, probably
• actually implementing the logic that forgets the disabled components

Alternatives

• Removing components (and possibly despawning the entity), re+inserting/spawning them after the scope.
• I'm spit-balling here but: Tables might be made up of chunks in which rows are stable across table growth; an entity could be moved to a dedicated chunk, which can be owned by the entity_scope to be moved back into the table afterwards. This would probably have a significant performance impact, not to mention the architectural impact.
• Instead of moving components around in tables, keep a bitset indicating enabled-ness

---

Showcase

use bevy_ecs::prelude::*;
#[derive(Component)]
struct A(u32);
#[derive(Component)]
struct B(u32);
let mut world = World::new();
let scoped_entity = world.spawn(A(1)).id();
let entity = world.spawn(B(1)).id();

world.component_scope(scoped_entity, |world, _, a: &mut A| {
    assert!(world.get_mut::<A>(scoped_entity).is_none());
    let b = world.get_mut::<B>(entity).unwrap();
    a.0 += b.0;
});
assert_eq!(world.get::<A>(scoped_entity).unwrap().0, 2);

[github-actions[bot]]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[alice-i-cecile]

Marked as X-Controversial due to the architectural implications, but I quite like the core strategy and really want this functionality.

[ElliottjPierce]

I have not looked very closely at the implementation yet, but here's a few thoughts.

This is probably the best way to implement component scopes. In my own ecs engine, I've implemented something similar to the chunked tables, and it was very much not worth it. Removing, scoping, and re-adding a component is probably also too slow, would create useless or maybe even invalid (with archetype invariants) archetypes, and maybe cause hook issues. Either hooks will run, making the previous sate unrecoverable, or the scope will have an invalid world state–this is general form of the relationship problem you mentioned.

Even in terms of the objective, I'm not convinced this is the right way to go. This is definitely not my decision, but what I've noticed is that these scope functions get really messy and are IMO not good practice. I didn't like resource scopes because they were limiting and promoted lots of nesting. I almost always just did a run_system_once and Commands with a closure or something. With components too, I feel like this could lead to a lot of bugs in user code. Now, every helper function a user makes needs to know if it's running as part of the scope or not, and if so, it may need a special way to handle the scoped entity. (Ex: The scope uses a helper function to sum up all the health components, but the scoped entity has a health component and is missed. Now that function needs to know if its running in a scope or not.) Maybe that's rare; I really don't know, but I don't like how much danger this allows users. I'd much prefer making an easier way to scope safe access to QueryParams.

At a conceptual level, I also think this might be unsound. As an example, what if you scope access to a component value and then clear the world? Now the old copy has been dropped, but you still have a mutable reference to a copy of a now dropped component! That's a big deal if the component has a Vec or something; now its a double free. And you can't just mem::forget the new one since maybe the vec was changed and the allocation moved. To fix this, you'd need some way of knowing just from &mut World if a given component is scoped, and I think tracking and checking that information everywhere is probably more trouble than it's worth. Unless I'm missing some way around this. (Maybe do the add and remove idea, but that has it's own problems. Or maybe add a new hook mode to fix those? Idk.)

I know I said this is probably the best way to do component scopes, and I stand by that (unless you come up with something better 👀 ). All other ways of doing this that I can think of come with massive performance issues and correctness concerns. I think this is the most useful way of doing it; it just needs to be unsafe. After all, you are giving mutable access to a "field" of the world while sill providing &mut World. That is inherently unsafe, even if you add a ton of guardrails. That said, I think we should look into alternatives to the scope pattern here. I think improving the run_system_once + Commands is probably a better way to handle these situations IMO. And in the rare case that the "remove, scope, and re-add" workflow is needed, I'd almost rather it be explicit, so users can see all the risks and side-effects openly. But that's just my opinion, and (for good reason) this is not my decision to make.

I'll try to find time to review the code later if possible. This is a really good idea to explore, but it is very complicated.

[janis-bhm]

What are the advantages of this proposal over the take + insert approach?

My goal was to touch the entity and related storage as little as possible, hopefully reducing the cost of these scopes. If the to-be-scoped entity is the only entity in it's archetype that ever gets scoped (this would be the case for resources, but probably not systems), there are no moves except for the components removed. for nested scopes there are no moves when re-enabling. Additionally, nothing can "mess" with the entity with its component(s) logically removed. I think the question here is if this is worth the complexity or if there is a better way to avoid moves completely. For completely avoiding moves it would be best to have something like disjoint ranges over tables but that sounds like it would be awful for iterating performance.

Also I think it'll be possible to re-use/re-purpose QueryData/WorldQuery to query multiple components in a follow up PR.

The other big difference from this PR is that it runs lifecycle hooks and observers, but I think we need to run them.

I was hoping that this wasn't the case :( I really like the idea of preserving everything about the entity and its relationships and components during scoping, but I can see how it would be bad to break the assumption that an entity-component pairing exists and is query-able if no on_remove hooks/observers were triggered. I think this means there needs to be an (unsafe) way to access the entity after the hooks and observers have run when disabling to copy-out the component the scope is interested in and the other way around for enabling.

[janis-bhm]

another idea might be to just keep a bitset near/in the table/archetype to keep track of enabled-ness but I'll have to think more about whether that could work, and it would involve a check in the hot loop of query iteration (though there already is one). Not sure why I didn't consider this earlier but I definitely love overcomplicating things.

[chescock]

My goal was to touch the entity and related storage as little as possible, hopefully reducing the cost of these scopes. If the to-be-scoped entity is the only entity in it's archetype that ever gets scoped (this would be the case for resources, but probably not systems), there are no moves except for the components removed. for nested scopes there are no moves when re-enabling.

My instinct here is to start with the simplest thing, and see whether the performance is good enough. Archetype moves aren't free, but they aren't all that expensive when there is only one entity being moved. And users already have some tools to mitigate the costs: If all of the components being moved use SparseSet storage then there won't be any table moves. And some of the more complex cases may be better solved using SystemState or run_system_cached to split the world access without needing to move anything.

But my experience is not from gamedev, so I may be misunderstanding the tradeoffs here! Do we have real use cases where the performance of a simple take() solution is inadequate?

(Oh, wait, maybe I should actually read the whole linked issue in case there's already an answer... Well the issue says "Performance is always nice, but is a secondary concern for this style of API" :). But the comments do express concern about the performance of archetype moves.)

[janis-bhm]

I think component_scope similairly to resource_scope will be very usefull. Especially if we can get it to work with bundles. But I don't think we should implement it like this. I think archetype moves are fine in this case, we can always worry about optimization later.

I think implementing component_scope in terms of removing and reinserting components also breaks invariants, though, for example when removing some component's C required component U, or, in the case of resource_scope by keeping the entity alive. So realistically, all the components have to go/the entity has to be despawned during component_scope. So an implementation based on removing and inserting would have to temporarily dump all other components into boxes via pre_remove and then reinsert via dynamic bundles. the benefit of this would be that lifecycle related stuff is done by the inserting/spawning.

[Trashtalk217]

Yes, I agree that removing and inserting might break some invariants.

(Although I will say that required components specifically don't guarentee that if A requires B, then A will always exist with B. You can individually remove B from an entity with A and B. The only guarentee given is that when inserting A, B is also inserted, see https://docs.rs/bevy/latest/bevy/prelude/trait.Component.html#required-components)

Still these invariants are often assumed, and it's good to not mess with them. I can imagine componenthooks being a real pain especially, since they are meant to maintain invariants. Should component_scope run them? I don't know.

Another option could be to allow component_scope to panic/error in some way. There might be cases where it's better to abort instead of doing something highly unusual / likely unintended by the user. How tolerant the function is might even be configurable. (I'm thinking of RelationshipHookMode when inserting.)

[chescock]

I think implementing component_scope in terms of removing and reinserting components also breaks invariants, though, for example when removing some component's C required component U, or, in the case of resource_scope by keeping the entity alive.

Yeah, as @Trashtalk217 says, we don't guarantee that about required components. More generally, it's possible to do the take/insert approach in user code today using only pub methods, so nothing should be assuming that that won't happen.

If a component really needs to ensure that an entity with a C always has a U, the way to do that is with hooks... which is one of the reasons that I think we need to make sure to run hooks if we make U unavailable in a component_scope.

[jamescarterbell]

This seems like a really good idea and a pretty clean approach. For Tilemap related things, I'm constantly having to remove the map entity from the world so that I can update it while I'm also updating/creating Chunk entities, and it would be nice to avoid that.

[Diddykonga]

Haven't read through the whole discussion, but what about adding access to the World instead of Schedules, and then expending the access set with entities as well, were you would generally only check the smoller of the two, and then Schedules/Systems would register their access through the World, which would be the same for a user in an Exclusive System trying to yoink a Resource and then potentially run a Query. (Haven't thought of the downsides, besides for it being heavier then scoping)