diff --git a/crates/bevy_gltf/src/convert_coordinates.rs b/crates/bevy_gltf/src/convert_coordinates.rs index e28992934815e..030da5fbdce71 100644 --- a/crates/bevy_gltf/src/convert_coordinates.rs +++ b/crates/bevy_gltf/src/convert_coordinates.rs @@ -1,34 +1,16 @@ -use core::f32::consts::PI; +//! Utilities for converting from glTF's [standard coordinate system](https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#coordinate-system-and-units) +//! to Bevy's. +use serde::{Deserialize, Serialize}; use bevy_math::{Mat4, Quat, Vec3}; use bevy_transform::components::Transform; pub(crate) trait ConvertCoordinates { - /// Converts the glTF coordinates to Bevy's coordinate system. - /// - glTF: - /// - forward: Z - /// - up: Y - /// - right: -X - /// - Bevy: - /// - forward: -Z - /// - up: Y - /// - right: X - /// - /// See + /// Converts from glTF coordinates to Bevy's coordinate system. See + /// [`GltfConvertCoordinates`] for an explanation of the conversion. fn convert_coordinates(self) -> Self; } -pub(crate) trait ConvertCameraCoordinates { - /// Like `convert_coordinates`, but uses the following for the lens rotation: - /// - forward: -Z - /// - up: Y - /// - right: X - /// - /// The same convention is used for lights. - /// See - fn convert_camera_coordinates(self) -> Self; -} - impl ConvertCoordinates for Vec3 { fn convert_coordinates(self) -> Self { Vec3::new(-self.x, self.y, -self.z) @@ -48,34 +30,84 @@ impl ConvertCoordinates for [f32; 4] { } } -impl ConvertCoordinates for Quat { - fn convert_coordinates(self) -> Self { - // Solution of q' = r q r* - Quat::from_array([-self.x, self.y, -self.z, self.w]) - } +/// Options for converting scenes and assets from glTF's [standard coordinate system](https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#coordinate-system-and-units) +/// (+Z forward) to Bevy's coordinate system (-Z forward). +/// +/// The exact coordinate system conversion is as follows: +/// - glTF: +/// - forward: +Z +/// - up: +Y +/// - right: -X +/// - Bevy: +/// - forward: -Z +/// - up: +Y +/// - right: +X +/// +/// Note that some glTF files may not follow the glTF standard. +/// +/// If your glTF scene is +Z forward and you want it converted to match Bevy's +/// `Transform::forward`, enable the `scenes` option. If you also want `Mesh` +/// assets to be converted, enable the `meshes` option. +/// +/// Cameras and lights in glTF files are an exception - they already use Bevy's +/// coordinate system. This means cameras and lights will match +/// `Transform::forward` even if conversion is disabled. +#[derive(Copy, Clone, Default, Debug, Serialize, Deserialize)] +pub struct GltfConvertCoordinates { + /// If true, convert scenes via the transform of the scene entity. + /// + /// The glTF loader creates an entity for each glTF scene. Entities are + /// then created for each node within the glTF scene. Nodes without a + /// parent in the glTF scene become children of the scene entity. + /// + /// This option only changes the transform of the scene entity. It does not + /// directly change the transforms of node entities - it only changes them + /// indirectly through transform inheritance. + pub scenes: bool, + + /// If true, convert mesh assets. This includes skinned mesh bind poses. + /// + /// This option only changes mesh assets and the transforms of entities that + /// instance meshes. It does not change the transforms of entities that + /// correspond to glTF nodes. + pub meshes: bool, } -impl ConvertCoordinates for Mat4 { - fn convert_coordinates(self) -> Self { - let m: Mat4 = Mat4::from_scale(Vec3::new(-1.0, 1.0, -1.0)); - // Same as the original matrix - let m_inv = m; - m_inv * self * m +impl GltfConvertCoordinates { + const CONVERSION_TRANSFORM: Transform = + Transform::from_rotation(Quat::from_xyzw(0.0, 1.0, 0.0, 0.0)); + + fn conversion_mat4() -> Mat4 { + Mat4::from_scale(Vec3::new(-1.0, 1.0, -1.0)) } -} -impl ConvertCoordinates for Transform { - fn convert_coordinates(mut self) -> Self { - self.translation = self.translation.convert_coordinates(); - self.rotation = self.rotation.convert_coordinates(); - self + pub(crate) fn scene_conversion_transform(&self) -> Transform { + if self.scenes { + Self::CONVERSION_TRANSFORM + } else { + Transform::IDENTITY + } + } + + pub(crate) fn mesh_conversion_transform(&self) -> Transform { + if self.meshes { + Self::CONVERSION_TRANSFORM + } else { + Transform::IDENTITY + } + } + + pub(crate) fn mesh_conversion_transform_inverse(&self) -> Transform { + // We magically know that the transform is its own inverse. We still + // make a distinction at the interface level in case that changes. + self.mesh_conversion_transform() } -} -impl ConvertCameraCoordinates for Transform { - fn convert_camera_coordinates(mut self) -> Self { - self.translation = self.translation.convert_coordinates(); - self.rotate_y(PI); - self + pub(crate) fn mesh_conversion_mat4(&self) -> Mat4 { + if self.meshes { + Self::conversion_mat4() + } else { + Mat4::IDENTITY + } } } diff --git a/crates/bevy_gltf/src/lib.rs b/crates/bevy_gltf/src/lib.rs index 96b9d7f1d917c..9942c3b1d6f11 100644 --- a/crates/bevy_gltf/src/lib.rs +++ b/crates/bevy_gltf/src/lib.rs @@ -91,7 +91,7 @@ //! You can use [`GltfAssetLabel`] to ensure you are using the correct label. mod assets; -mod convert_coordinates; +pub mod convert_coordinates; mod label; mod loader; mod vertex_attributes; @@ -118,6 +118,8 @@ pub mod prelude { pub use crate::{assets::Gltf, assets::GltfExtras, label::GltfAssetLabel}; } +use crate::convert_coordinates::GltfConvertCoordinates; + pub use {assets::*, label::GltfAssetLabel, loader::*}; // Has to store an Arc> as there is no other way to mutate fields of asset loaders. @@ -159,19 +161,9 @@ pub struct GltfPlugin { /// Can be modified with the [`DefaultGltfImageSampler`] resource. pub default_sampler: ImageSamplerDescriptor, - /// _CAUTION: This is an experimental feature with [known issues](https://github.com/bevyengine/bevy/issues/20621). Behavior may change in future versions._ - /// - /// How to convert glTF coordinates on import. Assuming glTF cameras, glTF lights, and glTF meshes had global identity transforms, - /// their Bevy [`Transform::forward`](bevy_transform::components::Transform::forward) will be pointing in the following global directions: - /// - When set to `false` - /// - glTF cameras and glTF lights: global -Z, - /// - glTF models: global +Z. - /// - When set to `true` - /// - glTF cameras and glTF lights: global +Z, - /// - glTF models: global -Z. - /// - /// The default is `false`. - pub use_model_forward_direction: bool, + /// The default glTF coordinate conversion setting. This can be overridden + /// per-load by [`GltfLoaderSettings::convert_coordinates`]. + pub convert_coordinates: GltfConvertCoordinates, /// Registry for custom vertex attributes. /// @@ -184,7 +176,7 @@ impl Default for GltfPlugin { GltfPlugin { default_sampler: ImageSamplerDescriptor::linear(), custom_vertex_attributes: HashMap::default(), - use_model_forward_direction: false, + convert_coordinates: GltfConvertCoordinates::default(), } } } @@ -234,7 +226,7 @@ impl Plugin for GltfPlugin { supported_compressed_formats, custom_vertex_attributes: self.custom_vertex_attributes.clone(), default_sampler, - default_use_model_forward_direction: self.use_model_forward_direction, + default_convert_coordinates: self.convert_coordinates, }); } } diff --git a/crates/bevy_gltf/src/loader/gltf_ext/scene.rs b/crates/bevy_gltf/src/loader/gltf_ext/scene.rs index d02a131002771..83e6778b99e37 100644 --- a/crates/bevy_gltf/src/loader/gltf_ext/scene.rs +++ b/crates/bevy_gltf/src/loader/gltf_ext/scene.rs @@ -10,10 +10,7 @@ use itertools::Itertools; #[cfg(feature = "bevy_animation")] use bevy_platform::collections::{HashMap, HashSet}; -use crate::{ - convert_coordinates::{ConvertCameraCoordinates as _, ConvertCoordinates as _}, - GltfError, -}; +use crate::GltfError; pub(crate) fn node_name(node: &Node) -> Name { let name = node @@ -29,8 +26,8 @@ pub(crate) fn node_name(node: &Node) -> Name { /// on [`Node::transform()`](gltf::Node::transform) directly because it uses optimized glam types and /// if `libm` feature of `bevy_math` crate is enabled also handles cross /// platform determinism properly. -pub(crate) fn node_transform(node: &Node, convert_coordinates: bool) -> Transform { - let transform = match node.transform() { +pub(crate) fn node_transform(node: &Node) -> Transform { + match node.transform() { gltf::scene::Transform::Matrix { matrix } => { Transform::from_matrix(Mat4::from_cols_array_2d(&matrix)) } @@ -43,15 +40,6 @@ pub(crate) fn node_transform(node: &Node, convert_coordinates: bool) -> Transfor rotation: bevy_math::Quat::from_array(rotation), scale: Vec3::from(scale), }, - }; - if convert_coordinates { - if node.camera().is_some() || node.light().is_some() { - transform.convert_camera_coordinates() - } else { - transform.convert_coordinates() - } - } else { - transform } } diff --git a/crates/bevy_gltf/src/loader/mod.rs b/crates/bevy_gltf/src/loader/mod.rs index 8aa7db8c34e6f..16e491873c628 100644 --- a/crates/bevy_gltf/src/loader/mod.rs +++ b/crates/bevy_gltf/src/loader/mod.rs @@ -61,8 +61,9 @@ use thiserror::Error; use tracing::{error, info_span, warn}; use crate::{ - vertex_attributes::convert_attribute, Gltf, GltfAssetLabel, GltfExtras, GltfMaterialExtras, - GltfMaterialName, GltfMeshExtras, GltfMeshName, GltfNode, GltfSceneExtras, GltfSkin, + convert_coordinates::ConvertCoordinates as _, vertex_attributes::convert_attribute, Gltf, + GltfAssetLabel, GltfExtras, GltfMaterialExtras, GltfMaterialName, GltfMeshExtras, GltfMeshName, + GltfNode, GltfSceneExtras, GltfSkin, }; #[cfg(feature = "bevy_animation")] @@ -80,7 +81,7 @@ use self::{ texture::{texture_handle, texture_sampler, texture_transform_to_affine2}, }, }; -use crate::convert_coordinates::ConvertCoordinates as _; +use crate::convert_coordinates::GltfConvertCoordinates; /// An error that occurs when loading a glTF file. #[derive(Error, Debug)] @@ -147,17 +148,9 @@ pub struct GltfLoader { pub custom_vertex_attributes: HashMap, MeshVertexAttribute>, /// Arc to default [`ImageSamplerDescriptor`]. pub default_sampler: Arc>, - /// How to convert glTF coordinates on import. Assuming glTF cameras, glTF lights, and glTF meshes had global identity transforms, - /// their Bevy [`Transform::forward`](bevy_transform::components::Transform::forward) will be pointing in the following global directions: - /// - When set to `false` - /// - glTF cameras and glTF lights: global -Z, - /// - glTF models: global +Z. - /// - When set to `true` - /// - glTF cameras and glTF lights: global +Z, - /// - glTF models: global -Z. - /// - /// The default is `false`. - pub default_use_model_forward_direction: bool, + /// The default glTF coordinate conversion setting. This can be overridden + /// per-load by [`GltfLoaderSettings::convert_coordinates`]. + pub default_convert_coordinates: GltfConvertCoordinates, } /// Specifies optional settings for processing gltfs at load time. By default, all recognized contents of @@ -199,19 +192,10 @@ pub struct GltfLoaderSettings { pub default_sampler: Option, /// If true, the loader will ignore sampler data from gltf and use the default sampler. pub override_sampler: bool, - /// _CAUTION: This is an experimental feature with [known issues](https://github.com/bevyengine/bevy/issues/20621). Behavior may change in future versions._ - /// - /// How to convert glTF coordinates on import. Assuming glTF cameras, glTF lights, and glTF meshes had global unit transforms, - /// their Bevy [`Transform::forward`](bevy_transform::components::Transform::forward) will be pointing in the following global directions: - /// - When set to `false` - /// - glTF cameras and glTF lights: global -Z, - /// - glTF models: global +Z. - /// - When set to `true` - /// - glTF cameras and glTF lights: global +Z, - /// - glTF models: global -Z. + /// Overrides the default glTF coordinate conversion setting. /// - /// If `None`, uses the global default set by [`GltfPlugin::use_model_forward_direction`](crate::GltfPlugin::use_model_forward_direction). - pub use_model_forward_direction: Option, + /// If `None`, uses the global default set by [`GltfPlugin::convert_coordinates`](crate::GltfPlugin::convert_coordinates). + pub convert_coordinates: Option, } impl Default for GltfLoaderSettings { @@ -224,7 +208,7 @@ impl Default for GltfLoaderSettings { include_source: false, default_sampler: None, override_sampler: false, - use_model_forward_direction: None, + convert_coordinates: None, } } } @@ -264,9 +248,9 @@ impl GltfLoader { paths }; - let convert_coordinates = match settings.use_model_forward_direction { + let convert_coordinates = match settings.convert_coordinates { Some(convert_coordinates) => convert_coordinates, - None => loader.default_use_model_forward_direction, + None => loader.default_convert_coordinates, }; #[cfg(feature = "bevy_animation")] @@ -312,16 +296,7 @@ impl GltfLoader { match outputs { ReadOutputs::Translations(tr) => { let translation_property = animated_field!(Transform::translation); - let translations: Vec = tr - .map(Vec3::from) - .map(|verts| { - if convert_coordinates { - Vec3::convert_coordinates(verts) - } else { - verts - } - }) - .collect(); + let translations: Vec = tr.map(Vec3::from).collect(); if keyframe_timestamps.len() == 1 { Some(VariableCurve::new(AnimatableCurve::new( translation_property, @@ -377,17 +352,8 @@ impl GltfLoader { } ReadOutputs::Rotations(rots) => { let rotation_property = animated_field!(Transform::rotation); - let rotations: Vec = rots - .into_f32() - .map(Quat::from_array) - .map(|quat| { - if convert_coordinates { - Quat::convert_coordinates(quat) - } else { - quat - } - }) - .collect(); + let rotations: Vec = + rots.into_f32().map(Quat::from_array).collect(); if keyframe_timestamps.len() == 1 { Some(VariableCurve::new(AnimatableCurve::new( rotation_property, @@ -688,7 +654,7 @@ impl GltfLoader { accessor, &buffer_data, &loader.custom_vertex_attributes, - convert_coordinates, + convert_coordinates.meshes, ) { Ok((attribute, values)) => mesh.insert_attribute(attribute, values), Err(err) => warn!("{}", err), @@ -715,7 +681,7 @@ impl GltfLoader { }; let morph_target_image = MorphTargetImage::new( morph_target_reader.map(|i| PrimitiveMorphAttributesIter { - convert_coordinates, + convert_coordinates: convert_coordinates.meshes, positions: i.0, normals: i.1, tangents: i.2, @@ -815,15 +781,11 @@ impl GltfLoader { let local_to_bone_bind_matrices: Vec = reader .read_inverse_bind_matrices() .map(|mats| { - mats.map(|mat| Mat4::from_cols_array_2d(&mat)) - .map(|mat| { - if convert_coordinates { - mat.convert_coordinates() - } else { - mat - } - }) - .collect() + mats.map(|mat| { + Mat4::from_cols_array_2d(&mat) + * convert_coordinates.mesh_conversion_mat4() + }) + .collect() }) .unwrap_or_else(|| { core::iter::repeat_n(Mat4::IDENTITY, gltf_skin.joints().len()).collect() @@ -906,7 +868,7 @@ impl GltfLoader { &node, children, mesh, - node_transform(&node, convert_coordinates), + node_transform(&node), skin, node.extras().as_deref().map(GltfExtras::from), ); @@ -939,8 +901,10 @@ impl GltfLoader { let mut entity_to_skin_index_map = EntityHashMap::default(); let mut scene_load_context = load_context.begin_labeled_asset(); + let world_root_transform = convert_coordinates.scene_conversion_transform(); + let world_root_id = world - .spawn((Transform::default(), Visibility::default())) + .spawn((world_root_transform, Visibility::default())) .with_children(|parent| { for node in scene.nodes() { let result = load_node( @@ -958,7 +922,7 @@ impl GltfLoader { #[cfg(feature = "bevy_animation")] None, &gltf.document, - convert_coordinates, + &convert_coordinates, ); if result.is_err() { err = Some(result); @@ -1400,10 +1364,10 @@ fn load_node( #[cfg(feature = "bevy_animation")] animation_roots: &HashSet, #[cfg(feature = "bevy_animation")] mut animation_context: Option, document: &Document, - convert_coordinates: bool, + convert_coordinates: &GltfConvertCoordinates, ) -> Result<(), GltfError> { let mut gltf_error = None; - let transform = node_transform(gltf_node, convert_coordinates); + let transform = node_transform(gltf_node); let world_transform = *parent_transform * transform; // according to https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#instantiation, // if the determinant of the transform is negative we must invert the winding order of @@ -1519,12 +1483,18 @@ fn load_node( }; let bounds = primitive.bounding_box(); + // Apply the inverse of the conversion transform that's been + // applied to the mesh asset. This preserves the mesh's relation + // to the node transform. + let mesh_entity_transform = convert_coordinates.mesh_conversion_transform_inverse(); + let mut mesh_entity = parent.spawn(( // TODO: handle missing label handle errors here? Mesh3d(load_context.get_label_handle(primitive_label.to_string())), MeshMaterial3d::( load_context.get_label_handle(&material_label), ), + mesh_entity_transform, )); let target_count = primitive.morph_targets().len(); @@ -1550,7 +1520,7 @@ fn load_node( let mut bounds_min = Vec3::from_slice(&bounds.min); let mut bounds_max = Vec3::from_slice(&bounds.max); - if convert_coordinates { + if convert_coordinates.meshes { let converted_min = bounds_min.convert_coordinates(); let converted_max = bounds_max.convert_coordinates(); diff --git a/examples/testbed/3d.rs b/examples/testbed/3d.rs index bfa2fbf648591..bac79a6a27c77 100644 --- a/examples/testbed/3d.rs +++ b/examples/testbed/3d.rs @@ -354,7 +354,10 @@ mod gizmos { mod gltf_coordinate_conversion { use bevy::{ - color::palettes::basic::*, gltf::GltfLoaderSettings, prelude::*, scene::SceneInstanceReady, + color::palettes::basic::*, + gltf::{convert_coordinates::GltfConvertCoordinates, GltfLoaderSettings}, + prelude::*, + scene::SceneInstanceReady, }; const CURRENT_SCENE: super::Scene = super::Scene::GltfCoordinateConversion; @@ -398,7 +401,10 @@ mod gltf_coordinate_conversion { SceneRoot(asset_server.load_with_settings( GltfAssetLabel::Scene(0).from_asset("models/Faces/faces.glb"), |s: &mut GltfLoaderSettings| { - s.use_model_forward_direction = Some(true); + s.convert_coordinates = Some(GltfConvertCoordinates { + scenes: true, + meshes: true, + }); }, )), DespawnOnExitState(CURRENT_SCENE), diff --git a/examples/tools/scene_viewer/main.rs b/examples/tools/scene_viewer/main.rs index 9d7eea45de120..aea41f01dfc0c 100644 --- a/examples/tools/scene_viewer/main.rs +++ b/examples/tools/scene_viewer/main.rs @@ -13,7 +13,7 @@ use bevy::{ asset::UnapprovedPathMode, camera::primitives::{Aabb, Sphere}, core_pipeline::prepass::{DeferredPrepass, DepthPrepass}, - gltf::GltfPlugin, + gltf::{convert_coordinates::GltfConvertCoordinates, GltfPlugin}, pbr::DefaultOpaqueRendererMethod, prelude::*, render::experimental::occlusion_culling::OcclusionCulling, @@ -52,17 +52,20 @@ struct Args { /// spawn a light even if the scene already has one #[argh(switch)] add_light: Option, - /// enable `GltfPlugin::use_model_forward_direction` + /// enable `GltfPlugin::convert_coordinates::scenes` #[argh(switch)] - use_model_forward_direction: Option, + convert_scene_coordinates: Option, + /// enable `GltfPlugin::convert_coordinates::meshes` + #[argh(switch)] + convert_mesh_coordinates: Option, } impl Args { fn rotation(&self) -> Quat { - if self.use_model_forward_direction == Some(true) { + if self.convert_scene_coordinates == Some(true) { // If the scene is converted then rotate everything else to match. This // makes comparisons easier - the scene will always face the same way - // relative to the camera. + // relative to the cameras and lights. Quat::from_xyzw(0.0, 1.0, 0.0, 0.0) } else { Quat::IDENTITY @@ -95,7 +98,10 @@ fn main() { ..default() }) .set(GltfPlugin { - use_model_forward_direction: args.use_model_forward_direction.unwrap_or(false), + convert_coordinates: GltfConvertCoordinates { + scenes: args.convert_scene_coordinates == Some(true), + meshes: args.convert_mesh_coordinates == Some(true), + }, ..default() }), CameraControllerPlugin, diff --git a/release-content/migration-guides/gltf-coordinate-conversion.md b/release-content/migration-guides/gltf-coordinate-conversion.md new file mode 100644 index 0000000000000..23797ea6c7c1c --- /dev/null +++ b/release-content/migration-guides/gltf-coordinate-conversion.md @@ -0,0 +1,82 @@ +--- +title: Changes to glTF coordinate conversion +pull_requests: [20394] +--- + +Bevy 0.17 added an option for coordinate conversion of glTF files - +`GltfPlugin::convert_coordinates` and `GltfLoaderSettings::convert_coordinates`. +The goal was to ensure that objects facing forward in the glTF matched the +direction of Bevy's `Transform::forward`. + +The conversion was disabled by default, so if you didn't enable the option then +you don't have to make any changes - your glTFs will work the same as before. + +Conversion is useful because glTF's standard forward is +Z, while Bevy's is -Z +(although not all glTF files follow the standard). Cameras and lights are an +exception - they have the correct `Transform::forward` with or without +conversion. This is because both glTF and Bevy use -Z forward for cameras and +lights. + +The Bevy 0.17 conversion was applied to scenes, nodes and meshes within the +glTF. This worked well for some users, but had +[bugs](https://github.com/bevyengine/bevy/issues/20621) and didn't work well for +other users. + +In Bevy 0.18, parts of the conversion have been removed or rearranged to avoid +bugs. The `convert_coordinates` boolean has been changed to a struct with +separate options to convert scenes and/or meshes. + +```diff + struct GltfPlugin { + ... +- bool convert_coordinates, ++ GltfConvertCoordinates convert_coordinates, + } +``` + +```rust +struct GltfConvertCoordinates { + scenes: bool, + meshes: bool, +} +``` + +Whether the changes affect you will depend on how you're using glTFs. + +If you simply spawn your glTF as a scene and want it to visually match the +`Transform::forward` of the entity it's spawned on, then you're supported by the +`GltfConvertCoordinates::scenes` option. + +If you want the `Mesh` assets within the glTF to be converted, then you're +supported by the `GltfConvertCoordinates::meshes` option. This can be combined +with the `scenes` option if you want both. + +There is no longer a way to enable conversion of nodes within the glTF scene. +This change was made to avoid bugs and give other users more options. If you +only needed scene and/or mesh conversion then you're not affected by this +change. + +If you want to start using conversion, the easiest way is to enable it for your +entire app through the `GltfPlugin` settings. This example enables scene +conversion: + +```rust +App::new() + .add_plugins(DefaultPlugins.set(GltfPlugin { + convert_coordinates: GltfConvertCoordinates { scenes: true, ..default() }, + ..default() + })) + .run(); +``` + +You can also choose the option per-asset through `GltfLoaderSettings`. This will +override the settings in `GltfPlugin`. + +```rust +let handle = asset_server.load_with_settings( + "fox.gltf#Scene0", + |settings: &mut GltfLoaderSettings| { + settings.convert_coordinates = Some(GltfConvertCoordinates { scenes: true, ..default() }); + }, +); +```