Skip to content

Commit

Permalink
Make adding a subasset label return a result for if there is a duplic…
Browse files Browse the repository at this point in the history
…ate label. (#18013)

# Objective

- Makes #18010 more easily debuggable. This doesn't solve that issue,
but protects us from it in the future.

## Solution

- Make `LoadContext::add_labeled_asset` and friends return an error if
it finds a duplicate asset.

## Testing

- Added a test - it fails before the fix.

---

## Migration Guide

- `AssetLoader`s must now handle the case of a duplicate subasset label
when using `LoadContext::add_labeled_asset` and its variants. If you
know your subasset labels are unique by construction (e.g., they include
an index number), you can simply unwrap this result.
  • Loading branch information
andriyDev authored Feb 24, 2025
1 parent 6bae04a commit ed1143b
Show file tree
Hide file tree
Showing 3 changed files with 316 additions and 234 deletions.
49 changes: 47 additions & 2 deletions crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ mod tests {
},
loader::{AssetLoader, LoadContext},
Asset, AssetApp, AssetEvent, AssetId, AssetLoadError, AssetLoadFailedEvent, AssetPath,
AssetPlugin, AssetServer, Assets,
AssetPlugin, AssetServer, Assets, DuplicateLabelAssetError, LoadState,
};
use alloc::{
boxed::Box,
Expand Down Expand Up @@ -695,6 +695,8 @@ mod tests {
CannotLoadDependency { dependency: AssetPath<'static> },
#[error("A RON error occurred during loading")]
RonSpannedError(#[from] ron::error::SpannedError),
#[error(transparent)]
DuplicateLabelAssetError(#[from] DuplicateLabelAssetError),
#[error("An IO error occurred during loading")]
Io(#[from] std::io::Error),
}
Expand Down Expand Up @@ -740,7 +742,7 @@ mod tests {
.sub_texts
.drain(..)
.map(|text| load_context.add_labeled_asset(text.clone(), SubText { text }))
.collect(),
.collect::<Result<Vec<_>, _>>()?,
})
}

Expand Down Expand Up @@ -1778,6 +1780,49 @@ mod tests {
app.world_mut().run_schedule(Update);
}

#[test]
fn fails_to_load_for_duplicate_subasset_labels() {
let mut app = App::new();

let dir = Dir::default();
dir.insert_asset_text(
Path::new("a.ron"),
r#"(
text: "b",
dependencies: [],
embedded_dependencies: [],
sub_texts: ["A", "A"],
)"#,
);

app.register_asset_source(
AssetSourceId::Default,
AssetSource::build()
.with_reader(move || Box::new(MemoryAssetReader { root: dir.clone() })),
)
.add_plugins((
TaskPoolPlugin::default(),
LogPlugin::default(),
AssetPlugin::default(),
));

app.init_asset::<CoolText>()
.init_asset::<SubText>()
.register_asset_loader(CoolTextLoader);

let asset_server = app.world().resource::<AssetServer>().clone();
let handle = asset_server.load::<CoolText>("a.ron");

run_app_until(&mut app, |_world| match asset_server.load_state(&handle) {
LoadState::Loading => None,
LoadState::Failed(err) => {
assert!(matches!(*err, AssetLoadError::AssetLoaderError(_)));
Some(())
}
state => panic!("Unexpected asset state: {state:?}"),
});
}

// validate the Asset derive macro for various asset types
#[derive(Asset, TypePath)]
pub struct TestAsset;
Expand Down
40 changes: 27 additions & 13 deletions crates/bevy_asset/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use alloc::{
};
use atomicow::CowArc;
use bevy_ecs::world::World;
use bevy_log::warn;
use bevy_platform_support::collections::{HashMap, HashSet};
use bevy_tasks::{BoxedFuture, ConditionalSendFuture};
use core::any::{Any, TypeId};
Expand Down Expand Up @@ -458,7 +457,7 @@ impl<'a> LoadContext<'a> {
&mut self,
label: String,
load: impl FnOnce(&mut LoadContext) -> A,
) -> Handle<A> {
) -> Result<Handle<A>, DuplicateLabelAssetError> {
let mut context = self.begin_labeled_asset();
let asset = load(&mut context);
let complete_asset = context.finish(asset);
Expand All @@ -475,7 +474,11 @@ impl<'a> LoadContext<'a> {
/// new [`LoadContext`] to track the dependencies for the labeled asset.
///
/// See [`AssetPath`] for more on labeled assets.
pub fn add_labeled_asset<A: Asset>(&mut self, label: String, asset: A) -> Handle<A> {
pub fn add_labeled_asset<A: Asset>(
&mut self,
label: String,
asset: A,
) -> Result<Handle<A>, DuplicateLabelAssetError> {
self.labeled_asset_scope(label, |_| asset)
}

Expand All @@ -488,7 +491,7 @@ impl<'a> LoadContext<'a> {
&mut self,
label: impl Into<CowArc<'static, str>>,
loaded_asset: CompleteLoadedAsset<A>,
) -> Handle<A> {
) -> Result<Handle<A>, DuplicateLabelAssetError> {
let label = label.into();
let CompleteLoadedAsset {
asset,
Expand All @@ -499,19 +502,25 @@ impl<'a> LoadContext<'a> {
let handle = self
.asset_server
.get_or_create_path_handle(labeled_path, None);
self.labeled_assets.insert(
label,
LabeledAsset {
asset: loaded_asset,
handle: handle.clone().untyped(),
},
);
let has_duplicate = self
.labeled_assets
.insert(
label.clone(),
LabeledAsset {
asset: loaded_asset,
handle: handle.clone().untyped(),
},
)
.is_some();
if has_duplicate {
return Err(DuplicateLabelAssetError(label.to_string()));
}
for (label, asset) in labeled_assets {
if self.labeled_assets.insert(label.clone(), asset).is_some() {
warn!("A labeled asset with the label \"{label}\" already exists. Replacing with the new asset.");
return Err(DuplicateLabelAssetError(label.to_string()));
}
}
handle
Ok(handle)
}

/// Returns `true` if an asset with the label `label` exists in this context.
Expand Down Expand Up @@ -661,3 +670,8 @@ pub enum ReadAssetBytesError {
#[error("The LoadContext for this read_asset_bytes call requires hash metadata, but it was not provided. This is likely an internal implementation error.")]
MissingAssetHash,
}

/// An error when labeled assets have the same label, containing the duplicate label.
#[derive(Error, Debug)]
#[error("Encountered a duplicate label while loading an asset: \"{0}\"")]
pub struct DuplicateLabelAssetError(pub String);
Loading

0 comments on commit ed1143b

Please sign in to comment.