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

[Merged by Bors] - Enforce type safe usage of Handle::get #4794

Closed
wants to merge 5 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented May 18, 2022

Objective

  • Sometimes, people might load an asset as one type, then use it with an Assets for a different type.
  • See e.g. I'm having trouble getting a specific Node from a Gltf Scene Asset. #4784.
  • This is especially likely with the Gltf types, since users may not have a clear conceptual model of what types the assets will be.
  • We had an instance of this ourselves, in the scene_viewer example

Solution

  • Make Assets::get require a type safe handle.

Changelog

Changed

  • Assets::<T>::get and Assets::<T>::get_mut now require that the passed handles are Handle<T>, improving the type safety of handles.

Added

  • HandleUntyped::typed_weak, a helper function for creating a weak typed version of an exisitng HandleUntyped.

Migration Guide

Assets::<T>::get and Assets::<T>::get_mut now require that the passed handles are Handle<T>, improving the type safety of handles. If you were previously passing in:

  • a HandleId, use &Handle::weak(id) instead, to create a weak handle. You may have been able to store a type safe Handle instead.
  • a HandleUntyped, use &handle_untyped.typed_weak() to create a weak handle of the specified type. This is most likely to be the useful when using load_folder
  • a Handle<U> of of a different type, consider whether this is the correct handle type to store. If it is (i.e. the same handle id is used for multiple different Asset types) use Handle::weak(handle.id) to cast to a different type.

@DJMcNab DJMcNab added A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change 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 May 18, 2022
@alice-i-cecile
Copy link
Member

Note to reviewers: ordinarily, I would flag this flavor of major API change as Controversial. However, this has been discussed already on Discord, and the design direction has sign-off from @cart.

fn get_asset<'world>(
id: &Handle<PngAsset>,
world: &'world World,
) -> Option<&'world PngAsset> {
Copy link
Member

@alice-i-cecile alice-i-cecile May 18, 2022

Choose a reason for hiding this comment

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

Note to reviewers: this is part of a test suite. Hence the overly broad name for a specific asset type.

I would probably rename the method here, but I don't feel strongly about it.

@@ -369,7 +370,7 @@ impl From<Color> for UiColor {
}

/// The image of the node
#[derive(Component, Clone, Debug, Reflect)]
#[derive(Component, Clone, Debug, Reflect, Deref)]
Copy link
Member

Choose a reason for hiding this comment

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

We should have DerefMut to match IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(Component, Clone, Debug, Reflect, Deref)]
#[derive(Component, Clone, Debug, Reflect, Deref, DerefMut)]

Copy link
Member Author

Choose a reason for hiding this comment

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

We should have a full review of which things internally should use the Deref derives, I guess.

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.

I'm really impressed at how painless this change was. I'm strongly in favor of more type safety here.

@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 18, 2022
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.

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
# Objective

- Sometimes, people might load an asset as one type, then use it with an `Asset`s for a different type.
- See e.g. #4784. 
- This is especially likely with the Gltf types, since users may not have a clear conceptual model of what types the assets will be.
- We had an instance of this ourselves, in the `scene_viewer` example

## Solution

- Make `Assets::get` require a type safe handle.

---

## Changelog

### Changed

- `Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles.

### Added
- `HandleUntyped::typed_weak`, a helper function for creating a weak typed version of an exisitng `HandleUntyped`.

## Migration Guide

`Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles. If you were previously passing in:
   - a `HandleId`, use `&Handle::weak(id)` instead, to create a weak handle. You may have been able to store a type safe `Handle` instead.
   - a `HandleUntyped`, use `&handle_untyped.typed_weak()` to create a weak handle of the specified type. This is most likely to be the useful when using [load_folder](https://docs.rs/bevy_asset/latest/bevy_asset/struct.AssetServer.html#method.load_folder)
   - a `Handle<U>` of  of a different type, consider whether this is the correct handle type to store. If it is (i.e. the same handle id is used for multiple different Asset types) use `Handle::weak(handle.id)` to cast to a different type.
@bors bors bot changed the title Enforce type safe usage of Handle::get [Merged by Bors] - Enforce type safe usage of Handle::get May 30, 2022
@bors bors bot closed this May 30, 2022
@DJMcNab DJMcNab deleted the assets_strict branch May 30, 2022 17:58
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective

- Sometimes, people might load an asset as one type, then use it with an `Asset`s for a different type.
- See e.g. bevyengine#4784. 
- This is especially likely with the Gltf types, since users may not have a clear conceptual model of what types the assets will be.
- We had an instance of this ourselves, in the `scene_viewer` example

## Solution

- Make `Assets::get` require a type safe handle.

---

## Changelog

### Changed

- `Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles.

### Added
- `HandleUntyped::typed_weak`, a helper function for creating a weak typed version of an exisitng `HandleUntyped`.

## Migration Guide

`Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles. If you were previously passing in:
   - a `HandleId`, use `&Handle::weak(id)` instead, to create a weak handle. You may have been able to store a type safe `Handle` instead.
   - a `HandleUntyped`, use `&handle_untyped.typed_weak()` to create a weak handle of the specified type. This is most likely to be the useful when using [load_folder](https://docs.rs/bevy_asset/latest/bevy_asset/struct.AssetServer.html#method.load_folder)
   - a `Handle<U>` of  of a different type, consider whether this is the correct handle type to store. If it is (i.e. the same handle id is used for multiple different Asset types) use `Handle::weak(handle.id)` to cast to a different type.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Sometimes, people might load an asset as one type, then use it with an `Asset`s for a different type.
- See e.g. bevyengine#4784. 
- This is especially likely with the Gltf types, since users may not have a clear conceptual model of what types the assets will be.
- We had an instance of this ourselves, in the `scene_viewer` example

## Solution

- Make `Assets::get` require a type safe handle.

---

## Changelog

### Changed

- `Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles.

### Added
- `HandleUntyped::typed_weak`, a helper function for creating a weak typed version of an exisitng `HandleUntyped`.

## Migration Guide

`Assets::<T>::get` and `Assets::<T>::get_mut` now require that the passed handles are `Handle<T>`, improving the type safety of handles. If you were previously passing in:
   - a `HandleId`, use `&Handle::weak(id)` instead, to create a weak handle. You may have been able to store a type safe `Handle` instead.
   - a `HandleUntyped`, use `&handle_untyped.typed_weak()` to create a weak handle of the specified type. This is most likely to be the useful when using [load_folder](https://docs.rs/bevy_asset/latest/bevy_asset/struct.AssetServer.html#method.load_folder)
   - a `Handle<U>` of  of a different type, consider whether this is the correct handle type to store. If it is (i.e. the same handle id is used for multiple different Asset types) use `Handle::weak(handle.id)` to cast to a different type.
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-Code-Quality A section of code that is hard to understand or change 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 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
Development

Successfully merging this pull request may close these issues.

4 participants