bevy icon indicating copy to clipboard operation
bevy copied to clipboard

AssetSaver and AssetTransformer split

Open thepackett opened this issue 2 years ago • 5 comments

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

thepackett avatar Jan 08 '24 21:01 thepackett

In updating the implementation of CompressedImageSaver after my change, I realized that I overlooked one of the uses of AssetSaver: compressing assets. This means that LoadAndSave is still useful in situations where there is no asset manipulation, so I have re-added it.

thepackett avatar Jan 09 '24 00:01 thepackett

Why not Load -> Transform (Image -> Image, but compressed, or maybe a separate asset type) -> Save?

JMS55 avatar Jan 09 '24 00:01 JMS55

Why not Load -> Transform (Image -> Image, but compressed, or maybe a separate asset type) -> Save?

The AssetTransformer trait as I implemented it is about transforming from one asset type to another (or to itself, with changes). Nothing is stopping a user from implementing a compressed asset that way (as its own asset type that you transform to), however I suspect for most use cases it would be undesirable. Most things like a compressed image don't have any uses as an Asset type. Usually the only "useful" representation is an array of bytes which can be later decompressed into a useful asset.

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, AssetSaver's job it to output the bytes of an asset that needs to be saved, so stuff like compression fits pretty naturally there.

thepackett avatar Jan 09 '24 01:01 thepackett

Is this ready to review, or needs more time?

JMS55 avatar Jan 09 '24 02:01 JMS55

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.

thepackett avatar Jan 09 '24 14:01 thepackett

This split makes substantially more sense to me, and the code quality is high. It's even used in an example :D

alice-i-cecile avatar Jan 26 '24 20:01 alice-i-cecile