Return an error when adding the same asset label multiple times.

crates/bevy_asset/src/lib.rs

@@ -677,7 +677,8 @@ mod tests {
         },
         loader::{AssetLoader, LoadContext},
         Asset, AssetApp, AssetEvent, AssetId, AssetLoadError, AssetLoadFailedEvent, AssetPath,
-        AssetPlugin, AssetServer, Assets, LoadState, UnapprovedPathMode,
+        AssetPlugin, AssetServer, Assets, DuplicateLabelError, LoadDirectError, LoadState,
+        UnapprovedPathMode,
     };
     use alloc::{
         boxed::Box,
@@ -776,7 +777,11 @@ mod tests {
                 sub_texts: ron
                     .sub_texts
                     .drain(..)
-                    .map(|text| load_context.add_labeled_asset(text.clone(), SubText { text }))
+                    .map(|text| {
+                        load_context
+                            .add_labeled_asset(text.clone(), SubText { text })
+                            .unwrap()
+                    })
                     .collect(),
             })
         }
@@ -1840,7 +1845,7 @@ mod tests {
 
         impl AssetLoader for NestedLoadOfSubassetLoader {
             type Asset = TestAsset;
-            type Error = crate::loader::LoadDirectError;
+            type Error = LoadDirectError;
             type Settings = ();
 
             async fn load(
@@ -1880,6 +1885,205 @@ mod tests {
         });
     }
 
+    #[test]
+    fn error_on_duplicate_subasset_names() {
+        let mut app = App::new();
+
+        let dir = Dir::default();
+        dir.insert_asset_text(Path::new("a.txt"), "");
+
+        app.register_asset_source(
+            AssetSourceId::Default,
+            AssetSource::build()
+                .with_reader(move || Box::new(MemoryAssetReader { root: dir.clone() })),
+        )
+        .add_plugins((TaskPoolPlugin::default(), AssetPlugin::default()));
+
+        struct DuplicateSubassetLoader;
+
+        impl AssetLoader for DuplicateSubassetLoader {
+            type Asset = TestAsset;
+            type Error = DuplicateLabelError;
+            type Settings = ();
+
+            async fn load(
+                &self,
+                _: &mut dyn Reader,
+                _: &Self::Settings,
+                load_context: &mut LoadContext<'_>,
+            ) -> Result<Self::Asset, Self::Error> {
+                load_context.add_labeled_asset("A".into(), SubText { text: "one".into() })?;
+                load_context.add_labeled_asset("A".into(), SubText { text: "two".into() })?;
+                Ok(TestAsset)
+            }
+        }
+
+        app.init_asset::<TestAsset>()
+            .init_asset::<SubText>()
+            .register_asset_loader(DuplicateSubassetLoader);
+
+        let asset_server = app.world().resource::<AssetServer>().clone();
+        let handle = asset_server.load::<TestAsset>("a.txt");
+
+        run_app_until(&mut app, |_world| match asset_server.load_state(&handle) {
+            LoadState::Loading => None,
+            LoadState::Failed(err) => {
+                assert!(err.to_string().contains("\"A\" is already in use"));
+                Some(())
+            }
+            state => panic!("Unexpected asset state: {state:?}"),
+        });
+    }
+
+    #[test]
+    fn error_on_diamond_duplicate_subasset_names() {
+        // Preventing duplicate subasset names is fairly trivial for a single LoadContext. However,
+        // we also need to handle the case of multiple LoadContext's for the same asset (through
+        // `labeled_asset_scope` or `begin_labeled_asset`).
+        let mut app = App::new();
+
+        let dir = Dir::default();
+        dir.insert_asset_text(Path::new("a.txt"), "");
+
+        app.register_asset_source(
+            AssetSourceId::Default,
+            AssetSource::build()
+                .with_reader(move || Box::new(MemoryAssetReader { root: dir.clone() })),
+        )
+        .add_plugins((TaskPoolPlugin::default(), AssetPlugin::default()));
+
+        struct DiamondDuplicateSubassetLoader;
+
+        impl AssetLoader for DiamondDuplicateSubassetLoader {
+            type Asset = TestAsset;
+            type Error = DuplicateLabelError;
+            type Settings = ();
+
+            async fn load(
+                &self,
+                _: &mut dyn Reader,
+                _: &Self::Settings,
+                load_context: &mut LoadContext<'_>,
+            ) -> Result<Self::Asset, Self::Error> {
+                let mut context_1 = load_context.begin_labeled_asset();
+                let mut context_2 = load_context.begin_labeled_asset();
+                context_1.add_labeled_asset("C".into(), SubText { text: "one".into() })?;
+                context_2.add_labeled_asset("C".into(), SubText { text: "two".into() })?;
+                let subasset_1 = context_1.finish(TestAsset);
+                let subasset_2 = context_2.finish(TestAsset);
+                load_context.add_loaded_labeled_asset("A", subasset_1)?;
+                load_context.add_loaded_labeled_asset("B", subasset_2)?;
+                Ok(TestAsset)
+            }
+        }
+
+        app.init_asset::<TestAsset>()
+            .init_asset::<SubText>()
+            .register_asset_loader(DiamondDuplicateSubassetLoader);
+
+        let asset_server = app.world().resource::<AssetServer>().clone();
+        let handle = asset_server.load::<TestAsset>("a.txt");
+
+        run_app_until(&mut app, |_world| match asset_server.load_state(&handle) {
+            LoadState::Loading => None,
+            LoadState::Failed(err) => {
+                assert!(err.to_string().contains("\"C\" is already in use"));
+                Some(())
+            }
+            state => panic!("Unexpected asset state: {state:?}"),
+        });
+    }
+
+    #[test]
+    fn no_error_for_diamond_duplicate_subasset_names_in_immediate_nested_assets() {
+        // While we want to ban duplicate subasset names within an asset, having two nested assets
+        // that reuse a subasset name should be supported (since these subasset names are not owned
+        // by the root asset).
+        let mut app = App::new();
+
+        let dir = Dir::default();
+        dir.insert_asset_text(Path::new("a.root"), "");
+        dir.insert_asset_text(Path::new("b.nest"), "");
+        dir.insert_asset_text(Path::new("c.nest"), "");
+
+        app.register_asset_source(
+            AssetSourceId::Default,
+            AssetSource::build()
+                .with_reader(move || Box::new(MemoryAssetReader { root: dir.clone() })),
+        )
+        .add_plugins((TaskPoolPlugin::default(), AssetPlugin::default()));
+
+        struct NestedAssetLoader;
+
+        impl AssetLoader for NestedAssetLoader {
+            type Asset = TestAsset;
+            type Error = DuplicateLabelError;
+            type Settings = ();
+
+            async fn load(
+                &self,
+                _: &mut dyn Reader,
+                _: &Self::Settings,
+                load_context: &mut LoadContext<'_>,
+            ) -> Result<Self::Asset, Self::Error> {
+                load_context.add_labeled_asset("Duplicate".into(), TestAsset)?;
+                Ok(TestAsset)
+            }
+
+            fn extensions(&self) -> &[&str] {
+                &["nest"]
+            }
+        }
+
+        struct DiamondDuplicateSubassetLoader;
+
+        impl AssetLoader for DiamondDuplicateSubassetLoader {
+            type Asset = TestAsset;
+            type Error = BevyError;
+            type Settings = ();
+
+            async fn load(
+                &self,
+                _: &mut dyn Reader,
+                _: &Self::Settings,
+                load_context: &mut LoadContext<'_>,
+            ) -> Result<Self::Asset, Self::Error> {
+                let b = load_context
+                    .loader()
+                    .immediate()
+                    .load::<TestAsset>("b.nest")
+                    .await?;
+                let c = load_context
+                    .loader()
+                    .immediate()
+                    .load::<TestAsset>("c.nest")
+                    .await?;
+
+                load_context.add_loaded_labeled_asset("B", b)?;
+                load_context.add_loaded_labeled_asset("C", c)?;
+                Ok(TestAsset)
+            }
+
+            fn extensions(&self) -> &[&str] {
+                &["root"]
+            }
+        }
+
+        app.init_asset::<TestAsset>()
+            .init_asset::<SubText>()
+            .register_asset_loader(NestedAssetLoader)
+            .register_asset_loader(DiamondDuplicateSubassetLoader);
+
+        let asset_server = app.world().resource::<AssetServer>().clone();
+        let handle = asset_server.load::<TestAsset>("a.root");
+
+        run_app_until(&mut app, |_world| match asset_server.load_state(&handle) {
+            LoadState::Loading => None,
+            LoadState::Loaded => Some(()),
+            state => panic!("Unexpected asset state: {state:?}"),
+        });
+    }
+
     // validate the Asset derive macro for various asset types
     #[derive(Asset, TypePath)]
     pub struct TestAsset;

crates/bevy_asset/src/loader.rs

@@ -19,7 +19,10 @@ use core::any::{Any, TypeId};
 use downcast_rs::{impl_downcast, Downcast};
 use ron::error::SpannedError;
 use serde::{Deserialize, Serialize};
-use std::path::{Path, PathBuf};
+use std::{
+    path::{Path, PathBuf},
+    sync::Mutex,
+};
 use thiserror::Error;
 
 /// Loads an [`Asset`] from a given byte [`Reader`]. This can accept [`AssetLoader::Settings`], which configure how the [`Asset`]
@@ -320,7 +323,7 @@ pub struct LoadContext<'a> {
     pub(crate) dependencies: HashSet<UntypedAssetId>,
     /// Direct dependencies used by this loader.
     pub(crate) loader_dependencies: HashMap<AssetPath<'static>, AssetHash>,
-    pub(crate) labeled_assets: HashMap<CowArc<'static, str>, LabeledAsset>,
+    pub(crate) labeled_assets: &'a Mutex<HashMap<CowArc<'static, str>, LabeledAsset>>,
 }
 
 impl<'a> LoadContext<'a> {
@@ -330,6 +333,7 @@ impl<'a> LoadContext<'a> {
         asset_path: AssetPath<'static>,
         should_load_dependencies: bool,
         populate_hashes: bool,
+        labeled_assets: &'a Mutex<HashMap<CowArc<'static, str>, LabeledAsset>>,
     ) -> Self {
         Self {
             asset_server,
@@ -338,7 +342,7 @@ impl<'a> LoadContext<'a> {
             should_load_dependencies,
             dependencies: HashSet::default(),
             loader_dependencies: HashMap::default(),
-            labeled_assets: HashMap::default(),
+            labeled_assets,
         }
     }
 
@@ -377,6 +381,7 @@ impl<'a> LoadContext<'a> {
             self.asset_path.clone(),
             self.should_load_dependencies,
             self.populate_hashes,
+            self.labeled_assets,
         )
     }
 
@@ -392,11 +397,11 @@ impl<'a> LoadContext<'a> {
         &mut self,
         label: String,
         load: impl FnOnce(&mut LoadContext) -> Result<A, E>,
-    ) -> Result<Handle<A>, E> {
+    ) -> Result<Handle<A>, LabeledAssetScopeError<E>> {
         let mut context = self.begin_labeled_asset();
-        let asset = load(&mut context)?;
+        let asset = load(&mut context).map_err(|e| LabeledAssetScopeError::ScopeError(e))?;
         let loaded_asset = context.finish(asset);
-        Ok(self.add_loaded_labeled_asset(label, loaded_asset))
+        Ok(self.add_loaded_labeled_asset(label, loaded_asset)?)
     }
 
     /// This will add the given `asset` as a "labeled [`Asset`]" with the `label` label.
@@ -409,9 +414,16 @@ 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>, DuplicateLabelError> {
         self.labeled_asset_scope(label, |_| Ok::<_, ()>(asset))
-            .expect("the closure returns Ok")
+            .map_err(|err| match err {
+                LabeledAssetScopeError::ScopeError(_) => unreachable!(),
+                LabeledAssetScopeError::DuplicateLabel(err) => err,
+            })
     }
 
     /// Add a [`LoadedAsset`] that is a "labeled sub asset" of the root path of this load context.
@@ -423,21 +435,29 @@ impl<'a> LoadContext<'a> {
         &mut self,
         label: impl Into<CowArc<'static, str>>,
         loaded_asset: LoadedAsset<A>,
-    ) -> Handle<A> {
+    ) -> Result<Handle<A>, DuplicateLabelError> {
         let label = label.into();
         let loaded_asset: ErasedLoadedAsset = loaded_asset.into();
         let labeled_path = self.asset_path.clone().with_label(label.clone());
         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(),
-            },
-        );
-        handle
+        if self
+            .labeled_assets
+            .lock()
+            .unwrap()
+            .insert(
+                label.clone(),
+                LabeledAsset {
+                    asset: loaded_asset,
+                    handle: handle.clone().untyped(),
+                },
+            )
+            .is_some()
+        {
+            return Err(DuplicateLabelError(label));
+        }
+        Ok(handle)
     }
 
     /// Returns `true` if an asset with the label `label` exists in this context.
@@ -454,7 +474,7 @@ impl<'a> LoadContext<'a> {
             value,
             dependencies: self.dependencies,
             loader_dependencies: self.loader_dependencies,
-            labeled_assets: self.labeled_assets,
+            labeled_assets: Default::default(),
         }
     }
 
@@ -585,3 +605,20 @@ 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,
 }
+
+#[derive(Error, Debug)]
+#[error("The subasset label \"{0}\" is already in use")]
+pub struct DuplicateLabelError(pub CowArc<'static, str>);
+
+#[derive(Error, Debug)]
+pub enum LabeledAssetScopeError<E> {
+    #[error(transparent)]
+    DuplicateLabel(DuplicateLabelError),
+    ScopeError(E),
+}
+
+impl<E> From<DuplicateLabelError> for LabeledAssetScopeError<E> {
+    fn from(value: DuplicateLabelError) -> Self {
+        Self::DuplicateLabel(value)
+    }
+}