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

Fix AssetTransformer breaking LabeledAssets #11626

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Fix AssetTransformer breaking LabeledAssets #11626

merged 5 commits into from
Feb 2, 2024

Conversation

RyanSpaker
Copy link
Contributor

@RyanSpaker RyanSpaker commented Jan 31, 2024

Objective

  • AssetTransformer provides an input asset, and output an asset, but provides no access to the LabeledAsset's created by the AssetLoader. Labeled sub assets are an extremely important piece of many assets, Gltf in particular, and without them the amount of transformation on an asset is limited. In order for AssetTransformer's to be useful, they need to have access to these sub assets.
  • LabeledAsset's loaded by AssetLoaders are provided to AssetSavers in the LoadAndSave process, but the LoadTransformAndSave process drops these values in the transform stage, and so AssetSaver is given none.
  • Fixes LoadTransformAndSave Drops Asset Dependencies #11606

Ideally the AssetTransformer should not ignore labeled sub assets, and they should be kept at least for the AssetSaver

Solution

  • I created a new struct similar to SavedAsset named TransformedAsset which holds the input asset, and the HashMap of LabeledAssets. The transform function now takes as input a TransformedAsset, and returns a TransformedAsset::<AssetOutput>. This gives the transform function access to the labeled sub assets created by the AssetLoader.
  • I also created TransformedSubAsset which holds mutable references to a sub asset and that sub assets HashMap of LabeledAssets. This allows you to travers the Tree of LabeledAssets by reference relatively easily.
  • The LoadTransformAndSave processor was then reworked to use the new structs, stopping the LabeledAssets from being dropped.

Changelog

  • Created TransformedAsset struct and TransformedSubAsset struct.
  • Changed get_untyped_handle to return a UntypedHandle directly rather than a reference and added get_handle as a typed variant in SavedAsset and TransformedAsset
  • Added SavedAsset::from_transformed as a constructor from a TransformedAsset
  • Switched LoadTransformAndSave process code to work with new TransformedAsset type
  • Added a ProcessError for AssetTransformer in process.rs
  • Switched AssetTransformer::transform to use TransformedAsset as input and output.
  • Switched AssetTransformer to use a BoxedFuture like AssetLoader and AssetSaver to allow for async transformation code.
  • Updated AssetTransformer example to use new structure.

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 31, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Jan 31, 2024
@alice-i-cecile
Copy link
Member

@thepackett, could I get your review?

Copy link
Contributor

@thepackett thepackett left a comment

Choose a reason for hiding this comment

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

I completely overlooked labeled assets when I initially implemented asset transformers. Thanks for spotting and fixing this!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 2, 2024
@alice-i-cecile
Copy link
Member

Thank you very much for catching and unbreaking this. I'd still like more examples in the assets folder, both for learning and testing, but that can wait for follow-up PRs.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 2, 2024
Merged via the queue into bevyengine:main with commit 8866c61 Feb 2, 2024
24 checks passed
@RyanSpaker RyanSpaker deleted the fix_transform branch February 2, 2024 18:11
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

- `AssetTransformer` provides an input asset, and output an asset, but
provides no access to the `LabeledAsset`'s created by the `AssetLoader`.
Labeled sub assets are an extremely important piece of many assets, Gltf
in particular, and without them the amount of transformation on an asset
is limited. In order for `AssetTransformer`'s to be useful, they need to
have access to these sub assets.
- LabeledAsset's loaded by `AssetLoader`s are provided to `AssetSaver`s
in the `LoadAndSave` process, but the `LoadTransformAndSave` process
drops these values in the transform stage, and so `AssetSaver` is given
none.
- Fixes bevyengine#11606

Ideally the AssetTransformer should not ignore labeled sub assets, and
they should be kept at least for the AssetSaver

## Solution

- I created a new struct similar to `SavedAsset` named
`TransformedAsset` which holds the input asset, and the HashMap of
`LabeledAsset`s. The transform function now takes as input a
`TransformedAsset`, and returns a `TransformedAsset::<AssetOutput>`.
This gives the transform function access to the labeled sub assets
created by the `AssetLoader`.
- I also created `TransformedSubAsset` which holds mutable references to
a sub asset and that sub assets HashMap of `LabeledAsset`s. This allows
you to travers the Tree of `LabeledAsset`s by reference relatively
easily.
- The `LoadTransformAndSave` processor was then reworked to use the new
structs, stopping the `LabeledAsset`s from being dropped.

---

## Changelog

- Created TransformedAsset struct and TransformedSubAsset struct.
- Changed `get_untyped_handle` to return a `UntypedHandle` directly
rather than a reference and added `get_handle` as a typed variant in
SavedAsset and TransformedAsset
- Added `SavedAsset::from_transformed` as a constructor from a
`TransformedAsset`
- Switched LoadTransformAndSave process code to work with new
`TransformedAsset` type
- Added a `ProcessError` for `AssetTransformer` in process.rs
- Switched `AssetTransformer::transform` to use `TransformedAsset` as
input and output.
- Switched `AssetTransformer` to use a `BoxedFuture` like `AssetLoader`
and `AssetSaver` to allow for async transformation code.
- Updated AssetTransformer example to use new structure.
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 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
Development

Successfully merging this pull request may close these issues.

LoadTransformAndSave Drops Asset Dependencies
3 participants