Replace the NestedLoader API with NestedLoadBuilder (matching LoadBuilder).

[andriyDev]

Objective

• Followup to Replace all different load variants in AssetServer with a builder. #23663.
• Make the nested loading API easier to understand (when reading from docs.rs).

Solution

• Remove the type-state stuff from NestedLoader. Make each way of calling load just be its own function.
• Add override_unapproved to the NestedLoader.
• Rename NestedLoader to NestedLoadBuilder (to match LoadBuilder).
• Rename .loader() to .load_builder() just like AssetServer::load_builder.

Some decisions:

• I included override_unapproved to match LoadBuilder. We could remove this to say "loaders shouldn't be able to override", but I don't think that's problematic, and I'd rather have it for completeness.
• I omitted with_guard from NestedLoadBuilder. This option doesn't really make sense, plus you could just use the async methods to do this instead.
• Since we omitted the with_guard (since it really doesn't make sense in this context), I decided not to reuse LoadBuilder in any way here. Either way, we need to deal with dependencies, so we couldn't just use it directly anyway.
• I just created all 9 load variants as separate methods. That's a lot compared to the 4 variants in LoadBuilder. We could reduce it to 6 load variants, if we kept the with_reader stuff as a type-state like we did with NestedLoader. I'm not a fan of how many variants we have, but I don't think there's a way to do this without opening up impossible configurations, like doing with_reader for a deferred call - this doesn't work!

[kfc35]

Are the lifetimes still applicable to include as part of a doc comment?

[andriyDev]

I did think about this but I decided it doesn't matter. IMO the only reason we needed docs about the lifetimes is because the usage of lifetimes was really complex, and that's in part due to the type-state stuff. Now that this is much simpler, I don't think we need to be very descriptive here. The same way how we don't need to be descriptive about the lifetimes of Query.

[kfc35]

should this be renamed to load_typed_internal to match the fn name and signature in bevy_asset server’s LoadBuilder? It might be easier to connect similarities between NestedLoadBuilder and the reg. LoadBuilder if function names and signatures match too (well, if they’re actually doing the similar/same things)

[andriyDev]

I don't want to use load_typed_internal because I already use that for load_typed_async_internal which is when we use the generic type. Either way this is private so I don't really care much to bikeshed this.

[yrns]

I only briefly looked at the other PR, but this one looks good to me. The notion of "immediate" is now much clearer with load* variants returning a handle and load*_async returning assets. The doc comments regarding "load kickoff" (LoadContext::should_load_dependencies) got lost and maybe could be reinserted for the load variants that return a handle. I assume load kickoff is irrelevant for the async variants?

[andriyDev]

The doc comments regarding "load kickoff" ... got lost

This was intentional - from the perspective of the user, whether should_load_dependencies is true or false is totally invisible: in either case they get back a handle that would refer to the asset they loaded. So I think explaining this in the doc comments makes it seem like something you should think about, but you shouldn't need to at all.

Good call out though!

I assume load kickoff is irrelevant for the async variants?

Correct! Even with should_load_dependencies = false, we need to load the dependency to return the actual data. For deferred loads, we only need a handle, which we can get without problems.

[greeble-dev]

I'm very much in favor of the direction this PR takes. I struggled to understand how to use the previous nested loader - this one is much simpler. But I think the new version is still confusing when it comes to deferred versus immediate loads.

Take load and load_async. At a glance I'd assume that load is just a synchronous version of load_async - so they do the same thing at different times. But that's not true! The important part is that load is "by reference" and load_async is "by value". These serve completely different purposes - the former is for linking separate assets together, the latter is for consuming the asset value within the loader. The fact that one is async is just a detail. I think the API should make the important part clear.

I've tried out an alternative - split the builder in two. Then one is framed as loading handles and the other is framed as loading values. Also added a convenience LoadContext::load_value. Quick and dirty test here: ef7bd02. I think this makes the behavior clearer.

-load_context.load(path);
+load_context.load_handle(path);

-load_context.load_builder().load_async(path);
+load_context.load_value(path);

-load_context.load_builder().with_settings(s).load(path);
+load_context.load_handle_builder().with_settings(s).load(path);

-load_context.load_builder().with_settings(s).load_async(path);
+load_context.load_value_builder().with_settings(s).load(path);

[greeble-dev]

Suggested change

	/// paths of assets used by the nested loader.
	/// that path as a dependency of this asset.
	pub async fn load_erased_async_from_reader<'a>(
	/// paths of assets used by the nested loader.
	pub async fn load_erased_async_from_reader<'a>(

[andriyDev]

@greeble-dev I am strongly against renaming load as load_handle. Our normal API (not nested loaders) just says load and is deferred by default. Keeping these the same seems like the correct move to me.

I am also against making separate builders - it adds more overhead to the LoadContext API, and requires us to duplicate a bunch of code. I'm already not a fan of how this duplicates the LoadBuilder, but the nested loads have different enough requirements that it makes sense. There is a minor benefit that we could make the reader be a value in load_value_builder, but I don't think it's worth the code duplication.

I'm convinced enough though to rename load_async it to load_value. I'm only weakly for this, since the fact these methods are async is important and signals we need to await them. But you're correct that the goal for a user is to get the value. This also distinguishes it from LoadBuilder::load_untyped_async which returns a handle not a value - this tipped the scales for me (they were quite balanced for me lol).