Disallow requesting write resource access in Queries#17116
Disallow requesting write resource access in Queries#17116alice-i-cecile merged 6 commits intobevyengine:mainfrom
Conversation
| fn shrink_fetch<'wlong: 'wshort, 'wshort>(fetch: Self::Fetch<'wlong>) -> Self::Fetch<'wshort>; | ||
|
|
||
| /// Creates a new instance of this fetch. | ||
| /// Readonly accesses resourses registered in [`update_component_access`]. |
There was a problem hiding this comment.
| /// Readonly accesses resourses registered in [`update_component_access`]. | |
| /// Readonly accesses resources registered in [`update_component_access`]. |
chescock
left a comment
There was a problem hiding this comment.
Yeah, I think this is the right move. I only added the code for mutable resources because it looked simple and not because we had a real use case, and it's clear that I was wrong about it being simple.
We should also remove the has_resource_write() check in AssetChanged. It's the only first-party use of resource access in queries, so users might use it as an example, and leaving that check there might trick them into thinking that mutable resource access is supported.
| } | ||
| } | ||
|
|
||
| if state.component_access.access().has_write_all_resources() { |
There was a problem hiding this comment.
It might be worth adding a check like
debug_assert!(!state.component_access.access().has_any_resource_write());so that anyone who tries it gets an error.
It should not be removed, because user could request write access to the resource in the |
I believe that's checked in |
It's not checked there fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
let state = QueryState::new_with_access(world, &mut system_meta.archetype_component_access);
init_query_param(world, system_meta, &state);
state
} |
|
The check is in |
|
Note that write access to resources during queries may be useful for certain index updating strategies. That said, it doesn't seem to actually work, so we should clean this up for now and re-add it later when we have clearer requirements. |
Related to bevyengine#16843 Since `WorldQuery::Fetch` is `Clone`, it can't store mutable references to resources, so it doesn't make sense to mutably access resources. In that sense, it is hard to find usecases of mutably accessing resources and to clearly define, what mutably accessing resources would mean, so it's been decided to disallow write resource access. Also changed documentation of safety requirements of `WorldQuery::init_fetch` and `WorldQuery::fetch` to clearly state to the caller, what safety invariants they need to uphold.
Related to #16843
Since
WorldQuery::FetchisClone, it can't store mutable references to resources, so it doesn't make sense to mutably access resources. In that sense, it is hard to find usecases of mutably accessing resources and to clearly define, what mutably accessing resources would mean, so it's been decided to disallow write resource access.Also changed documentation of safety requirements of
WorldQuery::init_fetchandWorldQuery::fetchto clearly state to the caller, what safety invariants they need to uphold.