Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more load_direct implementations #13415

Merged
merged 12 commits into from
May 21, 2024

Conversation

ricky26
Copy link
Contributor

@ricky26 ricky26 commented May 17, 2024

Objective

Solution

  • Implements ErasedLoadedAsset::downcast and adds some accessors to LoadedAsset<A>.
  • Changes load_direct/load_direct_with_reader to be typed, and introduces load_direct_untyped/load_direct_untyped_with_reader.
  • Introduces load_direct_with_settings and load_direct_with_reader_and_settings.

Testing

  • I've run cargo test and played with the examples which use load_direct.
  • I also extended the asset_processing example to use the new typed version of load_direct and use load_direct_with_settings.

Changelog

  • Introduced new load_direct methods in LoadContext to allow specifying type & settings

Migration Guide

  • LoadContext::load_direct has been renamed to LoadContext::load_direct_untyped. You may find the new load_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 to LoadContext::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. 馃槄

@ricky26 ricky26 changed the title Feature/direct load options Add more load_direct implementations May 17, 2024
@ricky26 ricky26 force-pushed the feature/direct-load-options branch from 7d06c03 to a11bd1c Compare May 17, 2024 14:37
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 17, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a 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.

@ricky26
Copy link
Contributor Author

ricky26 commented May 18, 2024

@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 .load_direct().load() isn't my favourite), and more likely to have other issues. However, I did the simpler tidy-up in the prior commits, in case this proves to be more controversial.

@alice-i-cecile
Copy link
Member

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.

Copy link
Contributor

@bushrat011899 bushrat011899 left a 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.

crates/bevy_asset/src/loader.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/loader.rs Outdated Show resolved Hide resolved
///
/// [`Process`]: crate::processor::Process
/// [`LoadTransformAndSave`]: crate::processor::LoadTransformAndSave
pub async fn load_direct_with_settings<'b, A: Asset, S: Settings + Default>(
Copy link
Contributor

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.

Copy link
Contributor Author

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. 馃槄

Copy link
Contributor

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 馃ぃ

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 20, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 21, 2024
Merged via the queue into bevyengine:main with commit 26df1c1 May 21, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LoadContext::load_direct_with_settings or similar
3 participants