-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Implement minimal asset saving #22622
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
base: main
Are you sure you want to change the base?
Conversation
b8953aa to
6f9d0e6
Compare
6f9d0e6 to
bc5703b
Compare
bc5703b to
1d215e4
Compare
1d215e4 to
77a14a9
Compare
| let asset_server = app.world().resource::<AssetServer>().clone(); | ||
| let mut saved_asset_builder = | ||
| SavedAssetBuilder::new(asset_server.clone(), "some/target/path.cool.ron".into()); | ||
| saved_asset_builder.add_labeled_asset_with_existing_handle( |
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.
So what happens if we're wrong about whether or not we already have a handle?
EG, say we don't realize this asset is already saved, and we save over it with a new labeled asset sans a handle.
What breaks, and does it break predictably?
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.
It's clear what "existing" means is not quite clear. "Existing" in this context means that your root asset already contains a handle, whose asset you want to include in the saved asset as a subasset.
So if you save multiple times, you can keep calling add_labeled_asset_with_new_handle. It's just "more work" to do so with add_labeled_asset_with_new_handle since you need to store that handle in the root asset.
So like, the wrong thing to do would be to do something like:
- Clone an asset that is stored in
Assets. - For each of its subassets, you call
add_labeled_asset_with_new_handle- and then do nothing with the handle. - Call save.
This is wrong because now the asset that you cloned out of Assets will contain handles that don't have a corresponding subasset in the SavedAsset - the handles won't match. So from the perspective of the AssetSaver it will see handles that don't match what they get when they call get_handle(subasset_label). In a future PR, we should also have a get_by_handle - which will just return you None or something. The correct thing to do in this example is in step 2 to either use add_labeled_asset_with_existing_handle, or you could call add_labeled_asset_with_new_handle and store that handle in the root asset.
Btw, a missing handle is not necessarily a mistake - for example, if you have a StandardMaterial, the handles it stores are most likely just references to other files. In other words, if you accidentally use add_labeled_asset_with_new_handle and then don't store that handle, the AssetSaver may interpret that handle as a nested-loaded asset, and just serialize its path.
Edit: Added more documentation to explain when to use either function.
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.
Actually even that is a little wrong, since it's possible for an Asset to not hold handles to its subassets - the AssetSaver can still save these by iterating through the labeled assets in the SavedAsset.
But in the common case, the root asset stores handles, so we should optimize for that.
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.
Well, that's part of my concern, and this seems like a good way to ensure that data passed through the save operation to a given subpath is the data that later gets referenced during load for a given subpath.
I am also concerned with data and handles that might have existed BEFORE that save operation though.
Consider the following:
- Asset exists with subassets in their respective asset stores. Someone calls
asset_server.load("a_path.asset#subassetpath"). They store that handle somewhere, on an entity. - Someone later saves an asset and associated subassets using
add_labeled_asset_with_new_handle. - Also, someone now, a second time, after the asset has been saved, calls
asset_server.load("a_path.asset#subassetpath")
This presents two questions:
- As of step two, does the reference pointed to by the handle in step 1 now contain new data?
- Are the handles returned in step 1 and step 3 clones of the same handle?
I -think- that the answer to these questions is "yes," but we should assert this behavior under test
greeble-dev
left a comment
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 clicking approve as the PR does what it says and is backed up by tests and an example. I left a few minor suggestions.
I do have some concerns with the overall direction:
- The code is significantly complicated by having to support handles and avoid copies. I'm not sure this complexity is worth it right now.
- We're not dogfooding since the engine doesn't have a complicated saver built-in - the example code is fairly contrived. This makes it hard to predict if the API will work well.
- My guess is that the subtleties around sub-assets and when to use
add_labeled_asset_with_new_handleversusadd_labeled_asset_with_existing_handlewill be challenging for users.
I don't think these concerns are blocking, but maybe they deserve more discussion. And as ever, there's the possibility that assets-as-entities ends up remaking the whole thing.
| CowArc<'static, str>: Borrow<Q>, | ||
| Q: ?Sized + Hash + Eq, | ||
| { | ||
| pub fn get_labeled<B: Asset>(&self, label: &str) -> Option<SavedAsset<'a, '_, B>> { |
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.
This change means get_labeled can no longer be called directly with a CowArc<str>. Only a minor regression, but is it necessary? I guess the lifetimes might be tricky.
| @@ -0,0 +1,372 @@ | |||
| //! This example demonstrates how to save assets. | |||
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.
Maybe worth calling the example asset_saving_subassets.rs? Or subasset_saving.rs? It's a fairly complicated example, so this would leave space for a simpler example that doesn't involve sub-assets.
|
|
||
| ## 1. Building the `SavedAsset` | ||
|
|
||
| To build the `SavedAsset`, either use `SavedAsset::from_asset`, or `SavedAssetBuilder`. For example: |
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 think this could do with a brief example of SavedAsset::from_asset that's separate from the SavedAssetBuilder example? Would make clear that SavedAssetBuilder is only needed for more complicated cases.
| // NOTE: We can't handle embedded dependencies in any way, since we need to write to | ||
| // another file to do so. | ||
| embedded_dependencies: vec![], | ||
| }; |
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.
| // NOTE: We can't handle embedded dependencies in any way, since we need to write to | |
| // another file to do so. | |
| embedded_dependencies: vec![], | |
| }; | |
| embedded_dependencies: vec![], | |
| }; | |
| // NOTE: We can't handle embedded dependencies in any way, since we need to write to | |
| // another file to do so. | |
| assert!(asset.embedded_dependencies.is_empty()); |
Assert seems safer if this is only used in tests.
| .web-asset-cache | ||
| examples/large_scenes/bistro/assets/* | ||
| examples/large_scenes/caldera_hotel/assets/* | ||
| examples/asset/saved_assets |
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 sure what this is for? The example asset+meta is already tracked by git, and the example doesn't add new ones.
Objective
Solution
SavedAssetinstances by building up their labeled assets. This can either take an existing handle + asset ref, or will create a handle for your asset ref.AssetSaver, and write out the correct meta file.Some things I am leaving to future PRs
AssetSaver.Weird implementation details
Moo), since I needed to store just a ref or owned hashmap. I wrote a lengthy comment explaining why it's needed (TL;DR variance is complicated, Cow doesn't work). Note: it's possible we could just use CowArc instead, but we never need to clone the map so this seems like overkill. Also having a nice place to explain the variance problems is useful here.add_labeled_asset_with_*<'b: 'a>, since otherwise, theCowArcgets coerced to'staticwhich means the lifetime of the subasset needs to be'static, which practically makes this unusable. By adding a second lifetime dedicated to theCowArc,Testing