Skip to content

AssetServer LoadState API consistency #15237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2024
Merged

AssetServer LoadState API consistency #15237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
260a7ae
- resovles #15098
crvarner Sep 15, 2024
b5ca400
renames to preserve original semantics of is_loaded_with_dependencies
crvarner Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 129 additions & 5 deletions crates/bevy_asset/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,13 +1095,19 @@ mod tests {

assert!(d_text.is_none());
assert!(matches!(d_load, LoadState::Failed(_)));
assert_eq!(d_deps, DependencyLoadState::Failed);
assert_eq!(d_rec_deps, RecursiveDependencyLoadState::Failed);
assert!(matches!(d_deps, DependencyLoadState::Failed(_)));
assert!(matches!(
d_rec_deps,
RecursiveDependencyLoadState::Failed(_)
));

assert_eq!(a_text.text, "a");
assert_eq!(a_load, LoadState::Loaded);
assert_eq!(a_deps, DependencyLoadState::Loaded);
assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Failed);
assert!(matches!(
a_rec_deps,
RecursiveDependencyLoadState::Failed(_)
));

assert_eq!(b_text.text, "b");
assert_eq!(b_load, LoadState::Loaded);
Expand All @@ -1110,13 +1116,131 @@ mod tests {

assert_eq!(c_text.text, "c");
assert_eq!(c_load, LoadState::Loaded);
assert_eq!(c_deps, DependencyLoadState::Failed);
assert_eq!(c_rec_deps, RecursiveDependencyLoadState::Failed);
assert!(matches!(c_deps, DependencyLoadState::Failed(_)));
assert!(matches!(
c_rec_deps,
RecursiveDependencyLoadState::Failed(_)
));

assert_eq!(asset_server.load_state(a_id), LoadState::Loaded);
assert_eq!(
asset_server.dependency_load_state(a_id),
DependencyLoadState::Loaded
);
assert!(matches!(
asset_server.recursive_dependency_load_state(a_id),
RecursiveDependencyLoadState::Failed(_)
));

assert!(asset_server.is_loaded(a_id));
assert!(asset_server.is_loaded_with_dependencies(a_id));
assert!(!asset_server.is_loaded_with_recursive_dependencies(a_id));

Some(())
});
}

#[test]
fn dependency_load_states() {
// The particular usage of GatedReader in this test will cause deadlocking if running single-threaded
#[cfg(not(feature = "multi_threaded"))]
panic!("This test requires the \"multi_threaded\" feature, otherwise it will deadlock.\ncargo test --package bevy_asset --features multi_threaded");

let a_path = "a.cool.ron";
let a_ron = r#"
(
text: "a",
dependencies: [
"b.cool.ron",
"c.cool.ron",
],
embedded_dependencies: [],
sub_texts: []
)"#;
let b_path = "b.cool.ron";
let b_ron = r#"
(
text: "b",
dependencies: [],
MALFORMED
embedded_dependencies: [],
sub_texts: []
)"#;

let c_path = "c.cool.ron";
let c_ron = r#"
(
text: "c",
dependencies: [],
embedded_dependencies: [],
sub_texts: []
)"#;

let dir = Dir::default();
dir.insert_asset_text(Path::new(a_path), a_ron);
dir.insert_asset_text(Path::new(b_path), b_ron);
dir.insert_asset_text(Path::new(c_path), c_ron);

let (mut app, gate_opener) = test_app(dir);
app.init_asset::<CoolText>()
.register_asset_loader(CoolTextLoader);
let asset_server = app.world().resource::<AssetServer>().clone();
let handle: Handle<CoolText> = asset_server.load(a_path);
let a_id = handle.id();
app.world_mut().spawn(handle);

gate_opener.open(a_path);
run_app_until(&mut app, |world| {
let _a_text = get::<CoolText>(world, a_id)?;
let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert_eq!(a_load, LoadState::Loaded);
assert_eq!(a_deps, DependencyLoadState::Loading);
assert_eq!(a_rec_deps, RecursiveDependencyLoadState::Loading);
Some(())
});

gate_opener.open(b_path);
run_app_until(&mut app, |world| {
let a_text = get::<CoolText>(world, a_id)?;
let b_id = a_text.dependencies[0].id();

let (b_load, _b_deps, _b_rec_deps) = asset_server.get_load_states(b_id).unwrap();
if !matches!(b_load, LoadState::Failed(_)) {
// wait until b fails
return None;
}

let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert_eq!(a_load, LoadState::Loaded);
assert!(matches!(a_deps, DependencyLoadState::Failed(_)));
assert!(matches!(
a_rec_deps,
RecursiveDependencyLoadState::Failed(_)
));
Some(())
});

gate_opener.open(c_path);
run_app_until(&mut app, |world| {
let a_text = get::<CoolText>(world, a_id)?;
let c_id = a_text.dependencies[1].id();
// wait until c loads
let _c_text = get::<CoolText>(world, c_id)?;

let (a_load, a_deps, a_rec_deps) = asset_server.get_load_states(a_id).unwrap();
assert_eq!(a_load, LoadState::Loaded);
assert!(
matches!(a_deps, DependencyLoadState::Failed(_)),
"Successful dependency load should not overwrite a previous failure"
);
assert!(
matches!(a_rec_deps, RecursiveDependencyLoadState::Failed(_)),
"Successful dependency load should not overwrite a previous failure"
);
Some(())
});
}

const SIMPLE_TEXT: &str = r#"
(
text: "dep",
Expand Down
47 changes: 31 additions & 16 deletions crates/bevy_asset/src/server/info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,8 +388,10 @@ impl AssetInfos {
loaded_asset.value.insert(loaded_asset_id, world);
let mut loading_deps = loaded_asset.dependencies;
let mut failed_deps = HashSet::new();
let mut dep_error = None;
let mut loading_rec_deps = loading_deps.clone();
let mut failed_rec_deps = HashSet::new();
let mut rec_dep_error = None;
loading_deps.retain(|dep_id| {
if let Some(dep_info) = self.get_mut(*dep_id) {
match dep_info.rec_dep_load_state {
Expand All @@ -404,7 +406,10 @@ impl AssetInfos {
// If dependency is loaded, reduce our count by one
loading_rec_deps.remove(dep_id);
}
RecursiveDependencyLoadState::Failed => {
RecursiveDependencyLoadState::Failed(ref error) => {
if rec_dep_error.is_none() {
rec_dep_error = Some(error.clone());
}
failed_rec_deps.insert(*dep_id);
loading_rec_deps.remove(dep_id);
}
Expand All @@ -419,7 +424,10 @@ impl AssetInfos {
// If dependency is loaded, reduce our count by one
false
}
LoadState::Failed(_) => {
LoadState::Failed(ref error) => {
if dep_error.is_none() {
dep_error = Some(error.clone());
}
failed_deps.insert(*dep_id);
false
}
Expand All @@ -437,7 +445,7 @@ impl AssetInfos {
let dep_load_state = match (loading_deps.len(), failed_deps.len()) {
(0, 0) => DependencyLoadState::Loaded,
(_loading, 0) => DependencyLoadState::Loading,
(_loading, _failed) => DependencyLoadState::Failed,
(_loading, _failed) => DependencyLoadState::Failed(dep_error.unwrap()),
};

let rec_dep_load_state = match (loading_rec_deps.len(), failed_rec_deps.len()) {
Expand All @@ -450,7 +458,7 @@ impl AssetInfos {
RecursiveDependencyLoadState::Loaded
}
(_loading, 0) => RecursiveDependencyLoadState::Loading,
(_loading, _failed) => RecursiveDependencyLoadState::Failed,
(_loading, _failed) => RecursiveDependencyLoadState::Failed(rec_dep_error.unwrap()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To preserve the previous behavior, I separated the errors based on where they were found. I'm not sure this makes sense though. In general, the interaction between DependencyLoadState and RecursiveDependencyLoadState is pretty confusing.

IMO we should simplify to just LoadState and DependencyLoadState (which are affected by their own dependencies recursively) unless there is a clear use case for distinguishing between deps and recursive deps.

Copy link
Member

Choose a reason for hiding this comment

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

Totally agree. Follow-up though :)

};

let (dependants_waiting_on_load, dependants_waiting_on_rec_load) = {
Expand Down Expand Up @@ -480,14 +488,14 @@ impl AssetInfos {
info.failed_rec_dependencies = failed_rec_deps;
info.load_state = LoadState::Loaded;
info.dep_load_state = dep_load_state;
info.rec_dep_load_state = rec_dep_load_state;
info.rec_dep_load_state = rec_dep_load_state.clone();
if watching_for_changes {
info.loader_dependencies = loaded_asset.loader_dependencies;
}

let dependants_waiting_on_rec_load = if matches!(
rec_dep_load_state,
RecursiveDependencyLoadState::Loaded | RecursiveDependencyLoadState::Failed
RecursiveDependencyLoadState::Loaded | RecursiveDependencyLoadState::Failed(_)
) {
Some(std::mem::take(
&mut info.dependants_waiting_on_recursive_dep_load,
Expand All @@ -505,7 +513,9 @@ impl AssetInfos {
for id in dependants_waiting_on_load {
if let Some(info) = self.get_mut(id) {
info.loading_dependencies.remove(&loaded_asset_id);
if info.loading_dependencies.is_empty() {
if info.loading_dependencies.is_empty()
&& !matches!(info.dep_load_state, DependencyLoadState::Failed(_))
{
// send dependencies loaded event
info.dep_load_state = DependencyLoadState::Loaded;
}
Expand All @@ -519,9 +529,9 @@ impl AssetInfos {
Self::propagate_loaded_state(self, loaded_asset_id, dep_id, sender);
}
}
RecursiveDependencyLoadState::Failed => {
RecursiveDependencyLoadState::Failed(ref error) => {
for dep_id in dependants_waiting_on_rec_load {
Self::propagate_failed_state(self, loaded_asset_id, dep_id);
Self::propagate_failed_state(self, loaded_asset_id, dep_id, error);
}
}
RecursiveDependencyLoadState::Loading | RecursiveDependencyLoadState::NotLoaded => {
Expand Down Expand Up @@ -570,11 +580,12 @@ impl AssetInfos {
infos: &mut AssetInfos,
failed_id: UntypedAssetId,
waiting_id: UntypedAssetId,
error: &Arc<AssetLoadError>,
) {
let dependants_waiting_on_rec_load = if let Some(info) = infos.get_mut(waiting_id) {
info.loading_rec_dependencies.remove(&failed_id);
info.failed_rec_dependencies.insert(failed_id);
info.rec_dep_load_state = RecursiveDependencyLoadState::Failed;
info.rec_dep_load_state = RecursiveDependencyLoadState::Failed(error.clone());
Some(std::mem::take(
&mut info.dependants_waiting_on_recursive_dep_load,
))
Expand All @@ -584,7 +595,7 @@ impl AssetInfos {

if let Some(dependants_waiting_on_rec_load) = dependants_waiting_on_rec_load {
for dep_id in dependants_waiting_on_rec_load {
Self::propagate_failed_state(infos, waiting_id, dep_id);
Self::propagate_failed_state(infos, waiting_id, dep_id, error);
}
}
}
Expand All @@ -595,14 +606,15 @@ impl AssetInfos {
return;
}

let error = Arc::new(error);
let (dependants_waiting_on_load, dependants_waiting_on_rec_load) = {
let Some(info) = self.get_mut(failed_id) else {
// The asset was already dropped.
return;
};
info.load_state = LoadState::Failed(Box::new(error));
info.dep_load_state = DependencyLoadState::Failed;
info.rec_dep_load_state = RecursiveDependencyLoadState::Failed;
info.load_state = LoadState::Failed(error.clone());
info.dep_load_state = DependencyLoadState::Failed(error.clone());
info.rec_dep_load_state = RecursiveDependencyLoadState::Failed(error.clone());
(
std::mem::take(&mut info.dependants_waiting_on_load),
std::mem::take(&mut info.dependants_waiting_on_recursive_dep_load),
Expand All @@ -613,12 +625,15 @@ impl AssetInfos {
if let Some(info) = self.get_mut(waiting_id) {
info.loading_dependencies.remove(&failed_id);
info.failed_dependencies.insert(failed_id);
info.dep_load_state = DependencyLoadState::Failed;
// don't overwrite DependencyLoadState if already failed to preserve first error
if !(matches!(info.dep_load_state, DependencyLoadState::Failed(_))) {
info.dep_load_state = DependencyLoadState::Failed(error.clone());
}
}
}

for waiting_id in dependants_waiting_on_rec_load {
Self::propagate_failed_state(self, failed_id, waiting_id);
Self::propagate_failed_state(self, failed_id, waiting_id, &error);
}
}

Expand Down
Loading