-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
AssetSaver and AssetTransformer split #11260
AssetSaver and AssetTransformer split #11260
Conversation
…dSave. Updated Example.
In updating the implementation of |
Why not Load -> Transform (Image -> Image, but compressed, or maybe a separate asset type) -> Save? |
The So to go with that implementation it would mean that a user would have to add useless asset types to their app, which wouldn't be ergonomic. Additionally, |
Is this ready to review, or needs more time? |
I think at this point it's ready to review. I can't think of anything else that needs to be done or added. |
/// This uses [`LoadTransformAndSaveSettings`] to configure the processor. | ||
/// | ||
/// [`Asset`]: crate::Asset | ||
pub struct LoadTransformAndSave< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not have to constrain the loader type, or the transformer output here? Huh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following. Does:
LoadTransformAndSave<L: AssetLoader, T: AssetTransformer<AssetInput = L::Asset>, S: AssetSaver<Asset = T::AssetOutput>>
need further constraints?
L
can be any AssetLoader
, T
can be any AssetTransformer
that accepts L
's output as an input, and S
can by any AssetSaver
that accepts T
's output as an input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a second look I guess it makes sense. I'm a bit surprised we don't have to specify e.g. the AssetTransformer output type, but it does make sense as is.
This split makes substantially more sense to me, and the code quality is high. It's even used in an example :D |
# Objective One of a few Bevy Asset improvements I would like to make: bevyengine#11216. Currently asset processing and asset saving are handled by the same trait, `AssetSaver`. This makes it difficult to reuse saving implementations and impossible to have a single "universal" saver for a given asset type. ## Solution This PR splits off the processing portion of `AssetSaver` into `AssetTransformer`, which is responsible for transforming assets. This change involves adding the `LoadTransformAndSave` processor, which utilizes the new API. The `LoadAndSave` still exists since it remains useful in situations where no "transformation" of the asset is done, such as when compressing assets. ## Notes: As an aside, Bikeshedding is welcome on the names. I'm not entirely convinced by `AssetTransformer`, which was chosen mostly because `AssetProcessor` is taken. Additionally, `LoadTransformSave` may be sufficient instead of `LoadTransformAndSave`. --- ## Changelog ### Added - `AssetTransformer` which is responsible for transforming Assets. - `LoadTransformAndSave`, a `Process` implementation. ### Changed - Changed `AssetSaver`'s responsibilities from processing and saving to just saving. - Updated `asset_processing` example to use new API. - Old asset .meta files regenerated with new processor.
Objective
One of a few Bevy Asset improvements I would like to make: #11216.
Currently asset processing and asset saving are handled by the same trait,
AssetSaver
. This makes it difficult to reuse saving implementations and impossible to have a single "universal" saver for a given asset type.Solution
This PR splits off the processing portion of
AssetSaver
intoAssetTransformer
, which is responsible for transforming assets. This change involves adding theLoadTransformAndSave
processor, which utilizes the new API. TheLoadAndSave
still exists since it remains useful in situations where no "transformation" of the asset is done, such as when compressing assets.Notes:
As an aside, Bikeshedding is welcome on the names. I'm not entirely convinced by
AssetTransformer
, which was chosen mostly becauseAssetProcessor
is taken. Additionally,LoadTransformSave
may be sufficient instead ofLoadTransformAndSave
.Changelog
Added
AssetTransformer
which is responsible for transforming Assets.LoadTransformAndSave
, aProcess
implementation.Changed
AssetSaver
's responsibilities from processing and saving to just saving.asset_processing
example to use new API.