Remove resources from Access#22910
Conversation
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
…-resources-from-access
|
Alright, since #23138, this PR is unblocked. Which leaves me with one question for reviewers. Is the following code acceptable? // 0.18
fn system(q1_: Query<EntityMut>, q2_: NonSend<R>) {} // works
// 0.19
fn system(q1_: Query<EntityMut, Without<R>>, q2_: NonSend<R>) {} // works againOne possible other solution is to add a fake // 0.18
fn system(q1_: Query<EntityMut>, q2_: NonSend<R>) {} // works
// 0.19
fn system(q1_: Query<EntityMut, Without<IsNonSend>>, q2_: NonSend<R>) {} // works againThis scales slightly better over multiple non-send data, but it's so specific that I (personally) don't think it's worth bothering with. |
alice-i-cecile
left a comment
There was a problem hiding this comment.
Good renames, great use of deprecations, solid and clear migration guide, and a much welcome cleanup.
Only blocking feedback is "we should add a test to verify that Query<EntityMut, Without<IsResource> + Res<Foo> is a valid system". I've also left a couple of small suggestions for improvements, but those are non-blocking.
I agree: I think this is so niche that it's not worth the maintenance or compile time cost. EntityMut + multiple NonSend accesses in a single component is... very unusual. |
chescock
left a comment
There was a problem hiding this comment.
This looks good! And even with the migration guide and #[deprecated] methods being added, it's still removing a few hundred lines of code!
I think we should fix the typos in the deprecation notes before merging, but the rest of my comments are just style nits.
crates/bevy_ecs/src/query/access.rs
Outdated
| } | ||
|
|
||
| /// Adds a read access to a component to the set. | ||
| pub fn add_unfiltered_component_read(&mut self, index: ComponentId) { |
There was a problem hiding this comment.
Do we actually need this method? It was a common pattern for resources, but I wouldn't expect it to be all that useful for other components.
... Oh, I see, it's used by NonSend and in some tests. I don't want to ask you to refactor those, but I might be inclined to leave this non-pub so that users don't think it's common and so that we can remove it in the future as a non-breaking change. It's not all that many lines of code if they really do need this behavior.
| if !accesses.is_empty() { | ||
| accesses.push(' '); | ||
| } | ||
| panic!("error[B0002]: Res<{}> in system {} conflicts with a previous system parameter. Consider using `Without<IsResource>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevy.org/learn/errors/b0002", DebugName::type_name::<T>(), system_meta.name); |
There was a problem hiding this comment.
Should we keep the "Consider removing the duplicate access" part? We used to raise this error mostly for things like fn system(_: ResMut<R>, _: Res<R>), and that case may still be common even if we now also raise it for fn system(_: Query<EntityMut>, _: Res<R>).
Do we need to update the text at https://bevy.org/learn/errors/b0002 at all?
There was a problem hiding this comment.
Probably, but that's a seperate PR.
| let mut filter = FilteredAccess::default(); | ||
| filter.add_read(state.resource_id); | ||
| filter.and_with(IS_RESOURCE); | ||
| component_access_set.add(filter); |
There was a problem hiding this comment.
There are a few places with this block of code that you can replace with FilteredAccessSet::add_resource_read now that we have it
| let mut filter = FilteredAccess::default(); | |
| filter.add_read(state.resource_id); | |
| filter.and_with(IS_RESOURCE); | |
| component_access_set.add(filter); | |
| component_access_set.add_resource_read(state.resource_id); |
The others are in Res, ResMut, and AssetChanged, but GitHub won't let me make suggestions there because the diff crosses added and removed lines.
There was a problem hiding this comment.
The others can't quite be replaced because of the way we immediately check for conflicts, but this one can be subbed out.
It looks like we have an existing test that verifies that systems with those parameters don't conflict. That should be roughly equivalent, although it's a little more complex to read: bevy/crates/bevy_ecs/src/schedule/mod.rs Line 1006 in 2494a37 |
Objective
There's a lot of code duplication in
access.rs. The same logic is duplicated between components and resources. This also takes up unnecessary memory inAccess, as it relies on bitsets spanning the entireComponentIdrange.Solution
Since resources are now a special kind of component, this can be removed.
Limitations
Since
!Senddata queries usedAccessresources,!Senddata queries now conflict with broad queries.Given how rarely non-send data is used, I recommend using
If this is also unacceptable, this PR is blocked on the
!Senddata removal from the ECS (or some hacky workaround).Extra Attention
@chescock brought
AssetChangedto my attention. It has a weird access pattern. See the following example:System
cregisters access withadd_writeforAssetChanges<Mesh>, whilerregisters access withadd_readfor bothMeshandAssetChanges<Mesh>. This system is invalid, and I've added a test to reflect that. However, since this stuff is tricky, I would like some extra eyes on it. Currently, it looks fine.