Add AssetChanged query filter

[tychedelia]

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]

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]

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]

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]

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.

[tychedelia]

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.

[tychedelia]

Sorry, I also misunderstood you. Multiple assets of the same type should be much easier to handle. I'd been hung up on if, for example we decided to merge mesh and material as has been discussed before.

[tychedelia]

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.

[pcwalton]

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

[chescock]

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.

[tychedelia]

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

[tychedelia]

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?

[chescock]

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.

[tychedelia]

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

[chescock]

Not quite. That adds access to components on entities in archetypes matched by the query. But you want to add access to resources from the world. Resources on the world have their own ArchetypeComponentIds that don't appear in any archetypes.

If we just want to support this use case of a finite set of read access, I think it's enough to add code to Query::new_with_access that does

for component_id in state.component_access.resource_reads() {
    access.add_resource_read(world.initialize_resource_internal(component_id).id());
}

More complete support would need to check for writes and for reads_all and writes_all, and look a bit like the code in FilteredResourcesMutParamBuilder

bevy/crates/bevy_ecs/src/system/builder.rs

Line 644 in 6178ce9

if access.has_read_all_resources() {

---

The background here is that adding access in a QueryData has only ever added access to entities matched by the query. There has never been a way to declare that you want access to resources from the world.

But the code you wrote looks like it should give access to resources! And that access is useful! So I think we should add the plumbing to make it work.

It might be cleaner to do that in a separate PR, though, and put this one back to what you had originally. This one would just be unsound until the other one was merged. If you'd like, I can try to put one together on Monday.

[alice-i-cecile]

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