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.

Sign up for GitHub

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

Jump to bottom

Make SavedAsset::get_labeled accept &str as label #11612

Merged
merged 11 commits into from
Jan 30, 2024
Merged

Make SavedAsset::get_labeled accept &str as label #11612

merged 11 commits into from
Jan 30, 2024

Conversation

RyanSpaker
Copy link
Contributor

@RyanSpaker RyanSpaker commented Jan 30, 2024 •
edited
Loading

Objective

  • SavedAsset's iter_labels returns &str, however accessing LabeledAssets requires CowArc<'static, str>
  • Although SavedAsset holds UntypedHandles in its hashmap of LabeledAssets, they are inaccessible as LabeledAssets are casted to SavedAsset or ErasedLoadedAsset, which don't contain their UntypedHandles
  • Adresses SavedAsset iter_labels returning strings rather than CowArc's #11609

Solution

  • Used Trait bounds to allow for either CowArc<'static, str> or &str to be used as a label in get_labeled and get_erased_labeled.
  • Added method get_untyped_handle to get UntypedHandle from the LabeledAsset.

@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@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 targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

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
alice-i-cecile approved these changes Jan 30, 2024
View reviewed changes
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.

Looks reasonable, thanks for doing such a good job motivating the changes.

RyanSpaker and others added 2 commits January 29, 2024 19:39
@RyanSpaker @alice-i-cecile
Update crates/bevy_asset/src/saver.rs
fa3ce03 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@RyanSpaker
Fixed Documentation
d082863
tguichaoua
tguichaoua suggested changes Jan 30, 2024
View reviewed changes
crates/bevy_asset/src/saver.rs Outdated Show resolved Hide resolved
NthTensor
NthTensor reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CowArc has got to be my favorite rust-ism. LGTM pending borrow trait bound.

@RyanSpaker RyanSpaker closed this Jan 30, 2024
@RyanSpaker RyanSpaker reopened this Jan 30, 2024
@RyanSpaker
Copy link
Contributor Author

I reverted back to iterating over &str, and used the new trait bound in get_untyped_handle. I also switched get_erased_labeled and get_labeled to use the new form as well. I believe this is no longer a breaking change, so I removed the migration guide.

@alice-i-cecile alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 30, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 30, 2024 •
edited
Loading

Remember to update your PR description with the final changes: the "Solution" section is out-of-date.

@RyanSpaker RyanSpaker changed the title Make SavedAsset::iter_labels return CowArc rather than str Make SavedAsset::get_labeled accept &str as label Jan 30, 2024
@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 Jan 30, 2024
@alice-i-cecile
Copy link
Member

Thanks!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 30, 2024
Merged via the queue into bevyengine:main with commit ad0af31 Jan 30, 2024
23 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
@RyanSpaker @alice-i-cecile @tjamaan
Make SavedAsset::get_labeled accept &str as label (bevyengine#11612)
d8ebb7c 
# Objective

- SavedAsset's iter_labels returns ```&str```, however accessing
LabeledAssets requires ```CowArc<'static, str>```
- Although SavedAsset holds UntypedHandles in its hashmap of
LabeledAssets, they are inaccessible as LabeledAssets are casted to
SavedAsset or ErasedLoadedAsset, which don't contain their
UntypedHandles
- Adresses bevyengine#11609

## Solution

- Used Trait bounds to allow for either ```CowArc<'static, str>``` or
```&str``` to be used as a label in get_labeled and get_erased_labeled.
- Added method get_untyped_handle to get UntypedHandle from the
LabeledAsset.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tguichaoua tguichaoua tguichaoua requested changes

@NthTensor NthTensor NthTensor approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RyanSpaker @alice-i-cecile @NthTensor @tguichaoua