From 5781806e72548e1a37d8492c47ad420e7ad86dd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= Date: Sun, 15 Oct 2023 20:20:29 +0200 Subject: [PATCH 01/21] only set up processed source if asset plugin is not unprocessed (#10123) # Objective - Since #9885, running on an iOS device crashes trying to create the processed folder - This only happens on real device, not on the simulator ## Solution - Setup processed assets only if needed --- crates/bevy_asset/src/io/source.rs | 25 +++++++++++++++---------- crates/bevy_asset/src/lib.rs | 6 +++++- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/bevy_asset/src/io/source.rs b/crates/bevy_asset/src/io/source.rs index ea07f8d39a..8ee192462a 100644 --- a/crates/bevy_asset/src/io/source.rs +++ b/crates/bevy_asset/src/io/source.rs @@ -241,20 +241,25 @@ impl AssetSourceBuilder { /// Returns a builder containing the "platform default source" for the given `path` and `processed_path`. /// For most platforms, this will use [`FileAssetReader`](crate::io::file::FileAssetReader) / [`FileAssetWriter`](crate::io::file::FileAssetWriter), /// but some platforms (such as Android) have their own default readers / writers / watchers. - pub fn platform_default(path: &str, processed_path: &str) -> Self { - Self::default() + pub fn platform_default(path: &str, processed_path: Option<&str>) -> Self { + let default = Self::default() .with_reader(AssetSource::get_default_reader(path.to_string())) .with_writer(AssetSource::get_default_writer(path.to_string())) .with_watcher(AssetSource::get_default_watcher( path.to_string(), Duration::from_millis(300), - )) - .with_processed_reader(AssetSource::get_default_reader(processed_path.to_string())) - .with_processed_writer(AssetSource::get_default_writer(processed_path.to_string())) - .with_processed_watcher(AssetSource::get_default_watcher( - processed_path.to_string(), - Duration::from_millis(300), - )) + )); + if let Some(processed_path) = processed_path { + default + .with_processed_reader(AssetSource::get_default_reader(processed_path.to_string())) + .with_processed_writer(AssetSource::get_default_writer(processed_path.to_string())) + .with_processed_watcher(AssetSource::get_default_watcher( + processed_path.to_string(), + Duration::from_millis(300), + )) + } else { + default + } } } @@ -315,7 +320,7 @@ impl AssetSourceBuilders { } /// Initializes the default [`AssetSourceBuilder`] if it has not already been set. - pub fn init_default_source(&mut self, path: &str, processed_path: &str) { + pub fn init_default_source(&mut self, path: &str, processed_path: Option<&str>) { self.default .get_or_insert_with(|| AssetSourceBuilder::platform_default(path, processed_path)); } diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index 148dc40e56..66af7f0216 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -122,7 +122,11 @@ impl Plugin for AssetPlugin { let mut sources = app .world .get_resource_or_insert_with::(Default::default); - sources.init_default_source(&self.file_path, &self.processed_file_path); + sources.init_default_source( + &self.file_path, + (!matches!(self.mode, AssetMode::Unprocessed)) + .then_some(self.processed_file_path.as_str()), + ); embedded.register_source(&mut sources); } { From 49d5c6b8a361a209da07dc774e33fe44b65e9dfb Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sun, 15 Oct 2023 11:21:49 -0700 Subject: [PATCH 02/21] Hot reload labeled assets whose source asset is not loaded (#9736) # Objective As called out in #9714, Bevy Asset V2 fails to hot-reload labeled assets whose source asset has changed (in cases where the root asset is not alive). ## Solution Track alive labeled assets for a given source asset and allow hot reloads in cases where a labeled asset is still alive. --- crates/bevy_asset/src/server/info.rs | 53 +++++++++++++++++++++++++++- crates/bevy_asset/src/server/mod.rs | 2 +- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/crates/bevy_asset/src/server/info.rs b/crates/bevy_asset/src/server/info.rs index baa4007001..f41057f622 100644 --- a/crates/bevy_asset/src/server/info.rs +++ b/crates/bevy_asset/src/server/info.rs @@ -69,6 +69,9 @@ pub(crate) struct AssetInfos { /// Tracks assets that depend on the "key" asset path inside their asset loaders ("loader dependencies") /// This should only be set when watching for changes to avoid unnecessary work. pub(crate) loader_dependants: HashMap, HashSet>>, + /// Tracks living labeled assets for a given source asset. + /// This should only be set when watching for changes to avoid unnecessary work. + pub(crate) living_labeled_assets: HashMap, HashSet>, pub(crate) handle_providers: HashMap, pub(crate) dependency_loaded_event_sender: HashMap, } @@ -88,6 +91,8 @@ impl AssetInfos { Self::create_handle_internal( &mut self.infos, &self.handle_providers, + &mut self.living_labeled_assets, + self.watching_for_changes, TypeId::of::(), None, None, @@ -107,6 +112,8 @@ impl AssetInfos { Self::create_handle_internal( &mut self.infos, &self.handle_providers, + &mut self.living_labeled_assets, + self.watching_for_changes, type_id, None, None, @@ -116,9 +123,12 @@ impl AssetInfos { ) } + #[allow(clippy::too_many_arguments)] fn create_handle_internal( infos: &mut HashMap, handle_providers: &HashMap, + living_labeled_assets: &mut HashMap, HashSet>, + watching_for_changes: bool, type_id: TypeId, path: Option>, meta_transform: Option, @@ -128,6 +138,16 @@ impl AssetInfos { .get(&type_id) .ok_or(MissingHandleProviderError(type_id))?; + if watching_for_changes { + if let Some(path) = &path { + let mut without_label = path.to_owned(); + if let Some(label) = without_label.take_label() { + let labels = living_labeled_assets.entry(without_label).or_default(); + labels.insert(label.to_string()); + } + } + } + let handle = provider.reserve_handle_internal(true, path.clone(), meta_transform); let mut info = AssetInfo::new(Arc::downgrade(&handle), path); if loading { @@ -136,6 +156,7 @@ impl AssetInfos { info.rec_dep_load_state = RecursiveDependencyLoadState::Loading; } infos.insert(handle.id, info); + Ok(UntypedHandle::Strong(handle)) } @@ -226,6 +247,8 @@ impl AssetInfos { let handle = Self::create_handle_internal( &mut self.infos, &self.handle_providers, + &mut self.living_labeled_assets, + self.watching_for_changes, type_id, Some(path), meta_transform, @@ -256,7 +279,7 @@ impl AssetInfos { Some(UntypedHandle::Strong(strong_handle)) } - /// Returns `true` if this path has + /// Returns `true` if the asset this path points to is still alive pub(crate) fn is_path_alive<'a>(&self, path: impl Into>) -> bool { let path = path.into(); if let Some(id) = self.path_to_id.get(&path) { @@ -267,12 +290,26 @@ impl AssetInfos { false } + /// Returns `true` if the asset at this path should be reloaded + pub(crate) fn should_reload(&self, path: &AssetPath) -> bool { + if self.is_path_alive(path) { + return true; + } + + if let Some(living) = self.living_labeled_assets.get(path) { + !living.is_empty() + } else { + false + } + } + // Returns `true` if the asset should be removed from the collection pub(crate) fn process_handle_drop(&mut self, id: UntypedAssetId) -> bool { Self::process_handle_drop_internal( &mut self.infos, &mut self.path_to_id, &mut self.loader_dependants, + &mut self.living_labeled_assets, self.watching_for_changes, id, ) @@ -521,6 +558,7 @@ impl AssetInfos { infos: &mut HashMap, path_to_id: &mut HashMap, UntypedAssetId>, loader_dependants: &mut HashMap, HashSet>>, + living_labeled_assets: &mut HashMap, HashSet>, watching_for_changes: bool, id: UntypedAssetId, ) -> bool { @@ -540,6 +578,18 @@ impl AssetInfos { dependants.remove(&path); } } + if let Some(label) = path.label() { + let mut without_label = path.to_owned(); + without_label.remove_label(); + if let Entry::Occupied(mut entry) = + living_labeled_assets.entry(without_label) + { + entry.get_mut().remove(label); + if entry.get().is_empty() { + entry.remove(); + } + }; + } } path_to_id.remove(&path); } @@ -566,6 +616,7 @@ impl AssetInfos { &mut self.infos, &mut self.path_to_id, &mut self.loader_dependants, + &mut self.living_labeled_assets, self.watching_for_changes, id.untyped(provider.type_id), ); diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 257813acd6..03cf286349 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -395,7 +395,7 @@ impl AssetServer { let path = path.into().into_owned(); IoTaskPool::get() .spawn(async move { - if server.data.infos.read().is_path_alive(&path) { + if server.data.infos.read().should_reload(&path) { info!("Reloading {path} because it has changed"); if let Err(err) = server.load_internal(None, path, true, None).await { error!("{}", err); From 02943737b25add58262f95346969ea068d491601 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sun, 15 Oct 2023 11:36:51 -0700 Subject: [PATCH 03/21] Return an error when loading non-existent labels (#9751) # Objective Calling `asset_server.load("scene.gltf#SomeLabel")` will silently fail if `SomeLabel` does not exist. Referenced in #9714 ## Solution We now detect this case and return an error. I also slightly refactored `load_internal` to make the logic / dataflow much clearer. --------- Co-authored-by: Pascal Hertleif --- crates/bevy_asset/src/path.rs | 6 +++ crates/bevy_asset/src/server/mod.rs | 72 +++++++++++++++++------------ 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs index 11168ca245..43b7d2cab5 100644 --- a/crates/bevy_asset/src/path.rs +++ b/crates/bevy_asset/src/path.rs @@ -203,6 +203,12 @@ impl<'a> AssetPath<'a> { self.label.as_deref() } + /// Gets the "sub-asset label". + #[inline] + pub fn label_cow(&self) -> Option> { + self.label.clone() + } + /// Gets the path to the asset in the "virtual filesystem". #[inline] pub fn path(&self) -> &Path { diff --git a/crates/bevy_asset/src/server/mod.rs b/crates/bevy_asset/src/server/mod.rs index 03cf286349..160a3e4d32 100644 --- a/crates/bevy_asset/src/server/mod.rs +++ b/crates/bevy_asset/src/server/mod.rs @@ -257,7 +257,7 @@ impl AssetServer { path: impl Into>, meta_transform: Option, ) -> Handle { - let mut path = path.into().into_owned(); + let path = path.into().into_owned(); let (handle, should_load) = self.data.infos.write().get_or_create_path_handle::( path.clone(), HandleLoadingMode::Request, @@ -265,13 +265,10 @@ impl AssetServer { ); if should_load { - let mut owned_handle = Some(handle.clone().untyped()); + let owned_handle = Some(handle.clone().untyped()); let server = self.clone(); IoTaskPool::get() .spawn(async move { - if path.take_label().is_some() { - owned_handle = None; - } if let Err(err) = server.load_internal(owned_handle, path, false, None).await { error!("{}", err); } @@ -291,6 +288,10 @@ impl AssetServer { self.load_internal(None, path, false, None).await } + /// Performs an async asset load. + /// + /// `input_handle` must only be [`Some`] if `should_load` was true when retrieving `input_handle`. This is an optimization to + /// avoid looking up `should_load` twice, but it means you _must_ be sure a load is necessary when calling this function with [`Some`]. async fn load_internal<'a>( &self, input_handle: Option, @@ -298,7 +299,7 @@ impl AssetServer { force: bool, meta_transform: Option, ) -> Result { - let mut path = path.into_owned(); + let path = path.into_owned(); let path_clone = path.clone(); let (mut meta, loader, mut reader) = self .get_meta_loader_and_reader(&path_clone) @@ -312,18 +313,8 @@ impl AssetServer { e })?; - let has_label = path.label().is_some(); - let (handle, should_load) = match input_handle { Some(handle) => { - if !has_label && handle.type_id() != loader.asset_type_id() { - return Err(AssetLoadError::RequestedHandleTypeMismatch { - path: path.into_owned(), - requested: handle.type_id(), - actual_asset_name: loader.asset_type_name(), - loader_name: loader.type_name(), - }); - } // if a handle was passed in, the "should load" check was already done (handle, true) } @@ -339,37 +330,51 @@ impl AssetServer { } }; + if path.label().is_none() && handle.type_id() != loader.asset_type_id() { + return Err(AssetLoadError::RequestedHandleTypeMismatch { + path: path.into_owned(), + requested: handle.type_id(), + actual_asset_name: loader.asset_type_name(), + loader_name: loader.type_name(), + }); + } + if !should_load && !force { return Ok(handle); } - let base_asset_id = if has_label { - path.remove_label(); - // If the path has a label, the current id does not match the asset root type. - // We need to get the actual asset id + + let (base_handle, base_path) = if path.label().is_some() { let mut infos = self.data.infos.write(); - let (actual_handle, _) = infos.get_or_create_path_handle_untyped( - path.clone(), + let base_path = path.without_label().into_owned(); + let (base_handle, _) = infos.get_or_create_path_handle_untyped( + base_path.clone(), loader.asset_type_id(), loader.asset_type_name(), - // ignore current load state ... we kicked off this sub asset load because it needed to be loaded but - // does not currently exist HandleLoadingMode::Force, None, ); - actual_handle.id() + (base_handle, base_path) } else { - handle.id() + (handle.clone(), path.clone()) }; - if let Some(meta_transform) = handle.meta_transform() { + if let Some(meta_transform) = base_handle.meta_transform() { (*meta_transform)(&mut *meta); } match self - .load_with_meta_loader_and_reader(&path, meta, &*loader, &mut *reader, true, false) + .load_with_meta_loader_and_reader(&base_path, meta, &*loader, &mut *reader, true, false) .await { Ok(mut loaded_asset) => { + if let Some(label) = path.label_cow() { + if !loaded_asset.labeled_assets.contains_key(&label) { + return Err(AssetLoadError::MissingLabel { + base_path, + label: label.to_string(), + }); + } + } for (_, labeled_asset) in loaded_asset.labeled_assets.drain() { self.send_asset_event(InternalAssetEvent::Loaded { id: labeled_asset.handle.id(), @@ -377,13 +382,15 @@ impl AssetServer { }); } self.send_asset_event(InternalAssetEvent::Loaded { - id: base_asset_id, + id: base_handle.id(), loaded_asset, }); Ok(handle) } Err(err) => { - self.send_asset_event(InternalAssetEvent::Failed { id: base_asset_id }); + self.send_asset_event(InternalAssetEvent::Failed { + id: base_handle.id(), + }); Err(err) } } @@ -935,6 +942,11 @@ pub enum AssetLoadError { loader_name: &'static str, error: Box, }, + #[error("The file at '{base_path}' does not contain the labeled asset '{label}'.")] + MissingLabel { + base_path: AssetPath<'static>, + label: String, + }, } /// An error that occurs when an [`AssetLoader`] is not registered for a given extension. From 3d79dc4cdc3d4e56668bc314c9ee0bacb4bf0452 Mon Sep 17 00:00:00 2001 From: Nuutti Kotivuori Date: Mon, 16 Oct 2023 04:57:55 +0300 Subject: [PATCH 04/21] Unify `FixedTime` and `Time` while fixing several problems (#8964) # Objective Current `FixedTime` and `Time` have several problems. This pull aims to fix many of them at once. - If there is a longer pause between app updates, time will jump forward a lot at once and fixed time will iterate on `FixedUpdate` for a large number of steps. If the pause is merely seconds, then this will just mean jerkiness and possible unexpected behaviour in gameplay. If the pause is hours/days as with OS suspend, the game will appear to freeze until it has caught up with real time. - If calculating a fixed step takes longer than specified fixed step period, the game will enter a death spiral where rendering each frame takes longer and longer due to more and more fixed step updates being run per frame and the game appears to freeze. - There is no way to see current fixed step elapsed time inside fixed steps. In order to track this, the game designer needs to add a custom system inside `FixedUpdate` that calculates elapsed or step count in a resource. - Access to delta time inside fixed step is `FixedStep::period` rather than `Time::delta`. This, coupled with the issue that `Time::elapsed` isn't available at all for fixed steps, makes it that time requiring systems are either implemented to be run in `FixedUpdate` or `Update`, but rarely work in both. - Fixes #8800 - Fixes #8543 - Fixes #7439 - Fixes #5692 ## Solution - Create a generic `Time` clock that has no processing logic but which can be instantiated for multiple usages. This is also exposed for users to add custom clocks. - Create three standard clocks, `Time`, `Time` and `Time`, all of which contain their individual logic. - Create one "default" clock, which is just `Time` (or `Time<()>`), which will be overwritten from `Time` on each update, and `Time` inside `FixedUpdate` schedule. This way systems that do not care specifically which time they track can work both in `Update` and `FixedUpdate` without changes and the behaviour is intuitive. - Add `max_delta` to virtual time update, which limits how much can be added to virtual time by a single update. This fixes both the behaviour after a long freeze, and also the death spiral by limiting how many fixed timestep iterations there can be per update. Possible future work could be adding `max_accumulator` to add a sort of "leaky bucket" time processing to possibly smooth out jumps in time while keeping frame rate stable. - Many minor tweaks and clarifications to the time functions and their documentation. ## Changelog - `Time::raw_delta()`, `Time::raw_elapsed()` and related methods are moved to `Time::delta()` and `Time::elapsed()` and now match `Time` API - `FixedTime` is now `Time` and matches `Time` API. - `Time` default timestep is now 64 Hz, or 15625 microseconds. - `Time` inside `FixedUpdate` now reflects fixed timestep time, making systems portable between `Update ` and `FixedUpdate`. - `Time::pause()`, `Time::set_relative_speed()` and related methods must now be called as `Time::pause()` etc. - There is a new `max_delta` setting in `Time` that limits how much the clock can jump by a single update. The default value is 0.25 seconds. - Removed `on_fixed_timer()` condition as `on_timer()` does the right thing inside `FixedUpdate` now. ## Migration Guide - Change all `Res