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

Allow removing and reloading assets with live handles #10785

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

cart
Copy link
Member

@cart cart commented Nov 28, 2023

Objective

Fixes #10444

Currently manually removing an asset prevents it from being reloaded while there are still active handles. Doing so will result in a panic, because the storage entry has been marked as "empty / None" but the ID is still assumed to be active by the asset server.

Patterns like images.remove() -> asset_server.reload() and images.remove() -> images.insert() would fail if the handle was still alive.

Solution

Most of the groundwork for this was already laid in Bevy Asset V2. This is largely just a matter of splitting out remove into two separate operations:

  • remove_dropped: remove the stored asset, invalidate the internal Assets entry (preventing future insertions with the old id), and recycle the id
  • remove_still_alive: remove the stored asset, but leave the entry otherwise untouched (and dont recycle the id).

remove_still_alive and insert can be called any number of times (in any order) for an id until remove_dropped has been called, which will invalidate the id.

From a user-facing perspective, there are no API changes and this is non breaking. The public Assets::remove will internally call remove_still_alive. remove_dropped can only be called by the internal "handle management" system.


Changelog

  • Fix a bug preventing Assets::remove from blocking future inserts for a specific AssetIndex.

@cart cart added the A-Assets Load files from disk to use for things like images, models, and sounds label Nov 28, 2023
@cart cart added this to the 0.12.1 milestone Nov 28, 2023
@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Nov 28, 2023
@JMS55 JMS55 self-requested a review November 28, 2023 22:28
@JMS55
Copy link
Contributor

JMS55 commented Nov 29, 2023

I think there's still the existing issue where the last strong handle drop will infinitely loop for assets that have already been removed, as contains() will always return false, I believe. I think we need to instead query the AssetServer info for this check. I did that in this PR: #10520

if !assets.contains(id.typed()) {
not_ready.push(drop_event);
continue;
}

@cart
Copy link
Member Author

cart commented Nov 29, 2023

@JMS55 good call / agreed. Just pushed a fix.

@cart cart added this pull request to the merge queue Nov 29, 2023
Merged via the queue into bevyengine:main with commit 0a588db Nov 29, 2023
22 checks passed
cart added a commit that referenced this pull request Nov 30, 2023
# Objective

Fixes #10444 

Currently manually removing an asset prevents it from being reloaded
while there are still active handles. Doing so will result in a panic,
because the storage entry has been marked as "empty / None" but the ID
is still assumed to be active by the asset server.

Patterns like `images.remove() -> asset_server.reload()` and
`images.remove() -> images.insert()` would fail if the handle was still
alive.

## Solution

Most of the groundwork for this was already laid in Bevy Asset V2. This
is largely just a matter of splitting out `remove` into two separate
operations:

* `remove_dropped`: remove the stored asset, invalidate the internal
Assets entry (preventing future insertions with the old id), and recycle
the id
* `remove_still_alive`: remove the stored asset, but leave the entry
otherwise untouched (and dont recycle the id).

`remove_still_alive` and `insert` can be called any number of times (in
any order) for an id until `remove_dropped` has been called, which will
invalidate the id.

From a user-facing perspective, there are no API changes and this is non
breaking. The public `Assets::remove` will internally call
`remove_still_alive`. `remove_dropped` can only be called by the
internal "handle management" system.

---

## Changelog

- Fix a bug preventing `Assets::remove` from blocking future inserts for
a specific `AssetIndex`.
james7132 pushed a commit to james7132/bevy that referenced this pull request Dec 1, 2023
# Objective

Fixes bevyengine#10444 

Currently manually removing an asset prevents it from being reloaded
while there are still active handles. Doing so will result in a panic,
because the storage entry has been marked as "empty / None" but the ID
is still assumed to be active by the asset server.

Patterns like `images.remove() -> asset_server.reload()` and
`images.remove() -> images.insert()` would fail if the handle was still
alive.

## Solution

Most of the groundwork for this was already laid in Bevy Asset V2. This
is largely just a matter of splitting out `remove` into two separate
operations:

* `remove_dropped`: remove the stored asset, invalidate the internal
Assets entry (preventing future insertions with the old id), and recycle
the id
* `remove_still_alive`: remove the stored asset, but leave the entry
otherwise untouched (and dont recycle the id).

`remove_still_alive` and `insert` can be called any number of times (in
any order) for an id until `remove_dropped` has been called, which will
invalidate the id.

From a user-facing perspective, there are no API changes and this is non
breaking. The public `Assets::remove` will internally call
`remove_still_alive`. `remove_dropped` can only be called by the
internal "handle management" system.

---

## Changelog

- Fix a bug preventing `Assets::remove` from blocking future inserts for
a specific `AssetIndex`.
@asafigan asafigan mentioned this pull request Dec 2, 2023
4 tasks
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DenseAssetStorage#insert "unreachable" panic
3 participants