Replace all different load variants in AssetServer with a builder.#23663
Replace all different load variants in AssetServer with a builder.#23663andriyDev wants to merge 6 commits intobevyengine:mainfrom
AssetServer with a builder.#23663Conversation
greeble-dev
left a comment
There was a problem hiding this comment.
Looks good, just got some nitpicky comments.
Also thinking that load_builder could be bikeshedded as it's a teeny bit verbose? LoadContext uses loader. Maybe build is an option? I suspect both of these end up too ambiguous though.
| @@ -0,0 +1,21 @@ | |||
| --- | |||
| title: Advanced AssetServer load variants are now expose through a builder pattern. | |||
There was a problem hiding this comment.
| title: Advanced AssetServer load variants are now expose through a builder pattern. | |
| title: Advanced AssetServer load variants are now exposed through a builder pattern. |
| Every load variant above can be reimplemented using `load_builder`, and each one of these methods | ||
| has deprecation messages on them explaining their new equivalent. |
There was a problem hiding this comment.
| Every load variant above can be reimplemented using `load_builder`, and each one of these methods | |
| has deprecation messages on them explaining their new equivalent. | |
| Every load variant above can be reimplemented using `load_builder`, and each one of these methods | |
| has deprecation messages on them explaining their new equivalent. | |
| For example, `load_with_settings_override` can be replaced by: | |
| ```rust | |
| asset_server | |
| .load_builder() | |
| .with_settings(settings) | |
| .override_unapproved() | |
| .load(path) |
I thought one example here might be nice so the user gets the flavor of things.
| #[must_use = "the load doesn't start until LoadBuilder has been consumed"] | ||
| pub fn load_builder(&self) -> LoadBuilder<'_> { | ||
| LoadBuilder::new(self) | ||
| } |
There was a problem hiding this comment.
Missing a comment? Even if it's as simple as "see [`LoadBuilder`]".
| #[must_use = "not using the returned strong handle may result in the unexpected release of the asset"] | ||
| pub fn load<'a, A: Asset>(&self, path: impl Into<AssetPath<'a>>) -> Handle<A> { | ||
| self.load_with_meta_transform(path, None, (), false) | ||
| self.load_builder().load(path.into()) |
There was a problem hiding this comment.
| self.load_builder().load(path.into()) | |
| self.load_builder().load(path) |
I don't think the into is necessary? Unless there's an optimization reason?
| #[deprecated(note = "Use `asset_server.load_builder().load_erased(type_id, path)` instead")] | ||
| pub fn load_erased<'a>( |
There was a problem hiding this comment.
Maybe load_erased and load_untyped shouldn't be deprecated? They're convenient and the duplication is modest.
| } | ||
| } | ||
|
|
||
| /// A builder for initiating a more complex load than the one provided by [`AssetServer::load`]. |
There was a problem hiding this comment.
Maybe worth adding one example here for flavor?
// For example, to load an asset with overridden settings and path approval:
asset_server
.load_builder()
.with_settings(settings)
.override_unapproved()
.load(path)
Objective
load_folder).load_erased- but it unfortunately doesn't support all the features that the other variants support.load_acquire_with_settings_overridewhich 1) loads the asset, 2) uses the settings provided, 3) allows reading unapproved asset paths, and 4) drops a guard once the load completes.Solution
load,load_erased,load_untyped, orload_untyped_async.load_asyncorload_erased_async, since those can be replicated usingload/load_erased+AssetServer::wait_for_asset_idto get the exact same effect.I am also intentionally leaving
NestedLoaderuntouched in this PR, but a followup will duplicate this API forNestedLoader, which should make it easier to understand.Unlike the
NestedLoaderAPI, we aren't doing any type-state craziness, so the docs are much more clear: users don't need to understand how type-state stuff works, they just call the handful of methods on the type. The "cost" here is we now need to be careful about including the cross product of loads between static asset type, runtime asset type, or dynamic asset type, crossed with deferred or async. In theory, if we added more kinds on either side, we would need to expand this cross product a lot. In practice though, it seems unlikely there will be any more variants there. (maybe there could be a blocking variant? I don't think this is a popular opinion though).A big con here is some somewhat common calls are now more verbose. Specifically,
asset_server.load_with_settings()has becomeasset_server.load_builder().with_settings().load(). I am not really concerned about this though, since it really isn't that painful.Testing
Showcase
Now instead of:
You can instead do:
We also now cover more variants! For example, you can now load an asset untyped with a guard, or with override_unapproved, etc.