Skip to content

Remove labeled_assets from LoadedAsset and ErasedLoadedAsset #15481

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Feb 10, 2025
Merged

Remove labeled_assets from LoadedAsset and ErasedLoadedAsset #15481

merged 11 commits into from
Feb 10, 2025

Conversation

@andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Sep 27, 2024

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

andriyDev commented Sep 27, 2024

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

andriyDev commented Oct 2, 2024

Rebased after #15509. Nothing really affected.

BenjaminBrienen
BenjaminBrienen approved these changes Oct 31, 2024
View reviewed changes
@andriyDev
Copy link
Contributor Author

andriyDev commented Oct 31, 2024 •
edited
Loading

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

@andriyDev
Copy link
Contributor Author

andriyDev commented Dec 14, 2024

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".

@andriyDev andriyDev force-pushed the simple-erased branch 2 times, most recently from a07a40e to 69d600e Compare December 29, 2024 20:24
@andriyDev andriyDev force-pushed the simple-erased branch 2 times, most recently from f930f32 to d042534 Compare January 15, 2025 03:34
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@andriyDev
Separate out labeled_assets from LoadedAsset into its own type.
ad79fab 
This makes it clear that loaded assets are not recursive.
@andriyDev
Update loader_builder to return complete assets.
7d1ad6f
@andriyDev
Simplify the TransformedAsset API to only support subassets in the …
56afbb4 
…"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.
@andriyDev
Simplify the SavedAsset API to only support subassets in the "root"…
1ad5f0b 
… asset.
@andriyDev
Update process to use complete assets.
560bd40
@andriyDev
Update the lib test to correctly handle the complete assets.
3817365
@andriyDev
Rename some variables to mention "complete assets".
880a533 
This makes it much clearer when we access `complete_asset.asset` as opposed to `asset.asset` or `loaded_asset.asset`.
@andriyDev
Add take functions to CompleteLoadedAsset and `CompleteErasedLoaded…
170ee86 
…Asset`.
bushrat011899
bushrat011899 approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable change to me. Separating out concerns like this makes the total asset pipeline larger to comprehend, but overall simpler.

Nathan-Fenner
Nathan-Fenner approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still mostly a bevy internals newbie, but I have been staring at the Asset code since yesterday and suspected that exactly this kind of refactor was possible. This looks like a bit improvement in clarity to me.

@chescock chescock added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 7, 2025
@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 10, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 10, 2025

I've taken a look at this, and I'm relatively comfortable with what's going on here. I'd really like to see more tests and examples for assets to increase confidence in these sorts of changes, but the motivation here is really strong and I find this new model simpler to understand. Merging.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 10, 2025
Merged via the queue into bevyengine:main with commit f176448 Feb 10, 2025
34 checks passed
@andriyDev andriyDev deleted the simple-erased branch February 10, 2025 21:25
@pcwalton pcwalton mentioned this pull request Feb 24, 2025
Closed
@cart cart mentioned this pull request Mar 17, 2025
Closed
andriyDev added a commit to andriyDev/bevy that referenced this pull request Mar 27, 2025
@andriyDev
Revert "Remove labeled_assets from LoadedAsset and `ErasedLoadedA…
c6be275 
…sset` (bevyengine#15481)"

This reverts commit f176448.
@andriyDev andriyDev mentioned this pull request Mar 27, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 27, 2025
@andriyDev
Revert PR #15481 to resolve a regression. (#18567)
a08760b 
# Objective

- Fixes #18010.

## Solution

- Revert the offending PRs! These are #15481 and #18013. We now no
longer get an error if there are duplicate subassets.
- In theory we could untangle #18013 from #15481, but that may be
tricky, and may still introduce regressions. To avoid this worry (since
we're already in RC mode), I am just reverting both.

## Testing

- This is just a revert.

---

## Migration Guide

<Remove the migration guides for #15481 and #18013>

I will make a PR to the bevy_website repo after this is merged.
mockersf pushed a commit that referenced this pull request Mar 27, 2025
@andriyDev @mockersf
Revert PR #15481 to resolve a regression. (#18567)
fe3656f 
- Fixes #18010.

- Revert the offending PRs! These are #15481 and #18013. We now no
longer get an error if there are duplicate subassets.
- In theory we could untangle #18013 from #15481, but that may be
tricky, and may still introduce regressions. To avoid this worry (since
we're already in RC mode), I am just reverting both.

- This is just a revert.

---

<Remove the migration guides for #15481 and #18013>

I will make a PR to the bevy_website repo after this is merged.
@andriyDev andriyDev mentioned this pull request May 2, 2025
Open
@andriyDev andriyDev mentioned this pull request Jun 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@bushrat011899 bushrat011899 bushrat011899 approved these changes

@Nathan-Fenner Nathan-Fenner Nathan-Fenner approved these changes

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Subassets of subassets do not work.

6 participants

@andriyDev @alice-i-cecile @bushrat011899 @Nathan-Fenner @BenjaminBrienen @chescock