Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A query filter for Asset updates #5069

Open
nicopap opened this issue Jun 22, 2022 · 1 comment · May be fixed by #16810
Open

A query filter for Asset updates #5069

nicopap opened this issue Jun 22, 2022 · 1 comment · May be fixed by #16810
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
Copy link
Contributor

nicopap commented Jun 22, 2022

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> and Aabb component in the ECS. We want to update Aabb whenever the Mesh for the Handle is updated. The Handle is updated in two circumstances:

  1. The user changes the Handle the entity has by replacing it in a system with a Query<&mut Handle<Mesh>> parameter
  2. The user changes the Mesh in the Assets<Mesh> in a system with a ResMut<Assets<Mesh>> parameter

To react to (1), it's trivial: just add a Changed<Handle<Mesh>> to the system that updates the Aabb.

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 a Handle<Mesh>, so you either need to iterate over ALL entities with a Handle<Mesh> and updates the ones that are equal to the Modified field, or keep a HashMap mapping handles to entities, and other gory details required to maintain this HashMap properly.

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 is true for entities with a Handle<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 optional Mesh field 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:

@nicopap nicopap added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 22, 2022
@Nilirad Nilirad added the A-Assets Load files from disk to use for things like images, models, and sounds label Jun 22, 2022
@alice-i-cecile alice-i-cecile removed the S-Needs-Triage This issue needs to be labelled label Jun 22, 2022
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.
@alice-i-cecile alice-i-cecile moved this to Wishlist in Asset Pipeline Jul 17, 2022
@Selene-Amanita Selene-Amanita added this to the 0.12 milestone Jul 29, 2023
@hymm hymm modified the milestones: 0.12, 0.13 Oct 7, 2023
@alice-i-cecile alice-i-cecile modified the milestones: 0.13, 0.14 Jan 29, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone May 13, 2024
@bas-ie bas-ie added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 2, 2024
@bas-ie
Copy link
Contributor

bas-ie commented Oct 2, 2024

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 BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Oct 13, 2024
@tychedelia tychedelia linked a pull request Dec 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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!
Projects
Status: Wishlist
Development

Successfully merging a pull request may close this issue.

7 participants