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

Remove labeled_assets from LoadedAsset and ErasedLoadedAsset #15481

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

Objective

Fixes #15417.

Solution

  • Remove the labeled_assets fields from LoadedAsset and ErasedLoadedAsset.
  • Created new structs CompleteLoadedAsset and CompleteErasedLoadedAsset to hold the labeled_subassets.
  • When a subasset is LoadContext::finished, it produces a CompleteLoadedAsset.
  • When a CompleteLoadedAsset is added to a LoadContext (as a subasset), their labeled_assets are merged, reporting any overlaps.

One important detail to note: nested subassets with overlapping names could in theory have been used in the past for the purposes of asset preprocessing. Even though there was no way to access these "shadowed" nested subassets, asset preprocessing does get access to these nested subassets. This does not seem like a case we should support though. It is confusing at best.

Testing

  • This is just a refactor.

Migration Guide

  • Most uses of LoadedAsset and ErasedLoadedAsset should be replaced with CompleteLoadedAsset and CompleteErasedLoadedAsset respectively.

@andriyDev andriyDev changed the title Remove labeled_assets from Remove labeled_assets from LoadedAsset and ErasedLoadedAsset Sep 27, 2024
@andriyDev andriyDev force-pushed the simple-erased branch 2 times, most recently from ce7c284 to 8cc63f6 Compare September 27, 2024 21:27
@andriyDev
Copy link
Contributor Author

Just to make it clear what the rationale for this is. This makes implementing asset saving easier, since we don't need to deal with recursive types. We just have to borrow the root assets, and borrow the subassets and we're done. Without this PR, for each subasset, we'd need to create a new type like SubAssetHolder that holds the borrow to the subasset, and a Vec of SubAssetHolder which in turn hold borrows to the nested subassets (and so on and so on).

@BenjaminBrienen BenjaminBrienen added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 27, 2024
@andriyDev
Copy link
Contributor Author

Rebased after #15509. Nothing really affected.

crates/bevy_asset/src/loader.rs Outdated Show resolved Hide resolved
@andriyDev
Copy link
Contributor Author

andriyDev commented Oct 31, 2024

Rebased since this PR was a little stale. I folded in the accidental doc comment fix into the first commit.

This makes it clear that loaded assets are not recursive.
…"root" asset.

Now that subassets can't have nested subassets, we can completely remove the `TransformedSubasset` type and just return a mutable borrow. It is now up to the asset to know how to link up the subassets.
This makes it much clearer when we access `complete_asset.asset` as opposed to `asset.asset` or `loaded_asset.asset`.
@andriyDev
Copy link
Contributor Author

Rebase after #15487 was merged, fairly trivial overall. This also had the funny side-effect that I could remove an expect lint for "large error result type", since apparently removing the meta field was enough to consider the struct "small".

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subassets of subassets do not work.
2 participants