From 723b52abd30f3fee9b9df16163f72be65bf9362d Mon Sep 17 00:00:00 2001 From: andriyDev Date: Tue, 3 Jun 2025 17:00:32 -0700 Subject: [PATCH] Allow returning an error from `labeled_asset_scope`. (#19449) # Objective - `LoadContext::labeled_asset_scope` cannot return errors back to the asset loader. This means users that need errors need to fall back to using the raw `begin_labeled_asset` and `add_loaded_labeled_asset`, which is more error-prone. ## Solution - Allow returning a (generic) error from `labeled_asset_scope`. - This has the unfortunate side effect that closures which don't return any errors need to A) return Ok at the end, B) need to specify an error type (e.g., `()`). --- ## Showcase ```rust // impl AssetLoader for MyLoader let handle = load_context.labeled_asset_scope("MySubasset", |mut load_context| { if !some_precondition { return Err(ThingsDontMakeSenseError); } let handle = load_context.add_labeled_asset("MySubasset/Other", SomeOtherThing(456)); Ok(Something{ id: 123, handle }) })?; ``` --- crates/bevy_asset/src/loader.rs | 13 +- crates/bevy_gltf/src/loader/mod.rs | 390 +++++++++--------- .../labeled_asset_scope_errors.md | 31 ++ 3 files changed, 237 insertions(+), 197 deletions(-) create mode 100644 release-content/migration-guides/labeled_asset_scope_errors.md diff --git a/crates/bevy_asset/src/loader.rs b/crates/bevy_asset/src/loader.rs index 0b7fd084ae..50bbfb5dfc 100644 --- a/crates/bevy_asset/src/loader.rs +++ b/crates/bevy_asset/src/loader.rs @@ -388,15 +388,15 @@ impl<'a> LoadContext<'a> { /// result with [`LoadContext::add_labeled_asset`]. /// /// See [`AssetPath`] for more on labeled assets. - pub fn labeled_asset_scope( + pub fn labeled_asset_scope( &mut self, label: String, - load: impl FnOnce(&mut LoadContext) -> A, - ) -> Handle { + load: impl FnOnce(&mut LoadContext) -> Result, + ) -> Result, E> { let mut context = self.begin_labeled_asset(); - let asset = load(&mut context); + let asset = load(&mut context)?; let loaded_asset = context.finish(asset); - self.add_loaded_labeled_asset(label, loaded_asset) + Ok(self.add_loaded_labeled_asset(label, loaded_asset)) } /// This will add the given `asset` as a "labeled [`Asset`]" with the `label` label. @@ -410,7 +410,8 @@ impl<'a> LoadContext<'a> { /// /// See [`AssetPath`] for more on labeled assets. pub fn add_labeled_asset(&mut self, label: String, asset: A) -> Handle { - self.labeled_asset_scope(label, |_| asset) + self.labeled_asset_scope(label, |_| Ok::<_, ()>(asset)) + .expect("the closure returns Ok") } /// Add a [`LoadedAsset`] that is a "labeled sub asset" of the root path of this load context. diff --git a/crates/bevy_gltf/src/loader/mod.rs b/crates/bevy_gltf/src/loader/mod.rs index 9bdeb23f26..abc555002a 100644 --- a/crates/bevy_gltf/src/loader/mod.rs +++ b/crates/bevy_gltf/src/loader/mod.rs @@ -1043,71 +1043,75 @@ fn load_material( is_scale_inverted: bool, ) -> Handle { let material_label = material_label(material, is_scale_inverted); - load_context.labeled_asset_scope(material_label.to_string(), |load_context| { - let pbr = material.pbr_metallic_roughness(); + load_context + .labeled_asset_scope::<_, ()>(material_label.to_string(), |load_context| { + let pbr = material.pbr_metallic_roughness(); - // TODO: handle missing label handle errors here? - let color = pbr.base_color_factor(); - let base_color_channel = pbr - .base_color_texture() - .map(|info| uv_channel(material, "base color", info.tex_coord())) - .unwrap_or_default(); - let base_color_texture = pbr - .base_color_texture() - .map(|info| texture_handle(&info.texture(), load_context)); + // TODO: handle missing label handle errors here? + let color = pbr.base_color_factor(); + let base_color_channel = pbr + .base_color_texture() + .map(|info| uv_channel(material, "base color", info.tex_coord())) + .unwrap_or_default(); + let base_color_texture = pbr + .base_color_texture() + .map(|info| texture_handle(&info.texture(), load_context)); - let uv_transform = pbr - .base_color_texture() - .and_then(|info| info.texture_transform().map(texture_transform_to_affine2)) - .unwrap_or_default(); + let uv_transform = pbr + .base_color_texture() + .and_then(|info| info.texture_transform().map(texture_transform_to_affine2)) + .unwrap_or_default(); - let normal_map_channel = material - .normal_texture() - .map(|info| uv_channel(material, "normal map", info.tex_coord())) - .unwrap_or_default(); - let normal_map_texture: Option> = - material.normal_texture().map(|normal_texture| { - // TODO: handle normal_texture.scale - texture_handle(&normal_texture.texture(), load_context) + let normal_map_channel = material + .normal_texture() + .map(|info| uv_channel(material, "normal map", info.tex_coord())) + .unwrap_or_default(); + let normal_map_texture: Option> = + material.normal_texture().map(|normal_texture| { + // TODO: handle normal_texture.scale + texture_handle(&normal_texture.texture(), load_context) + }); + + let metallic_roughness_channel = pbr + .metallic_roughness_texture() + .map(|info| uv_channel(material, "metallic/roughness", info.tex_coord())) + .unwrap_or_default(); + let metallic_roughness_texture = pbr.metallic_roughness_texture().map(|info| { + warn_on_differing_texture_transforms( + material, + &info, + uv_transform, + "metallic/roughness", + ); + texture_handle(&info.texture(), load_context) }); - let metallic_roughness_channel = pbr - .metallic_roughness_texture() - .map(|info| uv_channel(material, "metallic/roughness", info.tex_coord())) - .unwrap_or_default(); - let metallic_roughness_texture = pbr.metallic_roughness_texture().map(|info| { - warn_on_differing_texture_transforms( - material, - &info, - uv_transform, - "metallic/roughness", - ); - texture_handle(&info.texture(), load_context) - }); + let occlusion_channel = material + .occlusion_texture() + .map(|info| uv_channel(material, "occlusion", info.tex_coord())) + .unwrap_or_default(); + let occlusion_texture = material.occlusion_texture().map(|occlusion_texture| { + // TODO: handle occlusion_texture.strength() (a scalar multiplier for occlusion strength) + texture_handle(&occlusion_texture.texture(), load_context) + }); - let occlusion_channel = material - .occlusion_texture() - .map(|info| uv_channel(material, "occlusion", info.tex_coord())) - .unwrap_or_default(); - let occlusion_texture = material.occlusion_texture().map(|occlusion_texture| { - // TODO: handle occlusion_texture.strength() (a scalar multiplier for occlusion strength) - texture_handle(&occlusion_texture.texture(), load_context) - }); + let emissive = material.emissive_factor(); + let emissive_channel = material + .emissive_texture() + .map(|info| uv_channel(material, "emissive", info.tex_coord())) + .unwrap_or_default(); + let emissive_texture = material.emissive_texture().map(|info| { + // TODO: handle occlusion_texture.strength() (a scalar multiplier for occlusion strength) + warn_on_differing_texture_transforms(material, &info, uv_transform, "emissive"); + texture_handle(&info.texture(), load_context) + }); - let emissive = material.emissive_factor(); - let emissive_channel = material - .emissive_texture() - .map(|info| uv_channel(material, "emissive", info.tex_coord())) - .unwrap_or_default(); - let emissive_texture = material.emissive_texture().map(|info| { - // TODO: handle occlusion_texture.strength() (a scalar multiplier for occlusion strength) - warn_on_differing_texture_transforms(material, &info, uv_transform, "emissive"); - texture_handle(&info.texture(), load_context) - }); - - #[cfg(feature = "pbr_transmission_textures")] - let (specular_transmission, specular_transmission_channel, specular_transmission_texture) = - material + #[cfg(feature = "pbr_transmission_textures")] + let ( + specular_transmission, + specular_transmission_channel, + specular_transmission_texture, + ) = material .transmission() .map_or((0.0, UvChannel::Uv0, None), |transmission| { let specular_transmission_channel = transmission @@ -1127,152 +1131,156 @@ fn load_material( ) }); - #[cfg(not(feature = "pbr_transmission_textures"))] - let specular_transmission = material - .transmission() - .map_or(0.0, |transmission| transmission.transmission_factor()); + #[cfg(not(feature = "pbr_transmission_textures"))] + let specular_transmission = material + .transmission() + .map_or(0.0, |transmission| transmission.transmission_factor()); - #[cfg(feature = "pbr_transmission_textures")] - let ( - thickness, - thickness_channel, - thickness_texture, - attenuation_distance, - attenuation_color, - ) = material.volume().map_or( - (0.0, UvChannel::Uv0, None, f32::INFINITY, [1.0, 1.0, 1.0]), - |volume| { - let thickness_channel = volume - .thickness_texture() - .map(|info| uv_channel(material, "thickness", info.tex_coord())) - .unwrap_or_default(); - let thickness_texture: Option> = - volume.thickness_texture().map(|thickness_texture| { - texture_handle(&thickness_texture.texture(), load_context) - }); + #[cfg(feature = "pbr_transmission_textures")] + let ( + thickness, + thickness_channel, + thickness_texture, + attenuation_distance, + attenuation_color, + ) = material.volume().map_or( + (0.0, UvChannel::Uv0, None, f32::INFINITY, [1.0, 1.0, 1.0]), + |volume| { + let thickness_channel = volume + .thickness_texture() + .map(|info| uv_channel(material, "thickness", info.tex_coord())) + .unwrap_or_default(); + let thickness_texture: Option> = + volume.thickness_texture().map(|thickness_texture| { + texture_handle(&thickness_texture.texture(), load_context) + }); - ( - volume.thickness_factor(), - thickness_channel, - thickness_texture, - volume.attenuation_distance(), - volume.attenuation_color(), - ) - }, - ); - - #[cfg(not(feature = "pbr_transmission_textures"))] - let (thickness, attenuation_distance, attenuation_color) = - material - .volume() - .map_or((0.0, f32::INFINITY, [1.0, 1.0, 1.0]), |volume| { ( volume.thickness_factor(), + thickness_channel, + thickness_texture, volume.attenuation_distance(), volume.attenuation_color(), ) - }); + }, + ); - let ior = material.ior().unwrap_or(1.5); + #[cfg(not(feature = "pbr_transmission_textures"))] + let (thickness, attenuation_distance, attenuation_color) = + material + .volume() + .map_or((0.0, f32::INFINITY, [1.0, 1.0, 1.0]), |volume| { + ( + volume.thickness_factor(), + volume.attenuation_distance(), + volume.attenuation_color(), + ) + }); - // Parse the `KHR_materials_clearcoat` extension data if necessary. - let clearcoat = - ClearcoatExtension::parse(load_context, document, material).unwrap_or_default(); + let ior = material.ior().unwrap_or(1.5); - // Parse the `KHR_materials_anisotropy` extension data if necessary. - let anisotropy = - AnisotropyExtension::parse(load_context, document, material).unwrap_or_default(); + // Parse the `KHR_materials_clearcoat` extension data if necessary. + let clearcoat = + ClearcoatExtension::parse(load_context, document, material).unwrap_or_default(); - // Parse the `KHR_materials_specular` extension data if necessary. - let specular = - SpecularExtension::parse(load_context, document, material).unwrap_or_default(); + // Parse the `KHR_materials_anisotropy` extension data if necessary. + let anisotropy = + AnisotropyExtension::parse(load_context, document, material).unwrap_or_default(); - // We need to operate in the Linear color space and be willing to exceed 1.0 in our channels - let base_emissive = LinearRgba::rgb(emissive[0], emissive[1], emissive[2]); - let emissive = base_emissive * material.emissive_strength().unwrap_or(1.0); + // Parse the `KHR_materials_specular` extension data if necessary. + let specular = + SpecularExtension::parse(load_context, document, material).unwrap_or_default(); - StandardMaterial { - base_color: Color::linear_rgba(color[0], color[1], color[2], color[3]), - base_color_channel, - base_color_texture, - perceptual_roughness: pbr.roughness_factor(), - metallic: pbr.metallic_factor(), - metallic_roughness_channel, - metallic_roughness_texture, - normal_map_channel, - normal_map_texture, - double_sided: material.double_sided(), - cull_mode: if material.double_sided() { - None - } else if is_scale_inverted { - Some(Face::Front) - } else { - Some(Face::Back) - }, - occlusion_channel, - occlusion_texture, - emissive, - emissive_channel, - emissive_texture, - specular_transmission, - #[cfg(feature = "pbr_transmission_textures")] - specular_transmission_channel, - #[cfg(feature = "pbr_transmission_textures")] - specular_transmission_texture, - thickness, - #[cfg(feature = "pbr_transmission_textures")] - thickness_channel, - #[cfg(feature = "pbr_transmission_textures")] - thickness_texture, - ior, - attenuation_distance, - attenuation_color: Color::linear_rgb( - attenuation_color[0], - attenuation_color[1], - attenuation_color[2], - ), - unlit: material.unlit(), - alpha_mode: alpha_mode(material), - uv_transform, - clearcoat: clearcoat.clearcoat_factor.unwrap_or_default() as f32, - clearcoat_perceptual_roughness: clearcoat.clearcoat_roughness_factor.unwrap_or_default() - as f32, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_channel: clearcoat.clearcoat_channel, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_texture: clearcoat.clearcoat_texture, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_roughness_channel: clearcoat.clearcoat_roughness_channel, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_roughness_texture: clearcoat.clearcoat_roughness_texture, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_normal_channel: clearcoat.clearcoat_normal_channel, - #[cfg(feature = "pbr_multi_layer_material_textures")] - clearcoat_normal_texture: clearcoat.clearcoat_normal_texture, - anisotropy_strength: anisotropy.anisotropy_strength.unwrap_or_default() as f32, - anisotropy_rotation: anisotropy.anisotropy_rotation.unwrap_or_default() as f32, - #[cfg(feature = "pbr_anisotropy_texture")] - anisotropy_channel: anisotropy.anisotropy_channel, - #[cfg(feature = "pbr_anisotropy_texture")] - anisotropy_texture: anisotropy.anisotropy_texture, - // From the `KHR_materials_specular` spec: - // - reflectance: specular.specular_factor.unwrap_or(1.0) as f32 * 0.5, - #[cfg(feature = "pbr_specular_textures")] - specular_channel: specular.specular_channel, - #[cfg(feature = "pbr_specular_textures")] - specular_texture: specular.specular_texture, - specular_tint: match specular.specular_color_factor { - Some(color) => Color::linear_rgb(color[0] as f32, color[1] as f32, color[2] as f32), - None => Color::WHITE, - }, - #[cfg(feature = "pbr_specular_textures")] - specular_tint_channel: specular.specular_color_channel, - #[cfg(feature = "pbr_specular_textures")] - specular_tint_texture: specular.specular_color_texture, - ..Default::default() - } - }) + // We need to operate in the Linear color space and be willing to exceed 1.0 in our channels + let base_emissive = LinearRgba::rgb(emissive[0], emissive[1], emissive[2]); + let emissive = base_emissive * material.emissive_strength().unwrap_or(1.0); + + Ok(StandardMaterial { + base_color: Color::linear_rgba(color[0], color[1], color[2], color[3]), + base_color_channel, + base_color_texture, + perceptual_roughness: pbr.roughness_factor(), + metallic: pbr.metallic_factor(), + metallic_roughness_channel, + metallic_roughness_texture, + normal_map_channel, + normal_map_texture, + double_sided: material.double_sided(), + cull_mode: if material.double_sided() { + None + } else if is_scale_inverted { + Some(Face::Front) + } else { + Some(Face::Back) + }, + occlusion_channel, + occlusion_texture, + emissive, + emissive_channel, + emissive_texture, + specular_transmission, + #[cfg(feature = "pbr_transmission_textures")] + specular_transmission_channel, + #[cfg(feature = "pbr_transmission_textures")] + specular_transmission_texture, + thickness, + #[cfg(feature = "pbr_transmission_textures")] + thickness_channel, + #[cfg(feature = "pbr_transmission_textures")] + thickness_texture, + ior, + attenuation_distance, + attenuation_color: Color::linear_rgb( + attenuation_color[0], + attenuation_color[1], + attenuation_color[2], + ), + unlit: material.unlit(), + alpha_mode: alpha_mode(material), + uv_transform, + clearcoat: clearcoat.clearcoat_factor.unwrap_or_default() as f32, + clearcoat_perceptual_roughness: clearcoat + .clearcoat_roughness_factor + .unwrap_or_default() as f32, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_channel: clearcoat.clearcoat_channel, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_texture: clearcoat.clearcoat_texture, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_roughness_channel: clearcoat.clearcoat_roughness_channel, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_roughness_texture: clearcoat.clearcoat_roughness_texture, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_normal_channel: clearcoat.clearcoat_normal_channel, + #[cfg(feature = "pbr_multi_layer_material_textures")] + clearcoat_normal_texture: clearcoat.clearcoat_normal_texture, + anisotropy_strength: anisotropy.anisotropy_strength.unwrap_or_default() as f32, + anisotropy_rotation: anisotropy.anisotropy_rotation.unwrap_or_default() as f32, + #[cfg(feature = "pbr_anisotropy_texture")] + anisotropy_channel: anisotropy.anisotropy_channel, + #[cfg(feature = "pbr_anisotropy_texture")] + anisotropy_texture: anisotropy.anisotropy_texture, + // From the `KHR_materials_specular` spec: + // + reflectance: specular.specular_factor.unwrap_or(1.0) as f32 * 0.5, + #[cfg(feature = "pbr_specular_textures")] + specular_channel: specular.specular_channel, + #[cfg(feature = "pbr_specular_textures")] + specular_texture: specular.specular_texture, + specular_tint: match specular.specular_color_factor { + Some(color) => { + Color::linear_rgb(color[0] as f32, color[1] as f32, color[2] as f32) + } + None => Color::WHITE, + }, + #[cfg(feature = "pbr_specular_textures")] + specular_tint_channel: specular.specular_color_channel, + #[cfg(feature = "pbr_specular_textures")] + specular_tint_texture: specular.specular_color_texture, + ..Default::default() + }) + }) + .unwrap() } /// Loads a glTF node. diff --git a/release-content/migration-guides/labeled_asset_scope_errors.md b/release-content/migration-guides/labeled_asset_scope_errors.md new file mode 100644 index 0000000000..4acfa42816 --- /dev/null +++ b/release-content/migration-guides/labeled_asset_scope_errors.md @@ -0,0 +1,31 @@ +--- +title: `labeled_asset_scope` can now return errors. +pull_requests: [19449] +--- + +`labeled_asset_scope` now returns a user-specified error type based on their closure. Previously, +users would need to fall back to `begin_labeled_asset` and `add_loaded_labeled_asset` to handle +errors, which is more error-prone. Consider migrating to use `labeled_asset_scope` if this was you! + +However, `labeled_asset_scope` closures that don't return errors now needs to A) return Ok, and B) +specify an error type. + +If your code previously looked like this: + +```rust +labeled_asset_scope(label, |mut load_context| { + let my_asset = ...; + + my_asset +}); +``` + +You can migrate it to: + +```rust +labeled_asset_scope::<_, ()>(label, |mut load_context| { + let my_asset = ...; + + Ok(my_asset) +}).unwrap(); +```