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

Bevy Asset Saving and Improvements #11216

Open
1 of 3 tasks
thepackett opened this issue Jan 4, 2024 · 7 comments
Open
1 of 3 tasks

Bevy Asset Saving and Improvements #11216

thepackett opened this issue Jan 4, 2024 · 7 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@thepackett
Copy link
Contributor

thepackett commented Jan 4, 2024

What problem does this solve or what need does it fill?

Bevy Asset is currently designed around loading and processing assets, however with just a little bit of work it could be improved with additional functionality such as saving assets. This issue is to track my efforts in accomplishing this.

I have three goals:

  • Split AssetSaver into AssetSaver and AssetTransformer
  • Add saving functionality to the AssetServer
  • Add manual Asset processing

What solution would you like?

Split AssetSaver into AssetSaver and AssetTransformer

AssetSaver currently holds two responsibilities: processing assets and then converting them to bytes for an AssetWriter to save. By splitting it into two parts we can reduce code duplication and make the asset API more ergonomic.

This change will require modifying LoadAndSave<L: AssetLoader, S: AssetSaver> to something like:

LoadTransformAndSave<L: AssetLoader, T: AssetTransformer<Input = L::Asset>, S: AssetSaver<Asset = T::Output>>

Although this is technically a breaking change, it has a very limited surface area (just LoadAndSave and any custom implementations of AssetSaver), and a very easy migration (use LoadTransformAndSave and split your AssetSaver implementation into its two parts), so I believe that it is worth it to enable ergonomic saving and improve the Bevy Asset data model.

Note: I'm note entirely set on the name AssetTransformer, but AssetProcessor is taken. Bikeshedding is welcome on this.

Add saving functionality to the AssetServer

I would like to modify the AssetServer to expose an API such as:

asset_server.save(path: Path, asset: Asset)

With events to indicate saving progress.

Add manual Asset processing

Although the current AssetProcessor usage of preprocessing assets is preferable for the vast majority of use cases, it would still be handy to be able to manually process an asset from time to time. For example, transforming a DynamicScene to prep it for saving, or processing something once-off. I'm currently undecided about how to fit this into the API (AssetServer? AssetProcessor? Manually calling something like AssetTransformer::transform()?), but I will consider this further as I complete the first two tasks.

@thepackett thepackett added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 4, 2024
@alice-i-cecile alice-i-cecile 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 and removed S-Needs-Triage This issue needs to be labelled labels Jan 4, 2024
@alice-i-cecile
Copy link
Member

Agreed on all three of these: the split makes sense, and the additional functionality is clearly useful. Do try to add examples as you work: we're quite low on asset examples, especially for non-trivial workflows.

github-merge-queue bot pushed a commit that referenced this issue Jan 26, 2024
# 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.
tjamaan pushed a commit to tjamaan/bevy that referenced this issue Feb 6, 2024
# 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.
@andriyDev
Copy link
Contributor

First, I think we can call the first checkbox finished.

Second, we need to resolve #11595.

Third (the big one), I think we've sort of designed ourselves into a corner.

  • AssetSavers need the sub-assets of an asset to be saved (required).
  • Assets are stored by value in the Assets struct (whether in dense_storage or hash_map).
  • Assets are not Clone.
  • We ideally want asset saving to be able to happen in the background as an async task.

So we either must:

  1. make saving synchronous - now we can just take references to the sub-assets with no* problem.
  2. make Assets be Clone - cloning can be expensive and is quite sad for assets that aren't mutated, but now the cloned sub-assets can be passed to the AssetSaver directly.
  3. store assets in an Arc within Assets - assets are no longer densely stored.

Each of these comes with significant tradeoffs and pretty much all of these are in tension with #11266. I'm not sure what people are willing to give up!

@mrtracy
Copy link
Contributor

mrtracy commented Mar 16, 2024

I would like to comment on the theoretical interface proposed for saving above:

asset_server.save(path: Path, asset: Asset)

The Asset type passed above would actually need to be a SavedAsset<T> or equivalent: a structure which contains both the primary asset and all of its labeled sub-assets. There isn't much difference for singular assets with a well-known file type, but this is crucial for more complicated operations such as:

  • Container types (GLTF)
  • Saving with a specific compression format

The reasons for this are all on display with the GLTF loader (and I assume with any upcoming 'scene' loader as well). Firstly, the contents of the GLTF file are "flattened" by the loader: the meshes are placed individually into Assets<mesh>, the nodes are placed into a Assets<GltfNode> collection, etc. Perhaps even more interesting is that sub-assets are converted to a bevy specific types - mesh is one example, but the materials are also converted directly to bevy_pbr's StandardMaterial.

In order to reverse this process for saving, following things must be assumed

  1. When saving a container, there must be a "gather" step which assembles the sub-assets from their various collections
  2. There may be a significant format transformation involved before saving.

Therefore, I would suggest a two-step process for asset saving:

  1. A synchronous 'gather_from_world()' step which has read-only access to World. This step would be in charge of assembling the bytes to save - by cloning, copying, serializing, etc. The output of this would be a SavedAsset<T> (or equivalent), with data for the asset and all of its sub-assets.
  2. The resulting SavedAsset<T> is passed to asset_server for async I/O.

I think this would suffice for everything other than, possibly, extremely large assets. However, I would personally suggest that large archives (or stream-optimized DBs) would be better modeled as an asset source, not an asset with sub-objects.

@mrtracy
Copy link
Contributor

mrtracy commented Mar 16, 2024

Another challenge for the Saving interaction is managing the path-to-asset mapping in the server. For the purpose of general asset sharing and live-reloading, AssetServer maintains a mapping of source paths to handles. This only lives in the asset server, not in the Assets collections proper; this means that assets created at runtime do not have such a mapping.

A strategy will be needed for handling situations such as the following:

  • A completely new asset saved for the first time, meaning it now has a disk representation.
  • A container asset is loaded from disk, and then before saving:
    • Additional sub-assets are added
    • An existing sub-asset is removed
  • An existing asset is saved to a new file location.
  • An asset which was loaded from disk and has been modified, but the underlying disk asset is modified separately and no longer matches.

@andriyDev
Copy link
Contributor

So first, Arc-ing assets doesn't work - that means assets in Assets would not be safely mutable. So we can scratch that suggestion of mine.

Another problem is figuring out what "counts" as a subasset. If we want to do something like @mrtracy's "gather" step, we need to be able to figure out which assets to gather in the first place. We could have a VisitSubassets trait to mirror VisitDependencies. That would still require assets to be cloned.

We could also just do saving in an exclusive system to serialize all the bytes, and then write them to disk in an async task. Saving seems like a less common operation than loading so optimizing for it might not matter. Then again, someone might want to make save data for a user's game an asset, or have user generated content as an asset, which they both might want to be saved and in a timely fashion.

@andriyDev
Copy link
Contributor

I wanted to bring up a suggestion I read in the Bevy Discord: asset locking. The idea is that an asset can be locked (moving it from asset dense storage into an Arc), at which point you are given an Arc (or similar) to it. This Arc can then be passed in to an asset saver which can do the actual saving. At some point in the future, if all the Arcs have been dropped, the asset can be unlocked, which returns the asset back to dense storage.

Direct advantages of this approach:

  1. Assets remain in dense storage by default.
  2. Assets can be taken as an Arc to be passed into whatever thread wants it.
  3. Asset saving can just operate on a bunch of Arcs to provide references to the subassets.
  4. We don't need an exclusive system to do serialization, and we don't need to block Bevy entirely when trying to save an asset.

Some disadvantages of this:

  1. Asset storage is a little more complicated.
  2. We need to figure out when it is safe to unlock an asset.
  3. Getting mutable access to an asset has a new failure case (you can't mutate an asset if it is locked).

@andriyDev
Copy link
Contributor

I also think this could also reduce overhead with asset loading/processing. Currently if you have "load dependencies", as far as I can tell, the assets must be loaded from scratch. This means even if the load dependency is already loaded, the asset has to be reloaded. I think we could have the asset loader send a message to the asset server through a channel asking it to lock an asset and returning its Arc back to the loader once that completes. This should allow us to dedup the loading code between AssetServer and DirectNestedLoader

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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

4 participants