Repository: NiklasEi/bevy_asset_loader (rust)

Issue:
Nasty bug: Trying to load the same collection twice causes a freeze and no errors reported 
Nasty bug: Trying to load the same collection twice causes a freeze and no errors reported 
Version: 0.19 

For example, do this in the foxtrot example: 
...


 .add_loading_state(
            LoadingState::new(AppState::Loading)
                .continue_to_state(AppState::PostLoading)
                .load_collection:: < AudioAssets >()  
                  .load_collection:: < AudioAssets >()  //this is duplicated and this causes the problem 
              ....
              
              
              
      Suggestion:  Make this error case throw an error 

Pull request:
Don't get stuck with duplicate asset collections
resolves #192

Patch:
diff --git a/bevy_asset_loader/src/loading_state.rs b/bevy_asset_loader/src/loading_state.rs
index 8353230..1485534 100644
--- a/bevy_asset_loader/src/loading_state.rs
+++ b/bevy_asset_loader/src/loading_state.rs
@@ -498,7 +498,7 @@ pub(crate) struct LoadingStateSchedule<S: States>(pub S);
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default, States)]
 pub(crate) enum InternalLoadingState<S: States> {
-    /// Starting point. Here it will be decided whether or not dynamic asset collections need to be loaded.
+    /// Starting point. Here it will be decided whether dynamic asset collections need to be loaded.
     #[default]
     Initialize,
     /// Load dynamic asset collections and configure their key <-> asset mapping
@@ -545,7 +545,7 @@ struct LoadingConfiguration<State: States> {
     next: Option<State>,
     failure: Option<State>,
     loading_failed: bool,
-    loading_collections: usize,
+    loading_collections: HashSet<TypeId>,
     loading_dynamic_collections: HashSet<TypeId>,
 }
 
@@ -555,7 +555,7 @@ impl<State: States> Default for LoadingConfiguration<State> {
             next: None,
             failure: None,
             loading_failed: false,
-            loading_collections: 0,
+            loading_collections: default(),
             loading_dynamic_collections: default(),
         }
     }
diff --git a/bevy_asset_loader/src/loading_state/dynamic_asset_systems.rs b/bevy_asset_loader/src/loading_state/dynamic_asset_systems.rs
index 45ea197..2533865 100644
--- a/bevy_asset_loader/src/loading_state/dynamic_asset_systems.rs
+++ b/bevy_asset_loader/src/loading_state/dynamic_asset_systems.rs
@@ -5,8 +5,8 @@ use bevy::ecs::change_detection::ResMut;
 use bevy::ecs::schedule::{NextState, State, States};
 use bevy::ecs::system::{Res, SystemState};
 use bevy::ecs::world::World;
-use bevy::log::debug;
-use std::any::TypeId;
+use bevy::log::{debug, warn};
+use std::any::{type_name, TypeId};
 
 #[allow(clippy::type_complexity)]
 pub(crate) fn load_dynamic_asset_collections<S: States, C: DynamicAssetCollection + Asset>(
@@ -33,7 +33,9 @@ pub(crate) fn load_dynamic_asset_collections<S: States, C: DynamicAssetCollectio
         .state_configurations
         .get_mut(state.get())
     {
-        config.loading_dynamic_collections.insert(TypeId::of::<C>());
+        if !config.loading_dynamic_collections.insert(TypeId::of::<C>()) {
+            warn!("The dynamic asset collection {} was registered multiple times on the loading state {:?}", type_name::<C>(), state.get());
+        }
     }
     world.insert_resource(loading_collections);
 }
diff --git a/bevy_asset_loader/src/loading_state/systems.rs b/bevy_asset_loader/src/loading_state/systems.rs
index 118c6e3..a662036 100644
--- a/bevy_asset_loader/src/loading_state/systems.rs
+++ b/bevy_asset_loader/src/loading_state/systems.rs
@@ -41,7 +41,13 @@ pub(crate) fn start_loading_collection<S: States, Assets: AssetCollection>(
                 &state
             )
         });
-    config.loading_collections += 1;
+    if !config.loading_collections.insert(TypeId::of::<Assets>()) {
+        warn!(
+            "The asset collection '{}' was added multiple times to the loading state '{:?}'",
+            type_name::<Assets>(),
+            state.get()
+        );
+    }
     let handles = LoadingAssetHandles {
         handles: Assets::load(world),
         marker: PhantomData::<Assets>,
@@ -107,7 +113,7 @@ fn count_loaded_handles<S: States, Assets: AssetCollection>(cell: WorldCell) ->
         if failure {
             config.loading_failed = true;
         } else {
-            config.loading_collections -= 1;
+            config.loading_collections.remove(&TypeId::of::<Assets>());
         }
     } else {
         warn!("Failed to read loading state configuration in count_loaded_handles");
@@ -126,7 +132,7 @@ pub(crate) fn resume_to_finalize<S: States>(
         .state_configurations
         .get(user_state.get())
     {
-        if configuration.loading_collections == 0 {
+        if configuration.loading_collections.is_empty() {
             internal_state.set(InternalLoadingState::Finalize);
         }
         if configuration.loading_failed && configuration.failure.is_some() {


Test patch:
diff --git a/bevy_asset_loader/tests/multiple_loading_states.rs b/bevy_asset_loader/tests/multiple_loading_states.rs
index b2dacd9..279c868 100644
--- a/bevy_asset_loader/tests/multiple_loading_states.rs
+++ b/bevy_asset_loader/tests/multiple_loading_states.rs
@@ -1,39 +1,38 @@
-#![allow(dead_code, unused_imports)]
-
 use bevy::app::AppExit;
 use bevy::audio::AudioPlugin;
 use bevy::prelude::*;
 use bevy_asset_loader::prelude::*;
 
-#[cfg(all(
-    not(feature = "2d"),
-    not(feature = "3d"),
-    not(feature = "progress_tracking")
-))]
 #[test]
 fn multiple_loading_states() {
-    App::new()
-        .init_state::<MyStates>()
-        .add_plugins((
-            MinimalPlugins,
-            AssetPlugin::default(),
-            AudioPlugin::default(),
-        ))
-        .add_loading_state(
-            LoadingState::new(MyStates::Splash)
-                .continue_to_state(MyStates::Load)
-                .load_collection::<SplashAssets>(),
-        )
-        .add_loading_state(
-            LoadingState::new(MyStates::Load)
-                .continue_to_state(MyStates::Play)
-                .load_collection::<MyAssets>()
-                .load_collection::<MyOtherAssets>(),
-        )
-        .add_systems(Update, (quit.run_if(in_state(MyStates::Play)), timeout))
-        .add_systems(OnEnter(MyStates::Load), use_splash_assets)
-        .add_systems(OnEnter(MyStates::Play), use_loading_assets)
-        .run();
+    let mut app = App::new();
+    app.init_state::<MyStates>();
+
+    #[cfg(feature = "progress_tracking")]
+    app.add_plugins((
+        iyes_progress::ProgressPlugin::new(MyStates::Splash),
+        iyes_progress::ProgressPlugin::new(MyStates::Load),
+    ));
+    app.add_plugins((
+        MinimalPlugins,
+        AssetPlugin::default(),
+        AudioPlugin::default(),
+    ))
+    .add_loading_state(
+        LoadingState::new(MyStates::Splash)
+            .continue_to_state(MyStates::Load)
+            .load_collection::<SplashAssets>(),
+    )
+    .add_loading_state(
+        LoadingState::new(MyStates::Load)
+            .continue_to_state(MyStates::Play)
+            .load_collection::<MyAssets>()
+            .load_collection::<MyOtherAssets>(),
+    )
+    .add_systems(Update, (quit.run_if(in_state(MyStates::Play)), timeout))
+    .add_systems(OnEnter(MyStates::Load), use_splash_assets)
+    .add_systems(OnEnter(MyStates::Play), use_loading_assets)
+    .run();
 }
 
 fn timeout(time: Res<Time>) {
@@ -46,7 +45,11 @@ fn use_splash_assets(_splash_assets: Res<SplashAssets>) {
     // I could do something with the splash assets here
 }
 
-fn use_loading_assets(_my_assets: Res<MyAssets>, _my_other_assets: Res<MyOtherAssets>) {
+fn use_loading_assets(
+    _my_assets: Res<MyAssets>,
+    _my_other_assets: Res<MyOtherAssets>,
+    _splash_assets: Res<SplashAssets>,
+) {
     // I could do something with the `MyAssets` and `MyOtherAssets` collections here
 }
 
diff --git a/bevy_asset_loader/tests/same_collection_multiple_times.rs b/bevy_asset_loader/tests/same_collection_multiple_times.rs
new file mode 100644
index 0000000..cdec1e5
--- /dev/null
+++ b/bevy_asset_loader/tests/same_collection_multiple_times.rs
@@ -0,0 +1,59 @@
+use bevy::app::AppExit;
+use bevy::audio::AudioPlugin;
+use bevy::prelude::*;
+use bevy_asset_loader::prelude::*;
+
+#[test]
+fn same_collection_multiple_times() {
+    let mut app = App::new();
+    app.init_state::<MyStates>();
+
+    #[cfg(feature = "progress_tracking")]
+    app.add_plugins(iyes_progress::ProgressPlugin::new(MyStates::Load));
+    app.add_plugins((
+        MinimalPlugins,
+        AssetPlugin::default(),
+        AudioPlugin::default(),
+    ))
+    .add_loading_state(
+        LoadingState::new(MyStates::Load)
+            .continue_to_state(MyStates::Play)
+            .load_collection::<MyAssets>()
+            .load_collection::<MyAssets>(),
+    )
+    .configure_loading_state(LoadingStateConfig::new(MyStates::Load).load_collection::<MyAssets>())
+    .add_systems(Update, (quit.run_if(in_state(MyStates::Play)), timeout))
+    .add_systems(OnEnter(MyStates::Play), use_loading_assets)
+    .run();
+}
+
+#[derive(AssetCollection, Resource)]
+struct MyAssets {
+    #[asset(path = "audio/background.ogg")]
+    background: Handle<AudioSource>,
+}
+
+#[derive(Clone, Copy, Debug, States, Default, PartialEq, Eq, Hash)]
+enum MyStates {
+    #[default]
+    Load,
+    Play,
+}
+
+fn use_loading_assets(assets: Res<MyAssets>, asset_server: Res<AssetServer>) {
+    assert!(
+        asset_server.is_loaded_with_dependencies(&assets.background),
+        "The collection is not fully loaded!"
+    );
+}
+
+fn timeout(time: Res<Time>) {
+    if time.elapsed_seconds_f64() > 30. {
+        panic!("The app did not finish in 30 seconds");
+    }
+}
+
+fn quit(mut exit: EventWriter<AppExit>) {
+    info!("Everything fine, quitting the app");
+    exit.send(AppExit);
+}