Add `AssetChanged` query filter #16810

tychedelia · 2024-12-13T20:20:48Z

Objective

Implement a new AssetChanged query filter that allows users to query for entities whose related assets may have changed.

Closes A query filter for Asset updates #5069
Unblocks Cold specialization #16420. Currently, cold-specialization, a key rendering optimization for unlocking ancillary benefits of the retained render world, is blocked on being unable detect all scenarios in which an entity's mesh/material changes using events and observers. An AssetChanged filter will drastically simplify our implementation and be more robust to future changes.

Originally implemented by @nicopap in #5080.

Solution

Adds a new AssetChanged query filter that initializes a AssetChanges<A> resource that tracks changed assets and ticks in asset_events.
~~Reverts constrain WorldQuery::get_state to only use &Components #13343 and changes the api of get_state to accept impl Into<UnsafeWorldCell<'w>> to allow accessing the AssetChanges<A> resource.~~
Adds a AsAssetId trait used for newtype handle wrappers (e.g. Mesh3d) that allows associating a component with the underlying Asset it represents.

Testing

Tests are added for AssetChanged.
TBD on performance. We are going to add this Mesh3d and MeshMaterial3d (etc) in the renderer. Long term wins in render performance this unblocks should swamp tracking overhead for any realistic workload.

Migration Guide

The asset_events system is no longer public. Users should order their systems relative to the AssetEvents system set.

Victoronz · 2024-12-13T20:39:59Z

Why does this revert the get_state PR? In the get_state impl of this PR, I only see a call to resource_id on world, which is already available on &Components.

tychedelia · 2024-12-13T20:48:49Z

Why does this revert the get_state PR? In the get_state impl of this PR, I only see a call to resource_id on world, which is already available on &Components.

Because I didn't know that. Will change it back!

Victoronz · 2024-12-13T20:59:35Z

Separately, how should the resource access in init_fetch be reasoned about?
A type being private means it cannot be mentioned in user code, however you do not have to mention a type to obtain it via public API! In this case, this is possible via id-based resource fetching.
UnsafeWorldCell::get_resource gives its caller the responsibility to ensure it has access to that resource, however this safety contract cannot be passed on to users of the filter.
For both users, libraries and other in-engine areas, how should we communicate about this?

pcwalton · 2024-12-13T20:35:57Z

crates/bevy_asset/src/lib.rs

@@ -141,13 +141,15 @@
 // FIXME(3492): remove once docs are ready
 // FIXME(15321): solve CI failures, then replace with `#![expect()]`.
 #![allow(missing_docs, reason = "Not all docs are written yet, see #3492.")]
+#![allow(unsafe_code)]


Should we just #![allow(unsafe_code)] wherever we need it instead of throughout the crate?

That's what the other crates seem to do, I'm not unsure about our policy here.

Much prefer local allows here.

pcwalton · 2024-12-13T21:00:01Z

crates/bevy_asset/src/asset_changed.rs

+
+/// Filter that selects entities with a `A` for an asset that changed
+/// after the system last ran, where `A` is a component that implements
+/// [`AsAssetId`].


What about components that store handles to multiple assets of the same type? We don't need to do that right now, but we might have to revisit it later. In my app I have components with handles to dozens of AnimationClips for example. I think you could maybe just have AsAssetId's method return -> impl Iterator<Item = AssetId>.

This isn't blocking, just a suggestion.

The bigger issue is how to handle the associated type. We might have to use some kind of indirection via UntypedAssetId instead to be able to handle multiple kinds of assets.

crates/bevy_asset/src/asset_changed.rs

crates/bevy_asset/src/lib.rs

crates/bevy_ecs/src/query/fetch.rs

tychedelia · 2024-12-13T21:08:16Z

In this case, this is possible via id-based resource fetching.

How would the user forge an id if they do not have access to the underlying type? UnsafeWorldCell::get_resource already documents the aliasing requirements as an unsafe API. If users are iterating or creating random opaque ComponentIds and getting mutable access they trivially violate that contract by not knowing what resource they are touching.

Co-authored-by: Patrick Walton <[email protected]>

pcwalton · 2024-12-13T21:08:56Z

Overall this looks fine to me, but as I mentioned we should have an ECS expert look over the get_state signature change.

chescock · 2024-12-13T21:45:30Z

crates/bevy_asset/src/asset_changed.rs

+    #[inline]
+    fn update_component_access(state: &Self::State, access: &mut FilteredAccess<ComponentId>) {
+        <&A>::update_component_access(&state.asset_id, access);
+        access.add_resource_read(state.resource_id);


I don't think this line is quite enough. We need Query to report the access in Access<ArchetypeComponentId> for it to be used by the scheduler, and it currently does that in QueryState::update_archetype_component_access(), which only looks at component_reads_and_writes() and component_writes().

I think you need modify QueryState::new_with_access() to look at resource_reads_and_writes() and resource_writes() and convert them to ArchetypeComponentIds.

I think you also need to modify FilteredAccess::is_compatible() to consider resources to conflict even when the filters don't overlap. (That may need to get reworked in the future for resources-as-components, but the semantics seem reasonable.)

It would be good to add a unit test that verifies that the access is set up correctly. Maybe check that fn system(r: ResMut<AssetChanges<A>>, q: Query<Entity, AssetChanged<A>>) panics when passed to assert_system_does_not_conflict, and again with the parameters reversed.

Alright thanks for the close review. Will add a test to validate.

So just added a test and without making any other changes am getting the following error:

error[B0001]: Query<MyComponent, AssetChanged<MyComponent>> in system bevy_asset::asset_changed::tests::resource_conflict::system accesses component(s)AssetChanges<MyAsset> in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001

Thoughts?

Oh, right, ResMut does unfiltered access, so FilteredAccess::is_compatible() will see that the filters overlap and catch it. Great! So that won't be a problem unless someone tries to do mutable resource access from a query.

I think it's just the Access<ArchetypeComponentId> that would be an issue, then. I'm not coming up with a simple way to write a test for that, though.

So, I think it's just adding the following to QueryState::update_archetype_component_access? d0130a3

crates/bevy_asset/src/asset_changed.rs

alice-i-cecile · 2024-12-14T05:41:26Z

crates/bevy_ecs/src/query/state.rs

@@ -583,6 +583,16 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
            {
                access.add_component_write(archetype_component_id);
            }
+            if self.component_access.access.has_resource_read(component_id) {


Oh right, this is an existing soundness bug. Re-adding the tag.

Co-authored-by: Alice Cecile <[email protected]>

nicopap and others added 6 commits December 13, 2024 11:53

Add the AssetChanged query filter

da3a1fc

Implement djeedai feedbacks

ce19c2f

Fix clippy lints

5dbfea9

Rename handle_id

03d4848

Update to main.

5e9bd4a

Implement updates for AssetChanges.

2092eb1

tychedelia added A-ECS Entities, components, systems, and events X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-SME Decision or review from an SME is required labels Dec 13, 2024

tychedelia changed the title ~~Changed handle~~ Add AssetChanged query filter Dec 13, 2024

tychedelia added 2 commits December 13, 2024 12:23

Ci.

2f95290

Fmt.

eae75cf

ItsDoot added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Dec 13, 2024

tychedelia added 2 commits December 13, 2024 12:51

Switch back to using &Components.

4278615

Ci.

1001e27

Victoronz added the P-Unsound A bug that results in undefined compiler behavior label Dec 13, 2024

pcwalton reviewed Dec 13, 2024

View reviewed changes

Update crates/bevy_asset/src/lib.rs

b6ac681

Co-authored-by: Patrick Walton <[email protected]>

tychedelia added 4 commits December 13, 2024 13:29

Respond to feedback.

ec0803c

Ci.

b1640b5

Ci.

3015581

Ci.

d70303d

chescock reviewed Dec 13, 2024

View reviewed changes

alice-i-cecile added this to the 0.16 milestone Dec 13, 2024

alice-i-cecile added D-Unsafe Touches with unsafe code in some way A-Assets Load files from disk to use for things like images, models, and sounds and removed P-Unsound A bug that results in undefined compiler behavior S-Needs-SME Decision or review from an SME is required labels Dec 13, 2024

alice-i-cecile reviewed Dec 13, 2024

View reviewed changes

crates/bevy_asset/src/asset_changed.rs Show resolved Hide resolved

tychedelia added 4 commits December 13, 2024 15:04

Add docs.

53eb8c8

Check resource reads and write in update_archetype_component_access.

d0130a3

Ci.

08a81ca

component_id -> register_component.

13ddde4

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Dec 14, 2024