[Merged by Bors] - Fix FilteredAccessSet get_conflicts inconsistency by nicopap · Pull Request #5105 · bevyengine/bevy

nicopap · 2022-06-26T16:03:31Z

Objective

Enable Res and Query parameter mutual exclusion
Required for Add the AssetChanged query filter #5080

The FilteredAccessSet::get_conflicts methods didn't work properly with
Res and ResMut parameters. Because those added their access by using
the combined_access_mut method and directly modifying the global
access state of the FilteredAccessSet. This caused an inconsistency,
because get_conflicts assumes that ALL added access have a corresponding
FilteredAccess added to the filtered_accesses field.

In practice, that means that SystemParam that adds their access through
the Access returned by combined_access_mut and the ones that add
their access using the add method lived in two different universes. As
a result, they could never be mutually exclusive.

Solution

This commit fixes it by removing the combined_access_mut method. This
ensures that the combined_access field of FilteredAccessSet is always
updated consistently with the addition of a filter. When checking for
filtered access, it is now possible to account for Res and ResMut
invalid access. This is currently not needed, but might be in the
future.

We add the add_unfiltered_{read,write} methods to replace previous
usages of combined_access_mut.

We also add improved Debug implementations on FixedBitSet so that their
meaning is much clearer in debug output.

Changelog

Fix Res and Query parameter never being mutually exclusive.

Migration Guide

Note: this mostly changes ECS internals, but since the API is public, it is technically breaking:

Removed FilteredAccessSet::combined_access_mut
- Replace immutable usage of those by combined_access
- For mutable usages, use the new add_unfiltered_{read,write} methods instead of combined_access_mut followed by add_{read,write}

crates/bevy_ecs/src/storage/sparse_set.rs

crates/bevy_ecs/src/query/access.rs

alice-i-cecile

This is signifcantly clearer, and the logic changes make sense to me.

I particularly like the improved Debug methods.

nicopap · 2022-06-26T19:08:05Z

I've hesitated between forcing to add filters and special-handling the case where there are no filters and a conflict in get_conflicts, but I'm not sure if it acts the same way. The force-add filters will be slower. (but since the check is done only once at initialization, I don't think it's much of an issue)

alice-i-cecile · 2022-06-26T20:07:44Z

I've hesitated between forcing to add filters and special-handling the case where there are no filters and a conflict in get_conflicts, but I'm not sure if it acts the same way. The force-add filters will be slower. (but since the check is done only once at initialization, I don't think it's much of an issue)

I would optimize for clarity and correctness over initialization speed here, due to the critical and complex nature of this code. I agree with your choice.

alice-i-cecile · 2022-06-26T21:39:13Z

bors try

bors · 2022-06-27T00:39:32Z

try

Timed out.

alice-i-cecile · 2022-06-27T00:43:26Z

@nicopap is this building for you locally? It's timing out on CI.

nicopap · 2022-06-27T07:09:11Z

No idea. Builds on my machine

nicopap · 2022-07-01T07:33:38Z

Ok, now the tests are timing out on my own machine.

nicopap · 2022-07-01T09:18:55Z

Oh my god, I think this is a deadlock in the world test in crates/bevy_ecs/src/schedule/executor_parallel.rs. It's very racy. Someone send help 🙏

Edit: Turns out I'm an idiot! The change to Access::is_compatible was erroneous and caused some nasty bugs in the scheduler.

crates/bevy_ecs/src/query/access.rs

BoxyUwU · 2022-07-21T22:48:38Z

Why can't we change Res(Mut) to call .add instead of using combined_access_mut

nicopap · 2022-07-22T05:55:01Z

Why can't we change Res(Mut) to call .add instead of using combined_access_mut

add_unfiltered is exactly the same as add, but with a default empty FilteredAccess, it's just less work on the caller side, a more convenient API.

Also I removed combined_access_mut since you can't use it without breaking the assumptions about combined_access being a combination of the filtered accesses.

nicopap · 2022-11-03T16:33:12Z

bors try

bors · 2022-11-03T17:09:05Z

try

Build succeeded:

joseph-gio

I agree with the motivation for this PR, encapsulating the access invariants is very valuable. The implementation looks good and the doc changes are excellent.

joseph-gio · 2022-11-03T17:01:28Z

crates/bevy_ecs/src/query/access.rs

+/// ```text
+/// read_and_writes: [ ComponentId(5), ComponentId(7) ]
+/// ```
+struct FormattedBitSet<'a, T: SparseSetIndex> {


This debug formatting is so much nicer.

cart

I'm on board for these changes!

crates/bevy_ecs/src/storage/sparse_set.rs

The `FilteredAccessSet::get_conflicts` methods didn't work properly with `Res` and `ResMut` parameters. Because those added their access by using the `combined_access_mut` method and directly modifying the global access state of the FilteredAccessSet. This caused an inconsistency, because get_conflicts assumes that ALL added access have a corresponding `FilteredAccess` added to the `filtered_accesses` field. In practice, that means that SystemParam that adds their access through the `Access` returned by `combined_access_mut` and the ones that add their access using the `add` method lived in two different universes. As a result, they could never be mutually exclusive. This commit fixes it by removing the `combined_access_mut` method. This ensures that the `combined_access` field of FilteredAccessSet is always updated consistently with the addition of a filter. When checking for filtered access, it is now possible to account for `Res` and `ResMut` invalid access. This is currently not needed, but might be in the future. We add the `add_unfiltered_{read,write}` methods to replace previous usages of `combined_access_mut`. We also add improved Debug implementations on FixedBitSet so that their meaning is much clearer in debug output.

alice-i-cecile · 2022-11-16T11:05:13Z

bors r+

# Objective * Enable `Res` and `Query` parameter mutual exclusion * Required for #5080 The `FilteredAccessSet::get_conflicts` methods didn't work properly with `Res` and `ResMut` parameters. Because those added their access by using the `combined_access_mut` method and directly modifying the global access state of the FilteredAccessSet. This caused an inconsistency, because get_conflicts assumes that ALL added access have a corresponding `FilteredAccess` added to the `filtered_accesses` field. In practice, that means that SystemParam that adds their access through the `Access` returned by `combined_access_mut` and the ones that add their access using the `add` method lived in two different universes. As a result, they could never be mutually exclusive. ## Solution This commit fixes it by removing the `combined_access_mut` method. This ensures that the `combined_access` field of FilteredAccessSet is always updated consistently with the addition of a filter. When checking for filtered access, it is now possible to account for `Res` and `ResMut` invalid access. This is currently not needed, but might be in the future. We add the `add_unfiltered_{read,write}` methods to replace previous usages of `combined_access_mut`. We also add improved Debug implementations on FixedBitSet so that their meaning is much clearer in debug output. --- ## Changelog * Fix `Res` and `Query` parameter never being mutually exclusive. ## Migration Guide Note: this mostly changes ECS internals, but since the API is public, it is technically breaking: * Removed `FilteredAccessSet::combined_access_mut` * Replace _immutable_ usage of those by `combined_access` * For _mutable_ usages, use the new `add_unfiltered_{read,write}` methods instead of `combined_access_mut` followed by `add_{read,write}`

bors · 2022-11-16T11:27:34Z

Pull request successfully merged into main.

Build succeeded:

# Objective * Enable `Res` and `Query` parameter mutual exclusion * Required for #5080 The `FilteredAccessSet::get_conflicts` methods didn't work properly with `Res` and `ResMut` parameters. Because those added their access by using the `combined_access_mut` method and directly modifying the global access state of the FilteredAccessSet. This caused an inconsistency, because get_conflicts assumes that ALL added access have a corresponding `FilteredAccess` added to the `filtered_accesses` field. In practice, that means that SystemParam that adds their access through the `Access` returned by `combined_access_mut` and the ones that add their access using the `add` method lived in two different universes. As a result, they could never be mutually exclusive. ## Solution This commit fixes it by removing the `combined_access_mut` method. This ensures that the `combined_access` field of FilteredAccessSet is always updated consistently with the addition of a filter. When checking for filtered access, it is now possible to account for `Res` and `ResMut` invalid access. This is currently not needed, but might be in the future. We add the `add_unfiltered_{read,write}` methods to replace previous usages of `combined_access_mut`. We also add improved Debug implementations on FixedBitSet so that their meaning is much clearer in debug output. --- ## Changelog * Fix `Res` and `Query` parameter never being mutually exclusive. ## Migration Guide Note: this mostly changes ECS internals, but since the API is public, it is technically breaking: * Removed `FilteredAccessSet::combined_access_mut` * Replace _immutable_ usage of those by `combined_access` * For _mutable_ usages, use the new `add_unfiltered_{read,write}` methods instead of `combined_access_mut` followed by `add_{read,write}`

# Objective * Enable `Res` and `Query` parameter mutual exclusion * Required for bevyengine#5080 The `FilteredAccessSet::get_conflicts` methods didn't work properly with `Res` and `ResMut` parameters. Because those added their access by using the `combined_access_mut` method and directly modifying the global access state of the FilteredAccessSet. This caused an inconsistency, because get_conflicts assumes that ALL added access have a corresponding `FilteredAccess` added to the `filtered_accesses` field. In practice, that means that SystemParam that adds their access through the `Access` returned by `combined_access_mut` and the ones that add their access using the `add` method lived in two different universes. As a result, they could never be mutually exclusive. ## Solution This commit fixes it by removing the `combined_access_mut` method. This ensures that the `combined_access` field of FilteredAccessSet is always updated consistently with the addition of a filter. When checking for filtered access, it is now possible to account for `Res` and `ResMut` invalid access. This is currently not needed, but might be in the future. We add the `add_unfiltered_{read,write}` methods to replace previous usages of `combined_access_mut`. We also add improved Debug implementations on FixedBitSet so that their meaning is much clearer in debug output. --- ## Changelog * Fix `Res` and `Query` parameter never being mutually exclusive. ## Migration Guide Note: this mostly changes ECS internals, but since the API is public, it is technically breaking: * Removed `FilteredAccessSet::combined_access_mut` * Replace _immutable_ usage of those by `combined_access` * For _mutable_ usages, use the new `add_unfiltered_{read,write}` methods instead of `combined_access_mut` followed by `add_{read,write}`

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jun 26, 2022

nicopap force-pushed the improve-filtered-access-set branch from 0bdf8e4 to 04758b8 Compare June 26, 2022 16:05

alice-i-cecile requested a review from BoxyUwU June 26, 2022 16:11

nicopap force-pushed the improve-filtered-access-set branch from 04758b8 to 3b71c03 Compare June 26, 2022 16:22

alice-i-cecile reviewed Jun 26, 2022

View reviewed changes

crates/bevy_ecs/src/storage/sparse_set.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 26, 2022

View reviewed changes

crates/bevy_ecs/src/query/access.rs Show resolved Hide resolved

alice-i-cecile approved these changes Jun 26, 2022

View reviewed changes

alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Jun 26, 2022

nicopap force-pushed the improve-filtered-access-set branch from 3b71c03 to ecb159a Compare June 26, 2022 16:35

alice-i-cecile requested a review from cart June 26, 2022 16:40

bors bot added a commit that referenced this pull request Jun 26, 2022

Try #5105:

41e45f0

alice-i-cecile requested a review from maniwani June 29, 2022 03:58

nicopap force-pushed the improve-filtered-access-set branch from ecb159a to c2072e0 Compare June 30, 2022 12:36

nicopap commented Jul 1, 2022

View reviewed changes

crates/bevy_ecs/src/query/access.rs Outdated Show resolved Hide resolved

nicopap force-pushed the improve-filtered-access-set branch 2 times, most recently from c29ca7e to 61ce88e Compare July 1, 2022 10:47

nicopap mentioned this pull request Jul 21, 2022

Add the AssetChanged query filter #5080

Closed

nicopap force-pushed the improve-filtered-access-set branch from 61ce88e to 84ecf53 Compare September 1, 2022 08:53

bors bot added a commit that referenced this pull request Nov 3, 2022

Try #5105:

1638e18

joseph-gio approved these changes Nov 3, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 14, 2022

alice-i-cecile added this to the 0.9.1 milestone Nov 14, 2022

cart reviewed Nov 15, 2022

View reviewed changes

crates/bevy_ecs/src/storage/sparse_set.rs Outdated Show resolved Hide resolved

nicopap added 2 commits November 16, 2022 09:02

Add read_all access conflict test

8df367a

nicopap force-pushed the improve-filtered-access-set branch from 84ecf53 to 8df367a Compare November 16, 2022 08:03

Precise use of SparseSetIndex

d9dd553

bors bot changed the title ~~Fix FilteredAccessSet get_conflicts inconsistency~~ [Merged by Bors] - Fix FilteredAccessSet get_conflicts inconsistency Nov 16, 2022

bors bot closed this Nov 16, 2022

nicopap deleted the improve-filtered-access-set branch September 5, 2023 15:44

Uh oh!

Conversation

nicopap commented Jun 26, 2022

Objective

Solution

Changelog

Migration Guide

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

nicopap commented Jun 26, 2022

Uh oh!

alice-i-cecile commented Jun 26, 2022

Uh oh!

alice-i-cecile commented Jun 26, 2022

Uh oh!

bors bot commented Jun 27, 2022

try

Uh oh!

alice-i-cecile commented Jun 27, 2022

Uh oh!

nicopap commented Jun 27, 2022

Uh oh!

nicopap commented Jul 1, 2022

Uh oh!

nicopap commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

BoxyUwU commented Jul 21, 2022

Uh oh!

nicopap commented Jul 22, 2022

Uh oh!

nicopap commented Nov 3, 2022

Uh oh!

bors bot commented Nov 3, 2022

try

Uh oh!

joseph-gio left a comment

Choose a reason for hiding this comment

Uh oh!

joseph-gio Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile commented Nov 16, 2022

Uh oh!

bors bot commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicopap commented Jul 1, 2022 •

edited

Loading