Skip to content

Commit

Permalink
Enforce type safe usage of Handle::get (#4794)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
DJMcNab committed May 30, 2022
1 parent a02c5ae commit 1bbd5c2
Show file tree
Hide file tree
Showing 11 changed files with 32 additions and 18 deletions.
11 changes: 7 additions & 4 deletions crates/bevy_asset/src/asset_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,11 @@ mod test {
asset_server.get_handle_untyped(id)
}

fn get_asset(id: impl Into<HandleId>, world: &World) -> Option<&PngAsset> {
world.resource::<Assets<PngAsset>>().get(id.into())
fn get_asset<'world>(
id: &Handle<PngAsset>,
world: &'world World,
) -> Option<&'world PngAsset> {
world.resource::<Assets<PngAsset>>().get(id)
}

fn get_load_state(id: impl Into<HandleId>, world: &World) -> LoadState {
Expand All @@ -800,7 +803,7 @@ mod test {
);

// load the asset
let handle = load_asset(path.clone(), &app.world);
let handle = load_asset(path.clone(), &app.world).typed();
let weak_handle = handle.clone_weak();

// asset is loading
Expand All @@ -826,7 +829,7 @@ mod test {
assert!(get_asset(&weak_handle, &app.world).is_none());

// finally, reload the asset
let handle = load_asset(path.clone(), &app.world);
let handle = load_asset(path.clone(), &app.world).typed();
assert_eq!(LoadState::Loading, get_load_state(&handle, &app.world));
app.update();
assert_eq!(LoadState::Loaded, get_load_state(&handle, &app.world));
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_asset/src/assets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,20 @@ impl<T: Asset> Assets<T> {
///
/// This is the main method for accessing asset data from an [Assets] collection. If you need
/// mutable access to the asset, use [`get_mut`](Assets::get_mut).
pub fn get<H: Into<HandleId>>(&self, handle: H) -> Option<&T> {
pub fn get(&self, handle: &Handle<T>) -> Option<&T> {
self.assets.get(&handle.into())
}

/// Checks if an asset exists for the given handle
pub fn contains<H: Into<HandleId>>(&self, handle: H) -> bool {
pub fn contains(&self, handle: &Handle<T>) -> bool {
self.assets.contains_key(&handle.into())
}

/// Get mutable access to the asset for the given handle.
///
/// This is the main method for mutably accessing asset data from an [Assets] collection. If you
/// do not need mutable access to the asset, you may also use [get](Assets::get).
pub fn get_mut<H: Into<HandleId>>(&mut self, handle: H) -> Option<&mut T> {
pub fn get_mut(&mut self, handle: &Handle<T>) -> Option<&mut T> {
let id: HandleId = handle.into();
self.events.send(AssetEvent::Modified {
handle: Handle::weak(id),
Expand Down Expand Up @@ -398,6 +398,6 @@ mod tests {
let handle = assets_before.add(MyAsset);
app.add_asset::<MyAsset>(); // Ensure this doesn't overwrite the Asset
let assets_after = app.world.resource_mut::<Assets<MyAsset>>();
assert!(assets_after.get(handle).is_some());
assert!(assets_after.get(&handle).is_some());
}
}
10 changes: 9 additions & 1 deletion crates/bevy_asset/src/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl<T: Asset> Handle<T> {
/// Makes this handle Strong if it wasn't already.
///
/// This method requires the corresponding [Assets](crate::Assets) collection
pub fn make_strong(&mut self, assets: &mut Assets<T>) {
pub fn make_strong(&mut self, assets: &Assets<T>) {
if self.is_strong() {
return;
}
Expand Down Expand Up @@ -341,6 +341,14 @@ impl HandleUntyped {
matches!(self.handle_type, HandleType::Strong(_))
}

/// Create a weak typed [`Handle`] from this handle.
///
/// If this handle is strong and dropped, there is no guarantee that the asset
/// will still be available (if only the returned handle is kept)
pub fn typed_weak<T: Asset>(&self) -> Handle<T> {
self.clone_weak().typed()
}

/// Convert this handle into a typed [Handle].
///
/// The new handle will maintain the Strong or Weak status of the current handle.
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_text/src/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl<ID: Hash + Eq> TextPipeline<ID> {
.iter()
.map(|section| {
let font = fonts
.get(section.style.font.id)
.get(&section.style.font)
.ok_or(TextError::NoSuchFont)?;
let font_id = self.get_or_insert_font_id(&section.style.font, font);
let font_size = scale_value(section.style.font_size, scale_factor);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_text/src/text2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ pub fn extract_text2d_sprite(
.color
.as_rgba_linear();
let atlas = texture_atlases
.get(text_glyph.atlas_info.texture_atlas.clone_weak())
.get(&text_glyph.atlas_info.texture_atlas)
.unwrap();
let handle = atlas.texture.clone_weak();
let index = text_glyph.atlas_info.glyph_index as usize;
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ keywords = ["bevy"]
bevy_app = { path = "../bevy_app", version = "0.8.0-dev" }
bevy_asset = { path = "../bevy_asset", version = "0.8.0-dev" }
bevy_core_pipeline = { path = "../bevy_core_pipeline", version = "0.8.0-dev" }
bevy_derive = { path = "../bevy_derive", version = "0.8.0-dev" }
bevy_ecs = { path = "../bevy_ecs", version = "0.8.0-dev" }
bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.8.0-dev" }
bevy_input = { path = "../bevy_input", version = "0.8.0-dev" }
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ui/src/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ pub fn extract_uinodes(
}
let image = image.0.clone_weak();
// Skip loading images
if !images.contains(image.clone_weak()) {
if !images.contains(&image) {
continue;
}
extracted_uinodes.uinodes.push(ExtractedUiNode {
Expand Down Expand Up @@ -196,7 +196,7 @@ pub fn extract_text_uinodes(
for text_glyph in text_glyphs {
let color = text.sections[text_glyph.section_index].style.color;
let atlas = texture_atlases
.get(text_glyph.atlas_info.texture_atlas.clone_weak())
.get(&text_glyph.atlas_info.texture_atlas)
.unwrap();
let texture = atlas.texture.clone_weak();
let index = text_glyph.atlas_info.glyph_index as usize;
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_ui/src/ui_node.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::{Size, UiRect};
use bevy_asset::Handle;
use bevy_derive::{Deref, DerefMut};
use bevy_ecs::{prelude::Component, reflect::ReflectComponent};
use bevy_math::Vec2;
use bevy_reflect::prelude::*;
Expand Down Expand Up @@ -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, DerefMut)]
#[reflect(Component, Default)]
pub struct UiImage(pub Handle<Image>);

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ui/src/widget/image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub fn image_node_system(
mut query: Query<(&mut CalculatedSize, &UiImage), With<ImageMode>>,
) {
for (mut calculated_size, image) in query.iter_mut() {
if let Some(texture) = textures.get(image.0.clone_weak()) {
if let Some(texture) = textures.get(image) {
let size = Size {
width: texture.texture_descriptor.size.width as f32,
height: texture.texture_descriptor.size.height as f32,
Expand Down
5 changes: 3 additions & 2 deletions examples/2d/texture_atlas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ fn setup(
) {
let mut texture_atlas_builder = TextureAtlasBuilder::default();
for handle in &rpg_sprite_handles.handles {
let texture = textures.get(handle).unwrap();
texture_atlas_builder.add_texture(handle.clone_weak().typed::<Image>(), texture);
let handle = handle.typed_weak();
let texture = textures.get(&handle).expect("Textures folder contained a file which way matched by a loader which did not create an `Image` asset");
texture_atlas_builder.add_texture(handle, texture);
}

let texture_atlas = texture_atlas_builder.finish(&mut textures).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion examples/tools/scene_viewer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ Controls:
}

struct SceneHandle {
handle: Handle<Scene>,
handle: Handle<Gltf>,
animations: Vec<Handle<AnimationClip>>,
instance_id: Option<InstanceId>,
is_loaded: bool,
Expand Down

0 comments on commit 1bbd5c2

Please sign in to comment.