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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle<T>::as_weak() takes a type U which is potentially bad #3522

Closed
colepoirier opened this issue Jan 2, 2022 · 5 comments
Closed

Handle<T>::as_weak() takes a type U which is potentially bad #3522

colepoirier opened this issue Jan 2, 2022 · 5 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior

Comments

@colepoirier
Copy link
Contributor

On discord https://discord.com/channels/691052431525675048/749332104487108618/927078698035855420 while working on documenting bevy_assets @manokara noticed Handle::as_weak() takes a type U and wondered if "the handle would be invalid if U is different from T?"

@mockersf noted that "there's one place where that is used to convert a Handle to a Handle," and upon further investigation concluded that "the FontAtlasSet asset storage is handled manually, to have its handles in sync with fonts."

@mockersf said that this is "potentially bad and would require more thinking to improve," so I have created this issue to see what others think of the degree to which this is potentially bad and be a starting point for discussions of how to improve this.

@colepoirier colepoirier added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 2, 2022
@mockersf
Copy link
Member

mockersf commented Jan 2, 2022

let handle_font_atlas: Handle<FontAtlasSet> = section_data.0.as_weak();

In this line a Handle<Font> is changed to a Handle<FontAtlasSet>. This is used for easy correspondence between fonts assets and font atlas assets as they share the same handle

@mockersf mockersf added A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Jan 2, 2022
@manokara
Copy link
Contributor

manokara commented Jan 2, 2022

This is used for easy correspondence between fonts assets and font atlas assets as they share the same handle

Indeed. But since this is the only use case for it, I think what this method needs is a new name to make its intent more clear: transmute. And for correctness, if the handle id is HandleId::Id we should update the type uuid as well to match U's.

@cart
Copy link
Member

cart commented Jan 2, 2022

Imo we should avoid transmute for the sole reason that it is a "dirty word" in Rust generally indicating gross unsafety.

I do think its worth trying to sort out a way to make font atlas set UX reasonable without requiring handle casting.

@manokara
Copy link
Contributor

Imo we should avoid transmute for the sole reason that it is a "dirty word" in Rust generally indicating gross unsafety.

I have renamed it to cast in #3536, which is what it does and it's not as unsafe as something like transmute would imply.

I do think its worth trying to sort out a way to make font atlas set UX reasonable without requiring handle casting.

Yeah. Since this is currently the only use case for that method, I think there's two ways we could go about this:

  1. Make handle casting idiomatic and encourage that pattern if different assets share the same logic or "working space".
  2. Remove the method and rework font atlas UX as cart said.

And then we should probably rename this issue accordingly.

@alice-i-cecile alice-i-cecile moved this to Concrete and Controversial in Asset Pipeline Jul 17, 2022
bors bot pushed a commit that referenced this issue Oct 28, 2022
# Objective

Following discussion on #3536 and #3522, `Handle::as_weak()` takes a type `U`, reinterpreting the handle as of another asset type while keeping the same ID. This is mainly used today in font atlas code. This PR does two things:

- Rename the method to `cast_weak()` to make its intent more clear
- Actually change the type uuid in the handle if it's not an asset path variant.

## Migration Guide

- Rename `Handle::as_weak` uses to `Handle::cast_weak`

    The method now properly sets the associated type uuid if the handle is a direct reference (e.g. not a reference to an `AssetPath`), so adjust you code accordingly if you relied on the previous behavior.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Following discussion on bevyengine#3536 and bevyengine#3522, `Handle::as_weak()` takes a type `U`, reinterpreting the handle as of another asset type while keeping the same ID. This is mainly used today in font atlas code. This PR does two things:

- Rename the method to `cast_weak()` to make its intent more clear
- Actually change the type uuid in the handle if it's not an asset path variant.

## Migration Guide

- Rename `Handle::as_weak` uses to `Handle::cast_weak`

    The method now properly sets the associated type uuid if the handle is a direct reference (e.g. not a reference to an `AssetPath`), so adjust you code accordingly if you relied on the previous behavior.
@james7132
Copy link
Member

I can't find Handle::as_weak or Handle::cast_weak anymore. Likely was removed as a part of #8624. Closing this out.

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-Bug An unexpected or incorrect behavior
Projects
Status: Concrete and Controversial
Development

No branches or pull requests

5 participants