AssetServer LoadState API consistency #15237

crvarner · 2024-09-15T19:56:50Z

Objective

implements consistently named AssertServer methods for asset, dependency, and recursive dependency load states
returns relevant LoadState when required, including error information for failed loads
resolves AssetServer::is_loaded_with_dependencies should return a LoadState. #15098

Solution

implement consistently named LoadState accessor methods:
- load_state, dependency_load_state, recursive_dependency_load_state (return unwrapped load states)
- get_load_state, get_dependency_load_state, get_recursive_dependency_load_state (return Option)
- is_loaded, is_loaded_with_dependencies, is_loaded_with_recursive_dependencies (return bool)
adds AssetLoadError to DependencyLoadState::Failed and RecursiveDependencyLoadState::Failed

Testing

Added coverage to existing unit tests

crates/bevy_asset/src/server/info.rs

github-actions · 2024-09-15T20:17:51Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

alice-i-cecile · 2024-09-15T20:18:31Z

What else needs to be done before this is ready for review?

crvarner · 2024-09-15T20:21:29Z

I usually just wait for checks to pass first :)

crvarner · 2024-09-15T22:39:42Z

crates/bevy_asset/src/server/info.rs

@@ -506,7 +514,7 @@ impl AssetInfos {
            if let Some(info) = self.get_mut(id) {
                info.loading_dependencies.remove(&loaded_asset_id);
                if info.loading_dependencies.is_empty() {
-                    // send dependencies loaded event
+                    // send dependencies loaded event (wouldn't this overwrite previous Failed state?)


This seems broken as is. Example: A depends on B and C.

B fails to load.

Line 624: B removed from A.loading_dependencies and

Line 628: A.dep_load_state is set to Failed

C successfully loads

Line 515: C removed from A.loading_dependencies (now empty)

Line 518: A.dep_load_state overwritten to Loaded

Added a new unit test that confirmed the previous impl was overwriting previously failed dependencies. Added this check to fix it.

crvarner · 2024-09-15T23:05:11Z

crates/bevy_asset/src/server/info.rs

-                    info.dep_load_state = DependencyLoadState::Loaded;
+                    if !matches!(info.dep_load_state, DependencyLoadState::Failed(_)) {
+                        // send dependencies loaded event
+                        info.dep_load_state = DependencyLoadState::Loaded;


prior to this change, the added test dependency_load_states would fail due to a successful dependency load overwriting a previous failed dependency load.

crvarner · 2024-09-15T23:10:02Z

crates/bevy_asset/src/server/info.rs

@@ -450,7 +458,7 @@ impl AssetInfos {
                RecursiveDependencyLoadState::Loaded
            }
            (_loading, 0) => RecursiveDependencyLoadState::Loading,
-            (_loading, _failed) => RecursiveDependencyLoadState::Failed,
+            (_loading, _failed) => RecursiveDependencyLoadState::Failed(rec_dep_error.unwrap()),


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.

Totally agree. Follow-up though :)

crates/bevy_asset/src/lib.rs

crates/bevy_asset/src/server/mod.rs

alice-i-cecile

A couple of cleanup items, but I'm on board with this. I'm generally in favor of the error-handling cleanup you've mentioned as well, but let's keep this PR scoped.

I think I would prefer a Vec of errors, but I'll defer to you on whether to do that in this PR or follow-up.

crvarner · 2024-09-16T14:17:59Z

I think I would prefer a Vec of errors, but I'll defer to you on whether to do that in this PR or follow-up.

I also think a Vec would be better, but don’t see a super logical way to implement it without either the error handling cleanup going in first or a better understand of the interaction bwtween deps and rec_deps.

alice-i-cecile · 2024-09-16T14:39:46Z

KK, let's get this merged first, then clean up the error handling, then move to a vec or hashmap of errors :)

- implements consistently named AssertServer methods for asset, dependency, and recursive dependency load states - adds error to DependencyLoadState::Failed and RecursiveDependencyLoadState::Failed - fixes bug where successful dependency load overwrites previous dependency failed load state

crvarner · 2024-09-16T18:29:33Z

cleanup items resolved :)

janhohenheim · 2024-09-19T11:40:52Z

crates/bevy_asset/src/server/mod.rs

+        matches!(self.load_state(id), LoadState::Loaded)
+    }
+
+    /// Convenience method that returns true if the asset and all of its direct dependencies have been loaded.
    pub fn is_loaded_with_dependencies(&self, id: impl Into<UntypedAssetId>) -> bool {


Am I seeing it right that current calls to is_loaded_with_dependencies now has different semantics and that users should replace it with is_loaded_with_recursive_dependencies to keep their code working as before? If so, this seems like an incredibly silent footgun. Users upgrading their version without looking at the migration guides will have no idea that their code is now slightly wrong.
If my assessment is correct, we should ensure that this cannot happen, e.g. by leaving the semantics in place and introducing a is_loaded_with_direct_dependencies. I also don't like how the variant that you will usually want, namely is_loaded_with_recursive_dependencies, has a longer / more technical name than is_loaded_with_dependencies, which is probably not what you want.
But maybe I am misunderstanding the code?

No misunderstanding, you are absolutely right. I will revert the name of is_loaded_with_recursive_dependencies to is_loaded_with_dependencies to preserve the original semantics. I will rename the check for self + direct deps to is_loaded_with_direct_dependencies

janhohenheim

Great, thanks! I think the breaking changes are minor and trivial enough that it's alright we're missing a migration guide.

crvarner · 2024-09-19T19:11:49Z

Part of me wants to rename the LoadState returning methods as well to be consistent, but I think that belongs in a follow up PR focused on this: #15237 (comment)

crvarner commented Sep 15, 2024

View reviewed changes

crates/bevy_asset/src/server/info.rs Show resolved Hide resolved

crvarner force-pushed the issue15098-asset-server-is-loaded-with-deps-return branch 2 times, most recently from 2175185 to 6b17d95 Compare September 15, 2024 20:11

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 15, 2024

crvarner marked this pull request as ready for review September 15, 2024 20:20

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 15, 2024

crvarner commented Sep 15, 2024

View reviewed changes

alice-i-cecile reviewed Sep 16, 2024

View reviewed changes

crates/bevy_asset/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 16, 2024

View reviewed changes

crates/bevy_asset/src/server/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 16, 2024

View reviewed changes

crvarner force-pushed the issue15098-asset-server-is-loaded-with-deps-return branch from 28b5e84 to 260a7ae Compare September 16, 2024 18:13

crvarner requested a review from alice-i-cecile September 16, 2024 18:29

alice-i-cecile approved these changes Sep 16, 2024

View reviewed changes

alice-i-cecile requested a review from janhohenheim September 16, 2024 20:54

janhohenheim suggested changes Sep 19, 2024

View reviewed changes

janhohenheim removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 19, 2024

janhohenheim added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 19, 2024

renames to preserve original semantics of is_loaded_with_dependencies

b5ca400

janhohenheim approved these changes Sep 19, 2024

View reviewed changes

alice-i-cecile enabled auto-merge September 19, 2024 19:10

alice-i-cecile added this pull request to the merge queue Sep 19, 2024

Merged via the queue into bevyengine:main with commit 612897b Sep 19, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AssetServer LoadState API consistency #15237

AssetServer LoadState API consistency #15237

crvarner commented Sep 15, 2024

github-actions bot commented Sep 15, 2024

alice-i-cecile commented Sep 15, 2024

crvarner commented Sep 15, 2024

crvarner Sep 15, 2024

crvarner Sep 15, 2024

crvarner Sep 15, 2024

crvarner Sep 15, 2024

alice-i-cecile Sep 16, 2024

alice-i-cecile left a comment

crvarner commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

crvarner commented Sep 16, 2024

janhohenheim Sep 19, 2024 •

edited

Loading

crvarner Sep 19, 2024

janhohenheim left a comment

crvarner commented Sep 19, 2024

AssetServer LoadState API consistency #15237

AssetServer LoadState API consistency #15237

Conversation

crvarner commented Sep 15, 2024

Objective

Solution

Testing

github-actions bot commented Sep 15, 2024

alice-i-cecile commented Sep 15, 2024

crvarner commented Sep 15, 2024

crvarner Sep 15, 2024

Choose a reason for hiding this comment

crvarner Sep 15, 2024

Choose a reason for hiding this comment

crvarner Sep 15, 2024

Choose a reason for hiding this comment

crvarner Sep 15, 2024

Choose a reason for hiding this comment

alice-i-cecile Sep 16, 2024

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

crvarner commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

crvarner commented Sep 16, 2024

janhohenheim Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

crvarner Sep 19, 2024

Choose a reason for hiding this comment

janhohenheim left a comment

Choose a reason for hiding this comment

crvarner commented Sep 19, 2024

janhohenheim Sep 19, 2024 •

edited

Loading