QueryState::new_with_state

[Suficio]

Objective

Allows providing state to QueryState which requires input other than WorldQuery::init_state(&mut World) to be constructed.

This allows the user to provide a custom state such as ComponentId for dynamic queries which cannot be derived from the type definition.

Solution

Create new method QueryState::new_with_state, which is now called by QueryState::new using WorldQuery::init_state.

[alice-i-cecile]

The example is great, but we probably also want a third query_state initialization showing how to init filter state.

[alice-i-cecile]

Awesome work; thanks for the prompt response. This is a niche feature, but I think it will be useful as part of our "dynamic components" story.

[Suficio]

Awesome work; thanks for the prompt response. This is a niche feature, but I think it will be useful as part of our "dynamic components" story.

No problem, your prompt response is much more appreciated :)

With this feature it is already possible to make a dynamic query by implementing WorldQuery on some Ptr type.

[zicklag]

I'm not qualified enough to review the Bevy query internals yet/anymore, but I seem to remember needing this in my now very old scripting API/PR, so.., 👍 😁

[alice-i-cecile]

Yep, I mostly wanted to hear if you thought it was useful :) The implementation is trivial, and the docs are great.

[BoxyUwU]

I expect this is incredibly unsound as-is since it would let you construct a QueryState<&T> where the state has a componentid for U, which would then get bevy to turn a pointer to U into a &T.

Having a way to do the goal of this PR does seem useful for dynamic queries and I think relations could make use of this too.

[Suficio]

Nice catch @BoxyUwU!

While there is nothing unsafe that this function does, indeed we can incorrectly cast types as a consequence. There are two ways I see that this can be resolved.

1. Validate ComponentId for &T queries during <&T as WorldQuery>::init_fetch
2. Make QueryState::new_with_state unsafe

[alice-i-cecile]

The former is definitely safer. This would be a one time cost paid during system / query initialization, correct?

[BoxyUwU]

no, Fetch is created per archetype/table (which one depends on whether it's a dense or sparse iteration)

[Suficio]

Made the method unsafe and added the appropriate safety comment

[jakobhellermann]

Made the method unsafe and added the appropriate safety comment

Currently the # Safety comment only shows a bad example and suggest using safer methods instead, usually these comments also contain an exhautive list of things you need to validate so that the call is guaranteed safe. Something like

bevy/crates/bevy_ecs/src/storage/blob_vec.rs

Lines 141 to 144 in d38a8df

	/// # Safety
	/// - index must be in bounds
	/// - the memory in the [`BlobVec`] starting at index `index`, of a size matching this [`BlobVec`]'s
	/// `item_layout`, must have been previously allocated.

---

As I understand it, the point of the new_with_state method is to be able to be used if you only know a few ComponentIds at runtime and want to get Ptr<'_>s to them, instead of specifying T to get &Ts, right? And the reason you can't just

struct ComponentIdQuery(ComponentId);
struct ComponentIdQueryMut(ComponentId);

impl WorldQuery for ComponentIdQuery {
  type Item = Ptr<'_>;
  ...
}
impl WorldQuery for ComponentIdQueryMut {
  type Item = (PtrMut<'_>, &mut Ticks);
  ...
}

is that currently init_state is called just with the &mut World without access to any runtime query info.
What if instead we add a type Config to WorldQuery?

trait WorldQuery: WorldQueryGats {
  type State;
+ type Config;
  
-    fn init_state(world: &mut World) -> Self::State;
+    fn init_state(world: &mut World, config: Self::Config) -> Self::State;
}

where the current implementors set Config = () and QueryState::new only works with T: WorldQuery<Config = ()> and we add a fn WorldQuery::new_with_config?

Then you can add impls of WorldQuery for ComponentIdQuery with Item = Ptr<'_> Config = ComponentId and for Vec<T: WorldQuery> with Item = Vec<T::Item> Config = Vec<T::Config>, and you can safely call

let query = QueryState<Vec<ComponentIdQuery>>::new_with_config(vec![my_component_id_1, my_component_id_2]);

[Suficio]

Currently the # Safety comment only shows a bad example and suggest using safer methods instead, usually these comments also contain an exhautive list of things you need to validate so that the call is guaranteed safe.

It's sort of difficult because this method is only unsafe when querying component references, but I reordered the safety comment to more strongly word the requirements for safety.

As I understand it, the point of the new_with_state method is to be able to be used if you only know a few ComponentIds at runtime and want to get Ptr<'_>s to them, instead of specifying T to get &Ts, right?

This is exactly right! However, this isn't the only use, one could likely implement world query for Vec<T>.

Your solution is definitely an improvement, however, I would like to prioritize minimal changes with this PR.

Dynamic query work is still up for discussion on how it should be implemented, and I would like to avoid establishing any future design directions, and instead, work with the API surface as it is available today.

[BoxyUwU]

It's sort of difficult because this method is only unsafe when querying component references

I disagree with this, worldquery impls should be able to rely on the state being created from their own init_state fn and nothing else. More than just "unsafe" it should be unsound to call new_with_state with a value that isnt from init_state unless the worldquery explicitly relaxes this in its documentation. (if this isnt true the soundness of worldquery code gets a lot trickier to reason about, or we start wanting to newtype all our state to prevent public construction neither of which seems desirable to me)

If the above is true then this API is also kind of Not Great, I would much prefer the solution @jakobhellermann proposes with a type Config; since we could probably make that API safe

[Suficio]

The problem I see is that WorldQuery tuple implementations will require all elements to have Config = (), due to system params requiring Config = () (as systems are not able to provide the config), making queries with arbitrary Config unusable for the majority of use patterns.

I've tried the implementation suggested and it results in a lot of extra type constraints that need to be managed across the codebase, and due to the above, severely restricts the reusability of existing implementations.

More than just "unsafe" it should be unsound to call new_with_state with a value that isnt from init_state unless the worldquery explicitly relaxes this in its documentation.

Due to this restriction on Config = (), I've slightly relaxed the safety comment.

While I agree that queries should be able to rely on their init_state, it is not possible to initialize state using only &World, and the suggested Config approach is not able to facilitate the use cases I need.

[jakobhellermann]

Yeah, the Config = () issue for tuples is one I stumbled upon as well when protoyping my suggestion. So instead I required Config: Default, which needs to propagated to a few places. I then ran into an issue changing the tuple impl and didn't finish the prototype, but when I do, I'll open it as a draft PR to have a discussion on whether the few Config: Default bounds are too annoying.

[Suficio]

I think one could argue that having a default here is equally dangerous as providing the state outright, due to the potential implications of accidentally using a ComponentId default for Ptrs.

It results in a situation where you don't know whether init_state config has correct data or whether it needs to be inferred otherwise. I guess we could use an Option but this is still added type complexity for a very niche benefit that this PR brings.

The added benefit of this PR would be that you can avoid the added complication of manipulating type bounds.

[jakobhellermann]

I posted a PR of my proposal: #6390

[Suficio]

Resolved by #9774