Regression in query filter

[Ixentus]

Using latest main 20673db

This worked in 0.5:
Query<&mut MyComponent, Changed<MyComponent>>

In latest main it gives this error:
thread 'main' panicked at '$state_name<game::MyComponent> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.', /home/user/Code/bevy/crates/bevy_ecs/src/query/filter.rs:635:1

This is probably caused by #1929.

If anyone runs into this, you can remove Changed (or Added), and manually filter afterwards using my_component.is_changed()

[NiklasEi]

This is coming from #1929 and as far as I understand, it's not a bug.

[BoxyUwU]

We could technically remove the has_write check in the Changed<T> filter and it would be sound still but that's a bit of a hacky solution to make this work and could easily be made accidentally unsound if we refactor stuff later on. This kind of Query<&mut T, Changed<T>> or Query<&mut T, Added<T>> both seem like things we should support somehow though

[alice-i-cecile]

This is very much something we should support on the end user level, yes.

Changed and Added filters don't actually need write access to the component data, fixing this bug is sound. The trick is that they may need write access to their own data. But this only occurs if the underlying component data is changed. As long as the mutability rules are obeyed by the queries for the component data itself, this will always remain sound.

[mockersf]

Changed and Added filters don't actually need write access to the component data

They don't ask for write, they only ask for read, but there's already a write access for the same component, so we can't take a read on it.

Should we add a third level of permission? Read / Write / Meta?

[alice-i-cecile]

Should we add a third level of permission? Read / Write / Meta?

This is an interesting idea... I don't mind it as a workaround.

Overall this feels like an indication that the change tracker needs to somehow be more closely integrated onto the component storage itself. I'm not at all sure how we might do that though.

[alice-i-cecile]

Okay, thinking this through a bit more clearly...

The fundamental reason why this works is that the query filter reads the component data before the components are actually handed out to be changed. Thus, we should be able to use a Changed filter in one query, and a mutable fetch of the same data in another query on the same system. Doing so across systems is much more dangerous though (I think just shouldn't be allowed), because we have no guarantees on whether the system will have started mutating things before or after we check what's changed.

As a result, I think the correct solution is actually to split this out into two phases of access within each system, with the system's total access being the union of the access across the two phases. In the "filter" phase, we handle With / Without / Changed / Added, and then in the "query" phase we handle the actual data.

You could even free up data access once the filter phase is complete to allow for enhanced parallelism in the future, reducing the duration that systems which only filter on particular components block new writes on them.

[mockersf]

We already have two levels of check done:

to check if a system can run

is_compatible:

bevy/crates/bevy_ecs/src/query/access.rs

Lines 75 to 84 in b17f8a4

	pub fn is_compatible(&self, other: &Access<T>) -> bool {
	if self.reads_all {
	0 == other.writes.count_ones(..)
	} else if other.reads_all {
	0 == self.writes.count_ones(..)
	} else {
	self.writes.is_disjoint(&other.reads_and_writes)
	&& self.reads_and_writes.is_disjoint(&other.writes)
	}
	}

updated by update_archetype_component_access:

bevy/crates/bevy_ecs/src/query/fetch.rs

Lines 216 to 226 in 9657f58

	fn update_archetype_component_access(
	&self,
	archetype: &Archetype,
	access: &mut Access<ArchetypeComponentId>,
	) {
	if let Some(archetype_component_id) =
	archetype.get_archetype_component_id(self.component_id)
	{
	access.add_read(archetype_component_id);
	}
	}

to check if a query is valid

And update_component_access that will panic when building the query

bevy/crates/bevy_ecs/src/query/fetch.rs

Lines 208 to 214 in 9657f58

	fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
	if access.access().has_write(self.component_id) {
	panic!("&{} conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
	std::any::type_name::<T>());
	}
	access.add_read(self.component_id)
	}

Meta solution?

A solution with a Meta access right would be to block at the archetype level but not at the component level

[alice-i-cecile]

A solution with a Meta access right would be to block at the archetype level but not at the component level

I'm not sure I understand how this proposal would work; could you expand on why it would help? As I understand it this problem would arise even if we only had a single archetype per component.

[mockersf]

we are storing read and write for the archetype and the components level, so that already works when you have a single component and two systems that need a &mut query on that component, they are not scheduled in parallel.

The idea would be to add a right Meta that has the same property as Write for system check, and the same property as Read for query check

[cart]

Thus, we should be able to use a Changed filter in one query, and a mutable fetch of the same data in another query on the same system.

Not quite. These queries still "conflict" in that you could "collect" references of the mutable query fetches, then iterate the "changed filter" query, which could result in an immutable reference being accessed while the mutable reference exists. In practice it probably wouldn't cause problems for most people, but I still wouldn't be comfortable doing it.

The idea would be to add a right Meta that has the same property as Write for system check, and the same property as Read for query check

I don't think we need to complicate the access control here. I think the current logic behaves correctly when comparing two different queries / accesses (both within systems and across systems).

I think this can be resolved by simply adjusting our order of operations slightly and "sandboxing" query filter evaluation. When we first construct a query and collect component access:

1. Leave normal query fetch logic completely unchanged
2. Create a new (temporary) empty FilteredAccess and pass that into the filter's FetchState::update_component_access. This ensures filters don't conflict with the query's normal component access (which is safe because filters are evaluated before accessing the component). It also still ensures each filter's access doesn't internally conflict (although I think we could safely drop this, but imo thats a separate conversation).
3. Merge the temporary FilteredAccess with the main FilteredAccess. This ensures cross-query comparisons conflict in the same way that they do today (which is correct imo).

[cart]

I'm putting this together now. Should be a pretty quick fix.