Add the AssetChanged query filter

[nicopap]

Objective

• Closes A query filter for Asset updates #5069
• Blocked on: [Merged by Bors] - Fix FilteredAccessSet get_conflicts inconsistency #5105

Solution

• Add the AssetChanges<T> resource.
• Add the AssetChanged<T> world query
• Add the AssetOrHandleChanged<T> world query type alias

AssetChanges<T> resource does the following:

• It is a HashMap<HandleId, Tick>
• If AssetChanges<T> is present, each AssetEvent<T> emitted in Assets::<T>::asset_event_system will also update the relevant entry in the AssetChanges hash map.

AssetChanged<T> does the following:

• It acts like &Handle<T> with one difference:
• Its init_state adds the AssetChanges<T> resource in the world if not present
• Its init_fetch fetches the AssetChanges<T> resource in addition to the &Handle<T>. (panics if the resource was removed)
• It checks if the handle was updated since last time the system ran, and filters on it

Design decisions

Performance

• AssetChanges<T> is a fairly costly operation, since it does a hashmap lookup per entity iterated.
• We do not maintain and update an AssetChanges<T> for assets that do not have a AssetChanged<T> world query.
• The fact that AssetChanged both accesses (and locks) a component Handle<T> and a resource AssetChanges is unique and might come off as surprising. But I don't see how this could introduce a soundness issue.
• If using "Single arc tree" in Bevy Asset V2 #8624 turns out to be impossible for change tick, a few optimizations are possibles:
  • bloom filters seem promising
  • a coarse-grained asset tick, that we could compare directly instead of fetching individual asset change ticks.

Relation with Asset v2 (#8624)

The "Single Arc tree" bullet point in "Bevy Assets v2" section mentions we could attach metadata to handles. Using this for change ticks would make the implementation much more performant, and simplify the implementation.

AssetChanges Update time

I chose to update the change_ticks hash map during asset event
propagation rather than when accessing the asset, because we need access
somehow to the current change_tick. Updating the tick hash map during
modification would require passing the change_tick to all methods of
Assets that accepts a &mut self. Which would be a huge ergonomic
loss.

As a result, the asset change expressed by AssetChanged is only
effective after asset_event_system ran. This is in line with how
AssetEvents currently works, but might cause a 1 frame lag for asset
change detection.

Alternatives

1. [Merged by Bors] - Recalculate entity aabbs when meshes change #4944 (reverted due to perf issues)
2. Potentially (not confirmed) Bevy Asset V2 #8624 using the single arc tree as mentioned
3. Using a component (I will explore this)

---

Changelog

• Added AssetChanged<A> world query, it filters Handle<A> for which the underlying asset got modified since the last time the system ran.
• Added AssetOrHandleChanged<A>, a type alias for Or<(Changed<Handle<T>>, AssetChanged<T>)>, which accounts for case where the Handle changed instead of the underlying asset.

[alice-i-cecile]

Impressive work. This should be extremely useful.

The world-query impls look correct to me; nicely done.

I'm a bit nervous at how much of the change detection code is duplicated. Can we reuse primitives from bevy_ecs's change detection? I'm fine to make more parts pub as needed for this.

[alice-i-cecile]

This could use some more elaboration. In particular, I would note:

1. This is for the asset that that Handle<T> points to.
2. Entities without a Handle<T> component will never be found by this filter.
3. A small doc example :)
4. Contrast to the Changed<Handle<T>> query filter, which instead asks "has what the handle pointed to changed".
5. Demonstrate the Or<(Changed<Handle<T>>, AssetChanged<T>)> pattern, which will be commonly needed when you need to respond to asset changes.
6. This is incompatible with ResMut<Assets<T>>.

[nicopap]

Yes to all this, I'll clean up the docs. In particular, I think (5) is indeed going to be very common. I'm thinking maybe a type alias could help

pub type AssetOrHandleChanged<T> = Or<(
    Changed<Handle<T>>,
    AssetChanged<T>,
)>

(6) is kinda misleading. I think I might just store the change_ticks field in its own resource, so that the incompatibility is more visible and isolated.

[alice-i-cecile]

I like the idea of a type alias for 5 :)

[alice-i-cecile]

Can we do the same thing for AssetLoaded ala Added filters? If so, that would be incredibly valuable. I'm not sure if we should make that part of this PR, but I'm curious to hear your thoughts.

[alice-i-cecile]

This needs tests :) One of the ones that's easily overlooked is that we should assert that AssetChanged<T> is incompatible with ResMut<Assets<T>>.

That should be a compile fail test; I think it should be fine to put into the bevy_ecs_compile_fail crate.

[jakobhellermann]

That should be a compile fail test; I think it should be fine to put into the bevy_ecs_compile_fail crate.

I think it would fail at runtime when the system is added (or first ran?). Conflicting system params aren't caught at compile time.

[nicopap]

I'm a bit nervous at how much of the change detection code is duplicated.

Well, fault of ECS for not newtyping a Tick(u32) and define the difference checks as methods on those :P I guess I could abstract the checks as extension methods on u32 and use them instead. What do you think?

Can we do the same thing for AssetLoaded ala Added filters?

Very likely, I'll check and add if possible.

AssetChanged<T> is incompatible with ResMut<Assets<T>>

Currently it is! But I can make it not so by splitting change_ticks out of Assets.

This needs tests

I agree. I'll see to it. What do you think of benchmarks as well? I think this filter might be particularly slow due to the 2 levels of indirection at each iteration

[nicopap]

In discord, an alternative design where ResMut<Assets<T>> is replaced by AssetsMut<T> was proposed. This in combination with the "mutating methods all require a tick parameter" would ensure same-frame update detection. However it would require a lot of user code change and might make it impossible to conciliate AssetMut with AssetChanged.

[alice-i-cecile]

My preference would be to newtype a Tick(u32) to reduce duplication. IMO we should do this as a seperate uncontroversial PR and then rebase to make this easier to review. It's a good idea regardless.

[cart]

The "Single Arc tree" bullet point in "Bevy Assets v2" section mentions we could attach metadata to handles. Using this for change ticks would make the implementation much more performant, and simplify the implementation.

I believe we could store change ticks on Handles, if we're willing to store ticks as something like Arc<AtomicUsize>. Whether we should is a bit of an open question (ill need to spend more time with the problem). We could also consider "smart pointer" change detection that bumps the tick on mutate (much like we do for components). Not (yet) advocating for it, but it would be compatible with going down the Assets as Entities route (see the section in the Asset V2 pr for details on that).

In general the current approach in this PR should be compatible with the Asset V2 effort.

And I do like that this is currently a "only pay the price if you use it" abstraction. Probably worth waiting to merge until after Asset V2 lands though and then updating this. This isn't the kind of port I'd block an Asset V2 merge on, so it might get lost in mix if we merge it now.

[nicopap]

Thank you for the feedback. I agree with you, I think waiting is the good idea. I'll put the PR back into draft mode.

[djeedai]

We were discussing uniformizing components and assets change detection under Mut<T> on Discord here, for the sake of being able to handle both with Mut<T> alone generically without having to know that the thing is a component or an asset (something I've been trying to do for example for several releases in Bevy Tweening, because animating doesn't care where the object to animate comes from, but currently needs 2 duplicates codepaths only because of different change detection mechanisms).

This PR was linked as a related work, however it looks from my untrained eye like it's going in the opposite direction and making assets and components even more different. I think it would be useful if possible to explain how this change detection mechanism differs from the one of components (based on ticks and Mut<T>), and why it's not possible to use that proven system for assets. I personally don't know enough to see the obstacles, and therefore any solution not going in that direction feels wrong. From a first quick reading it looks like the work does use ticks, but possibly differs in where those ticks are stored (?).

I understand that the objective of this change is more about query filtering, however if we could converge asset change detection toward Mut<T> then we would have an existing familiar pattern for users and would immediately benefit I think (?) from existing query filtering mechanisms like Added<T> which as far as I know are based on ticks like Mut.

[nicopap]

@djeedai

All the changes relevant to your question are in the crates/bevy_asset/src/assets.rs file.

Ticks for components are stored in Column. Tick filters are the Added<T> and Changed<T> world queries. Tick filters read the tick from the tick storage of the column. Each time the query matches a component, the tick filter will fetch the tick in the column and compare it with the SystemChangeTick. The tick filter will skip entities with incompatible change ticks.

Unlike the ECS, an Assets<A> is a HashMap<AssetId, A>. Assets v2 allows for some additional metadata per assets. The "single arc tree" — mentioned in the PR description — let you get metadata from Handle<A>. But the change tick is not part of those metadata.

In bevy_assets, change ticks are events. They are stored in the queued_events field of the Assets struct. The asset_events system pushes those events to an EventWriter. Normally, you would access the asset events through the EventReader<AssetEvent<A>> system parameter. This PR simply modifies the asset_events system to update an additional resource: AssetChanges<A>.

Consider reading the "Solution" section of the PR description to get the rest of the story.

The design you describe

I'm not sure I follow you, but I gather you want an Assets SystemParam with a Assets::get_mut(…) -> Option<Mut<A>> method, rather than get_mut that always marks the asset as changed. Maybe you are also thinking of a sort of WorldQuery that let you access a Mut<A> instead of the current Handle<A>?

I think it would be possible. You say that this PR makes "assets and components even more different". But I don't think that's true. It would move forward the design space toward an unified query API for assets and components IMO.

Though I think, indeed, most of the energy required to be spent for an unifying design would be spent in orthogonal sections of the code; Notably, by system-paramifying Assets, so that its methods would have access to SystemChangeTick, though this isn't an exhaustive list of changes required to accomplish unification. Even then, already, if this PR gets merged, we would have started to move toward this unified design.

[djeedai]

I think that's mostly OK overall, but there's a couple of TODOs that should probably be fixed, like the change detection at start that @alice-i-cecile highlighted in comment (although not sure since they approved anyway).

[djeedai]

Why hidden docs? This is a public type.

[nicopap]

I don't want this to be part of the public API. It has to be public because the asset_events system accesses it, and is itself public. But otherwise it shouldn't be touched by 3rd parties. Bereft the choice of making the type pub(crate), the second best option is making it #[doc(hidden)].

We could probably make asset_events private though. What do you think?

[djeedai]

My two cents rule for my own projects is:

• label all systems so users can order theirs relative to mine
• make systems themselves non public

That works around the issue here I think. But not sure this is the best approach.

Otherwise I'm fine with the explanation. Just wanted to confirm this was intended. Maybe leave a comment to explain?

[alice-i-cecile]

That's generally my preferred approach as well.

[djeedai]

Yeah that sounds like a major footgun. Even if documented here, this is quite the unexpected behavior for users.

[djeedai]

Thanks @nicopap for the detailed answer. I was skeptical about whether the change goes in the direction of uniform component/asset handling because of the various new Asset-prefixed types, which make it look like everything is duplicated. However I do get that this moves the cursor in the direction where both assets and components have a similar query filter mechanism, which could be in the future merged together, so from that perspective I guess it does improve the situation compared to the current version where asset change detection is only event based. So no objection for that change. Reviewed as best as I could; this is a bit out of my knowledge area.

[djeedai]

@cart

I believe we could store change ticks on Handles, if we're willing to store ticks as something like Arc<AtomicUsize>.

I see that Handle<T> is an Arc to a StrongHandle, which as I understand is where any metadata, including change detection ticks, goes. Why would we need another Arc? Can't we just store an AtomicUsize as a field of StrongHandle?

Whether we should is a bit of an open question (ill need to spend more time with the problem).

Looking at this PR it looks like this would help a few things like query filters and change detection via smart pointer. This also makes the mental model for change detection consistent between assets and components (even if we don't immediately use exactly the same types). Is there any catch you can think of already? Or is it just about due diligence before diving into that path?

We could also consider "smart pointer" change detection that bumps the tick on mutate (much like we do for components).

The ideal for me (for Bevy Tweening, but I'm sure there are other uses) would be if we can use exactly Mut itself, so code handling mutations can deal with assets and components uniformly/generically without having to care about the kind of mutated object nor having to carry around Assets<T> like is needed at the minute.

[bas-ie]

Backlog cleanup: from what I can tell, this 2022 PR could still prove valuable and probably should be adopted. I'll close the PR but mention as much on #5069.