A query filter for Asset updates #5069
Labels
A-Assets
Load files from disk to use for things like images, models, and sounds
C-Feature
A new feature, making something new possible
D-Modest
A "normal" level of difficulty; suitable for simple features or challenging fixes
S-Adopt-Me
The original PR author has no intent to complete this work. Pick me up!
Comments
nicopap
added
C-Feature
Nilirad
added
the
A-Assets
nicopap
added a commit
to nicopap/bevy
that referenced
this issue
Jun 23, 2022
Solves bevyengine#5069. `AssetChanged<T>` is just a thin wrapper around the `&Handle<T>` query, with additional logic to check in `Assets<T>` if the corresponding `HandleId` was changed between the last and current runs of the system. This adds some overhead in the `asset_event_system` for keeping track of when which handle got updated. Otherwise, the overhead is constrained to the `AssetChanged` query, which is ripe for optimizations. NOTE: We 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. A potential future update could implement some sort of specialization so that when `Assets` is accessed through a `ResMut`, the called methods are called with the `ResMut`'s change_tick. But with current rust, this is either impossible or causing huge ergonomic loss on the `Assets` API.
nicopap
added a commit
to nicopap/bevy
that referenced
this issue
Jun 23, 2022
Solves bevyengine#5069. `AssetChanged<T>` is just a thin wrapper around the `&Handle<T>` query, with additional logic to check in `Assets<T>` if the corresponding `HandleId` was changed between the last and current runs of the system. This adds some overhead in the `asset_event_system` for keeping track of when which handle got updated. Otherwise, the overhead is constrained to the `AssetChanged` query, which is ripe for optimizations. NOTE: We 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. A potential future update could implement some sort of specialization so that when `Assets` is accessed through a `ResMut`, the called methods are called with the `ResMut`'s change_tick. But with current rust, this is either impossible or causing huge ergonomic loss on the `Assets` API.
nicopap
added a commit
to nicopap/bevy
that referenced
this issue
Jun 28, 2022
Solves bevyengine#5069. `AssetChanged<T>` is just a thin wrapper around the `&Handle<T>` query, with additional logic to check in `Assets<T>` if the corresponding `HandleId` was changed between the last and current runs of the system. This adds some overhead in the `asset_event_system` for keeping track of when which handle got updated. Otherwise, the overhead is constrained to the `AssetChanged` query, which is ripe for optimizations. NOTE: We 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. A potential future update could implement some sort of specialization so that when `Assets` is accessed through a `ResMut`, the called methods are called with the `ResMut`'s change_tick. But with current rust, this is either impossible or causing huge ergonomic loss on the `Assets` API.
richchurcher
added
the
S-Adopt-Me
|
I've closed linked PR #5080 due to its age and inactivity, but it appears this might still be useful work worthy of adoption if anyone's keen! At least, judging by the various cross-issue and PR mentions. |
BenjaminBrienen
added
the
D-Modest
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this solve or what need does it fill?
A major pain point of using
Handle<Asset>as components in the ECS currently is that the assets pointed-to by the handle can change, and there is no simple way of reacting to changes to the asset in the context of the ECS.For example: AABB calculation. We have a
Handle<Mesh>andAabbcomponent in the ECS. We want to updateAabbwhenever theMeshfor theHandleis updated. TheHandleis updated in two circumstances:Handlethe entity has by replacing it in a system with aQuery<&mut Handle<Mesh>>parameterMeshin theAssets<Mesh>in a system with aResMut<Assets<Mesh>>parameterTo react to (1), it's trivial: just add a
Changed<Handle<Mesh>>to the system that updates theAabb.To react to (2), it becomes harder. The only way to react to asset changes is to use the
AssetEvent::Modified { handle: Handle<Mesh> }event. This event only holds aHandle<Mesh>, so you either need to iterate over ALL entities with aHandle<Mesh>and updates the ones that are equal to theModifiedfield, or keep aHashMapmapping handles to entities, and other gory details required to maintain thisHashMapproperly.Most of asset changes are done through (2), which makes it very difficult to react to asset changes in the general case.
This was observed in #4944 and also in personal project related to scene hot-reloading.
What solution would you like?
Introducing a
AssetChanged<A>query filter that istruefor entities with aHandle<A>for which the underlying asset got modified since the last time the system ran.What alternative(s) have you considered?
In the case of
Mesh, it seems that moving Aabb calculation to the generation of the Mesh and keeping it as an optionalMeshfield is a correct solution. This would fall into the category of "asset pre-processing" which is both possible today (through a constructor) and subject to future possible improvements.But this is only a specific solution to a specific problem. In general, it seems reasonable to be able to bridge the gap between assets and ECS with such a query filter.
Additional context
See discussions:
The text was updated successfully, but these errors were encountered: