Skip to content

Replace all different load variants in AssetServer with a builder.#23663

Open
andriyDev wants to merge 6 commits intobevyengine:mainfrom
andriyDev:load-builder
Open

Replace all different load variants in AssetServer with a builder.#23663
andriyDev wants to merge 6 commits intobevyengine:mainfrom
andriyDev:load-builder

Conversation

@andriyDev
Copy link
Copy Markdown
Contributor

Objective

  • In 0.18, we had 10 different functions that load assets (I'm not even counting load_folder).
  • In 0.19, we've even added load_erased - but it unfortunately doesn't support all the features that the other variants support.
  • We apparently needed load_acquire_with_settings_override which 1) loads the asset, 2) uses the settings provided, 3) allows reading unapproved asset paths, and 4) drops a guard once the load completes.
    • That's fine if that's necessary. But we needed to create an explicit variant for that.
  • We need fewer load paths!

Solution

  • Create a builder.
  • Store all these options dynamically instead of statically handling each case.
  • Have the caller choose a particular "kind" of load when they are ready: load, load_erased, load_untyped, or load_untyped_async.
    • I intentionally didn't provide a load_async or load_erased_async, since those can be replicated using load/load_erased + AssetServer::wait_for_asset_id to get the exact same effect.

I am also intentionally leaving NestedLoader untouched in this PR, but a followup will duplicate this API for NestedLoader, which should make it easier to understand.

Unlike the NestedLoader API, 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 become asset_server.load_builder().with_settings().load(). I am not really concerned about this though, since it really isn't that painful.

Testing

  • Tests all pass!

Showcase

Now instead of:

asset_server.load_acquire_with_settings_override("some_path", |settings: &mut GltfLoaderSettings| { ... }, my_lock_guard);

You can instead do:

asset_server.load_builder()
    .with_guard(my_lock_guard)
    .with_settings(|settings: &mut GltfLoaderSettings| { ... })
    .override_unapproved()
    .load("some_path");

We also now cover more variants! For example, you can now load an asset untyped with a guard, or with override_unapproved, etc.

@andriyDev andriyDev 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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 4, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Assets Apr 4, 2026
Copy link
Copy Markdown
Contributor

@greeble-dev greeble-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: Advanced AssetServer load variants are now expose through a builder pattern.
title: Advanced AssetServer load variants are now exposed through a builder pattern.

Comment on lines +20 to +21
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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +368 to 371
#[must_use = "the load doesn't start until LoadBuilder has been consumed"]
pub fn load_builder(&self) -> LoadBuilder<'_> {
LoadBuilder::new(self)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Comment on lines +387 to 388
#[deprecated(note = "Use `asset_server.load_builder().load_erased(type_id, path)` instead")]
pub fn load_erased<'a>(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`].
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

2 participants