Queries as entities #18860
Queries as entities #18860
Conversation
|
Can you add a test that shows that EDIT: For example a system like this, while no other such query exists, should not panic as otherwise it accesses it's state twice, first for using the query and second by finding it via the query. fn system(query: Query<EntityMut<'static>>) {
for mut entity_mut in query {
entity_mut
.get_mut::<QueryState<EntityMut<'static>>>()
.is_none_or(|_aliased_mut| unreachable!());
}
} |
crates/bevy_ecs/src/query/state.rs
Outdated
| let observer = Observer::new( | ||
| move |trigger: Trigger<ArchetypeCreated>, mut world: DeferredWorld| -> Result { | ||
| std::println!("sync archetype for {}", core::any::type_name::<D>()); | ||
|
|
||
| let archetype_id: ArchetypeId = trigger.event().0; | ||
| let archetype_ptr: *const Archetype = world | ||
| .archetypes() | ||
| .get(archetype_id) | ||
| .expect("Invalid ArchetypeId"); | ||
| let Ok(mut entity) = world.get_entity_mut(entity) else { | ||
| // TODO query is not exists anymore, despawn this observer | ||
| return Ok(()); | ||
| }; | ||
| let mut state = entity | ||
| .get_mut::<QueryState<D, F>>() | ||
| .expect("QueryState not found"); | ||
| state.sync_by_observer = true; | ||
| unsafe { | ||
| state.new_archetype_internal(&*archetype_ptr); | ||
| } | ||
|
|
||
| Ok(()) | ||
| }, | ||
| ); | ||
| world.commands().spawn(observer); |
There was a problem hiding this comment.
This should be turned into an entity observer rather than a global observer. Take a look at Observer::watch_entity to see how to do so. Then you don't need that TODO check.
There was a problem hiding this comment.
The problem is when I emmit the event I dont have infomation about the targets. Maybe storing in a resource? Also what is the different between entity observer and a global one?
There was a problem hiding this comment.
This should be turned into an entity observer rather than a global observer.
What we really need here might be type-erased QueryState. Even with global observers, we still have one for each D, F pair, which isn't that much fewer than one per query. (Especially if we manage to de-duplicate query states!)
alice-i-cecile
added
A-ECS
crates/bevy_ecs/src/query/state.rs
Outdated
| let mut state = entity | ||
| .get_mut::<QueryState<D, F>>() | ||
| .expect("QueryState not found"); | ||
| state.sync_by_observer = true; |
There was a problem hiding this comment.
I think you want to set sync_by_observer in the on_add hook so that it's set even if no new archetypes are added.
Alternately, you could set it manually when you spawn the query state entity. You could even spawn the observer there instead of needing a hook!
| let state = world | ||
| .get_entity(entity) | ||
| .unwrap() | ||
| .get::<QueryState<D, F>>() |
There was a problem hiding this comment.
We'll want to benchmark to make sure this extra lookup doesn't degrade performance.
And we probably want to cache the ComponentId for QueryState<D, F> to avoid the hashtable lookup.
There was a problem hiding this comment.
I'm limiting this review to the part of the code that creates archetypes and emits an event. When I tried implementing this a couple of months ago, I got stuck on the type machinery of Query and QueryState internals, so I won't be much help there. I also am not checking safety.
The way in which we're currently passing up that a new archetype has been created so we can emit an event on the world-level is quite brittle. Especially when looking at future plans for archetype removal, I think we'd want a better solution here. I see broadly two solutions:
- Archetype management seems like something that should be handled on a World level of abstraction, because that's the only place you can call observers. Maybe we should unabstract a couple layers and handle more stuff in
impl World { ... }. @ItsDoot has done a lot of abstraction work there recently and I don't know how difficult this would be. - Find a way to trigger events from outside of the World. It would be great if we can pass a
EventTriggertoArchetypesupon creation, but that does make Archetypes less encapsulated.
This is a fairly complicated PR so I don't recommend that you also try to tackle type-erased QueryState for this PR (except if it's way less complicated than I think it is).
Lastly I would like to see a test where you do a bunch of ecs operations and keep count of the number of ArchetypeCreated events and compare that to the number of archetypes in Archetypes. That would also help greatly with brittleness.
Otherwise, I'm really interested in this feature and I hope that we can land this for 0.17.
| archetype: &Archetype, | ||
| access: &mut Access<ArchetypeComponentId>, | ||
| ) { | ||
| if !self.matched_archetypes.contains(archetype.id().index()) { |
There was a problem hiding this comment.
This probably warrants a small comment: 'check if the archetype access has already been updated by an observer and if so don't bother'
There was a problem hiding this comment.
This can be removed if we land #16885, or at least move this logic to system param instead.
|
Thanks for all the feedback. Making this as draft while I'm working on the fix. Also notice the unsoundness raised by @urben1680, query like |
| // Its still possible archetype created after the state is created and before getting sync by | ||
| // the observer. The archetype [Observer, ObserverState] and [QueryState<D, F>] for example. | ||
| // SAFETY: We keep a mutable reference but this method doesn't read that reference. | ||
| state.0.update_archetypes_unsafe_world_cell(world); |
There was a problem hiding this comment.
I found this nasty bug, it's still possible to miss some archetypes that cause query like Query<Entity> miss some data.
There was a problem hiding this comment.
Oh, that is subtle! It would be good to add a unit test that fails without this line.
| let mut component_access = state.component_access.clone(); | ||
| // The Query need access to its own state. Just to be consistent, this will not | ||
| // cause a conflict since QueryStateWrapper is immutable | ||
| component_access.add_component_read(component_id); |
There was a problem hiding this comment.
This now cause fn system(_query: Query<Entity>, _world: DeferredWorld) {} to fail because DeferredWorld expects empty read access. We can remove this or modify DeferredWorld to accept read to immutable components. I prefer the later one
There was a problem hiding this comment.
I think we don't need this now that you're using an immutable component. We need to satisfy the safety requirements for the call to get::<QueryStateWrapper<D, F>>(), and when it was an ordinary mutable component then we could only do that by registering access here. But now that it's immutable, we can satisfy the requirements by noting that there cannot be any mutable access to the component.
(This might also affect fn system(query1: Query<Entity>, query2: Query<EntityMut>), since the EntityMut in the second query would conflict with the QueryStateWrapper access from the first query.)
|
I have updated to use wrapper type and immutable components. I will do a final cleanup and run benchmark after land #16885. Thank everyone for helping.
Immutable component solved this.
I added a debug-only drop check for it. |
|
I ran the benchmarks on my old laptop which I don't trust. But the regression on |
|
Hmm, CI seems stuck or something? Can someone retrigger it? Edit: nvm |
| assert_eq!(tab_navigation.tabindex_query.iter().count(), 2); | ||
| let system = move |tab_navigation: TabNavigation| { | ||
| assert_eq!(tab_navigation.tabgroup_query.iter().count(), 1); | ||
| assert_eq!(tab_navigation.tabindex_query.iter().count(), 2); |
There was a problem hiding this comment.
Ignore my change please. I don't know how to fix this test. The tabindex_query is matching bunch of other entities since queries are entities now.
| @@ -409,23 +433,40 @@ fn assert_component_access_compatibility( | |||
| panic!("error[B0001]: Query<{}, {}> in system {system_name} accesses component(s) {accesses}in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001", ShortName(query_type), ShortName(filter_type)); | |||
| } | |||
|
|
|||
| /// Safety: Caller must ensure componet id is `QueryStateWrapper<D, F>` | |||
| unsafe fn get_query_state<'w, D: QueryData + 'static, F: QueryFilter + 'static>( | |||
There was a problem hiding this comment.
I had an idea for using queries as entities to store a key -> query_entity mapping that will allow me to call various queries while holding exclusive world access.
Is there any chance this could be pub? Or is there some different higher level api that'll be built on this?
There was a problem hiding this comment.
I have an idea to dedup query state, then you can world.query<D, F>() when you have mutable reference to the world. It will take care of all the entities under the hood.
|
Triage: has merge conflicts |
janhohenheim
added
the
S-Waiting-on-Author
Objective
Solution
QueryStateWrappera component and use observer to update it state.ArchetypeCreatedevent. This will be emitted right after the archetype is created and before any entities are moved in.QueryStatefromQuerysystem param is spawned as entity. I choose to do it to minimize the changes. Alot of rendering code is self-manageQueryState. We still spawn duplicatedQueryStatefor now.Testing
Future work
QueryStateprivate so we can easily dedup? How do we gonna dedup?Querynew_archetype? It's currently doing nothing.Benchmark
Here is benchmark run with
cargo bench -p benches --bench ecs -- "iter|spawn|system"Click me