AssetServer::is_loaded_with_dependencies should return a LoadState.

[alice-i-cecile]

AssetServer::is_loaded_with_dependencies returns a bool not a LoadState.

Originally posted by @bushrat011899 in #15056 (comment)

This is much less helpful than returning failure information! When this is updated, make sure to update the linked documentation in the module docs.

[alice-i-cecile]

The main challenge here will be determining what to do with failed states: different dependencies could fail for different reasons. I think that returning the first observed failure is probably a good, easy-to-implement and easy-to-use solution.

[crvarner]

@alice-i-cecile is_ prefix naming actually makes sense to return a boolean and would be weird to return a LoadState.

Propose also renaming method to AssetServer::load_state_with_recursive_dependencies to be consistent with AssetServer::load_state and AssetServer::recursive_dependency_load_state

[alice-i-cecile]

Sure :) Make sure you make is_loaded consistent.

[crvarner]

So, currently RecursiveDependencyLoadState::Failed does not contain the AssetLoadError like LoadState (this seems to be the only difference between the two). Are you sauing you want to change this so that RecurisveDependencyLoadState::Failed contains the first error encountered and bubbles it up?

[alice-i-cecile]

Yes please. It might make sense to return a Vec of such errors instead of just the first, but I'll defer to your judgement there during implementation.

[crvarner]

Sounds good. 3 more questions lol.

1. Currently, a failed load causes the LoadState, DependencyLoadState, and RecursiveDependencyLoadState to all be set to Failed. I would have expected only the LoadState to be set to failed unless the intent is that each deeper level LoadState encompasses the parent? Not sure exactly what these states are supposed to represent in the success/fail cases.

2. I'm pretty new to Rust, is it reasonable to create an Arc<AssetLoadError> and clone it into each different LoadState level (replacing the Box in the regular LoadState)? Feels like a lot of cloning but I'm not sure what else to do without adding lifetime params to a bunch of types.

3. The API currently includes load_state and get_load_state for each level of LoadState. The former calls the latter and adds an unwrap_or:

pub fn get_load_state(&self, id: impl Into<UntypedAssetId>) -> Option<LoadState> {
    self.data
        .infos
        .read()
        .get(id.into())
        .map(|i| i.load_state.clone())
}

pub fn load_state(&self, id: impl Into<UntypedAssetId>) -> LoadState {
    self.get_load_state(id).unwrap_or(LoadState::NotLoaded)
}

Is this something we want to keep? It isn't super clear why you would want one or the other.

Sorry for all the questions!

[alice-i-cecile]

This is more complex than I anticipated! Thanks for diving in, and feel free to drop it if you feel you're in over your head.

1. I'm not sure what the intentions here are either! My expectation is that a failure should propagate upwards, but not down the tree. @cart, are you able to clarify? If we don't hear back, we should leave the semantic s there alone for this work and fix it later if that's desirable.

2. Cloning Arc's is really quite cheap: I wouldn't worry about it in a failure path, especially for something like asset loading.

3. Removing get_load_state in favor of simply load_state would be a nice change: I don't think that the Option is semantically meaningful here. That should be a new PR / issue though.

[crvarner]

Thank you, sounds good regarding 2 and 3. I’ll leave the semantics as is for 1 and look into them later if needed.

Thinking forward, I’m not sure I understand the value of the non-recursive DependencyLoadState and I also saw there are multiple types of dependencies (normal and loader) which are not distinguishable by LoadState. Both could be worth looking into if there is a semantic change in the future.

Any idea if these same asset dependencies are intended to be used as part of the bsn hierarchy?

[alice-i-cecile]

I’m not sure I understand the value of the non-recursive DependencyLoadState

Seconding this! I have a decent understanding of the high level of bevy_asset, but a lot of these details feel quite mysterious to me still, and the docs don't do enough to demystify it.

Any idea if these same asset dependencies are intended to be used as part of the bsn hierarchy?

My understanding is that asset dependencies are orthogonal to the bsn hierarchy: they're used to be able to generate the required asset, but once that's in place they won't be nested in the entity hierarchy in any way. @cart may have more solid answers to all of these questions though.