-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Add more load_direct implementations #13415
Add more load_direct implementations #13415
Conversation
7d06c03
to
a11bd1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally very happy to see these added. I agree with you on the API explosion here 🤔 IMO we should 1) make sure to just call the most general form with null params from all of the other related methods to avoid messy/error-prone duplication 2) consider using the builder pattern in some form (load_direct(foo).with_settings(settings).with_reader(reader)
) to avoid the combinatorial explosion.
2 is just an idea: if you can't find a nice way to make the code work I won't block on it.
@alice-i-cecile I've had a go at implementing a builder-style API. I suspect this is more open to bike-shedding (in particular |
Hmm, I don't think that's an improvement 😅 Neat to see, but I would probably revert the builder experiment. I'll ask folks for more opinions / ideas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the additional flexibility this provides to end users! Just a couple of minor issues I noticed in the documentation I've also noted my personal opinion that this should probably be refactored into the builder pattern, but that is absolutely out of scope of this PR and likely contentious.
/// | ||
/// [`Process`]: crate::processor::Process | ||
/// [`LoadTransformAndSave`]: crate::processor::LoadTransformAndSave | ||
pub async fn load_direct_with_settings<'b, A: Asset, S: Settings + Default>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future Work: The LoadContext
really needs a builder API:
load
load_untyped
load_with_settings
load_direct
load_direct_untyped
load_direct_with_reader
load_direct_with_settings
load_direct_untyped_with_reader
load_direct_with_reader_and_settings
I feel like calling context.loader().direct().typed::<A>().with_settings(...).load(...)
is much friendlier to code completion, and would allow for documentation on each of the individual options in the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't considered moving the non-direct loads into the "same" builder. Good shout. It's worth noting that specifying type to load with a TypeId
but still getting an untyped asset is a useful permutation we still don't have. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's getting worse by the minute! LGTM before anyone else has any ideas 🤣
Objective
LoadContext::load_direct
which allow picking asset type & configuring settings.LoadContext::load_direct_with_settings
or similar #12963.Solution
ErasedLoadedAsset::downcast
and adds some accessors toLoadedAsset<A>
.load_direct
/load_direct_with_reader
to be typed, and introducesload_direct_untyped
/load_direct_untyped_with_reader
.load_direct_with_settings
andload_direct_with_reader_and_settings
.Testing
load_direct
.asset_processing
example to use the new typed version ofload_direct
and useload_direct_with_settings
.Changelog
load_direct
methods inLoadContext
to allow specifying type & settingsMigration Guide
LoadContext::load_direct
has been renamed toLoadContext::load_direct_untyped
. You may find the newload_direct
is more appropriate for your use case (and the migration may only be moving one type parameter).LoadContext::load_direct_with_reader
has been renamed toLoadContext::load_direct_untyped_with_reader
.This might not be an obvious win as a solution because it introduces quite a few new
load_direct
alternatives - but it does follow the existing pattern pretty well. I'm very open to alternatives. 😅