Skip to content

WIP: Fix glTF model forward direction #20135

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP: Fix glTF model forward direction #20135

wants to merge 1 commit into from
+266 −11

Conversation

superdump
Copy link
Contributor

This is to illustrate what I think is needed for the glTF model forward direction being +z.

@superdump superdump mentioned this pull request Jul 14, 2025
Open
@github-actions GitHub Actions
Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

janhohenheim
janhohenheim reviewed Jul 14, 2025
View reviewed changes
examples/transforms/model_forward.rs
// With higher speed the curvature of the orbit would be smaller.
let incremental_turn_weight = cube.turn_speed * timer.delta_secs();
let old_rotation = transform.rotation;
transform.rotation = old_rotation.lerp(look_at_sphere.rotation, incremental_turn_weight);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be smooth_nudge nowadays

crates/bevy_transform/src/components/transform.rs
@@ -317,16 +320,44 @@ impl Transform {

/// Equivalent to [`-local_z()`][Transform::local_z]
#[inline]
pub fn forward(&self) -> Dir3 {
Copy link
Member

Choose a reason for hiding this comment

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

Should probably deprecate these methods instead of removing them

crates/bevy_transform/src/components/transform.rs
/// Equivalent to [`-local_z()`][Transform::local_z] if `flip_model_forward` is false,
/// else [`local_z()`][Transform::local_z]
///
/// glTF has opposing forward directions for cameras and lights, and for models. Model
Copy link
Member

Choose a reason for hiding this comment

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

To be asset agnostic, this should not even mention glTF imo

Copy link
Member

Choose a reason for hiding this comment

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

If we decide to do this, we should only mention glTF in an explanatory note.

@superdump superdump force-pushed the fix-gltf-model-forward branch from fb2bcc4 to aaa8f70 Compare July 14, 2025 16:16
janhohenheim
janhohenheim reviewed Jul 14, 2025
View reviewed changes
crates/bevy_gltf/src/loader/gltf_ext/scene.rs
@@ -42,6 +42,7 @@ pub(crate) fn node_transform(node: &Node, convert_coordinates: bool) -> Transfor
translation: Vec3::from(translation),
rotation: bevy_math::Quat::from_array(rotation),
scale: Vec3::from(scale),
flip_model_forward: true,
Copy link
Member

Choose a reason for hiding this comment

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

Per @atlv24: let's make this a u8 enum :)

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon X-Contentious There are nontrivial implications that should be thought through A-Transform Translations, rotations and scales A-glTF Related to the glTF 3D scene/model format S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Uncontroversial This work is generally agreed upon labels Jul 14, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 14, 2025
alice-i-cecile
alice-i-cecile reviewed Jul 14, 2025
View reviewed changes
crates/bevy_transform/src/components/transform.rs
@@ -317,16 +324,44 @@ impl Transform {

/// Equivalent to [`-local_z()`][Transform::local_z]
#[inline]
pub fn forward(&self) -> Dir3 {
pub fn camera_forward(&self) -> Dir3 {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to cover lights too. Maybe just duplicate methods?

@janhohenheim janhohenheim added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 14, 2025
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@janhohenheim janhohenheim janhohenheim left review comments

Assignees
No one assigned
Labels
A-glTF Related to the glTF 3D scene/model format A-Transform Translations, rotations and scales M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.
3 participants
@superdump @alice-i-cecile @janhohenheim