From 7acf69747921553ba968b0a602f25c44031fc8b4 Mon Sep 17 00:00:00 2001 From: reduz Date: Sat, 25 Jun 2022 11:23:24 +0200 Subject: [PATCH] Fix VECTOR/LOCAL transitions in Node3D Fixes #62225, supersedes #62227 --- core/math/basis.cpp | 12 ++-- core/math/basis.h | 30 ++++----- scene/3d/node_3d.cpp | 148 ++++++++++++++++++++++++++++--------------- scene/3d/node_3d.h | 30 +++++++-- 4 files changed, 143 insertions(+), 77 deletions(-) diff --git a/core/math/basis.cpp b/core/math/basis.cpp index 65353d81183..ce5e9aa9b3e 100644 --- a/core/math/basis.cpp +++ b/core/math/basis.cpp @@ -365,12 +365,12 @@ Basis Basis::rotated_local(const Vector3 &p_axis, real_t p_angle) const { return (*this) * Basis(p_axis, p_angle); } -Basis Basis::rotated(const Vector3 &p_euler) const { - return Basis(p_euler) * (*this); +Basis Basis::rotated(const Vector3 &p_euler, EulerOrder p_order) const { + return Basis::from_euler(p_euler, p_order) * (*this); } -void Basis::rotate(const Vector3 &p_euler) { - *this = rotated(p_euler); +void Basis::rotate(const Vector3 &p_euler, EulerOrder p_order) { + *this = rotated(p_euler, p_order); } Basis Basis::rotated(const Quaternion &p_quaternion) const { @@ -935,9 +935,9 @@ void Basis::set_axis_angle_scale(const Vector3 &p_axis, real_t p_angle, const Ve rotate(p_axis, p_angle); } -void Basis::set_euler_scale(const Vector3 &p_euler, const Vector3 &p_scale) { +void Basis::set_euler_scale(const Vector3 &p_euler, const Vector3 &p_scale, EulerOrder p_order) { _set_diagonal(p_scale); - rotate(p_euler); + rotate(p_euler, p_order); } void Basis::set_quaternion_scale(const Quaternion &p_quaternion, const Vector3 &p_scale) { diff --git a/core/math/basis.h b/core/math/basis.h index 9cce22510bf..4be325cdd2e 100644 --- a/core/math/basis.h +++ b/core/math/basis.h @@ -56,20 +56,6 @@ struct _NO_DISCARD_ Basis { _FORCE_INLINE_ real_t determinant() const; - void from_z(const Vector3 &p_z); - - void rotate(const Vector3 &p_axis, real_t p_angle); - Basis rotated(const Vector3 &p_axis, real_t p_angle) const; - - void rotate_local(const Vector3 &p_axis, real_t p_angle); - Basis rotated_local(const Vector3 &p_axis, real_t p_angle) const; - - void rotate(const Vector3 &p_euler); - Basis rotated(const Vector3 &p_euler) const; - - void rotate(const Quaternion &p_quaternion); - Basis rotated(const Quaternion &p_quaternion) const; - enum EulerOrder { EULER_ORDER_XYZ, EULER_ORDER_XZY, @@ -79,6 +65,20 @@ struct _NO_DISCARD_ Basis { EULER_ORDER_ZYX }; + void from_z(const Vector3 &p_z); + + void rotate(const Vector3 &p_axis, real_t p_angle); + Basis rotated(const Vector3 &p_axis, real_t p_angle) const; + + void rotate_local(const Vector3 &p_axis, real_t p_angle); + Basis rotated_local(const Vector3 &p_axis, real_t p_angle) const; + + void rotate(const Vector3 &p_euler, EulerOrder p_order = EULER_ORDER_YXZ); + Basis rotated(const Vector3 &p_euler, EulerOrder p_order = EULER_ORDER_YXZ) const; + + void rotate(const Quaternion &p_quaternion); + Basis rotated(const Quaternion &p_quaternion) const; + Vector3 get_euler_normalized(EulerOrder p_order = EULER_ORDER_YXZ) const; void get_rotation_axis_angle(Vector3 &p_axis, real_t &p_angle) const; void get_rotation_axis_angle_local(Vector3 &p_axis, real_t &p_angle) const; @@ -119,7 +119,7 @@ struct _NO_DISCARD_ Basis { Vector3 get_scale_local() const; void set_axis_angle_scale(const Vector3 &p_axis, real_t p_angle, const Vector3 &p_scale); - void set_euler_scale(const Vector3 &p_euler, const Vector3 &p_scale); + void set_euler_scale(const Vector3 &p_euler, const Vector3 &p_scale, EulerOrder p_order = EULER_ORDER_YXZ); void set_quaternion_scale(const Quaternion &p_quaternion, const Vector3 &p_scale); // transposed dot products diff --git a/scene/3d/node_3d.cpp b/scene/3d/node_3d.cpp index 6e36815f2b3..ea5206b3c56 100644 --- a/scene/3d/node_3d.cpp +++ b/scene/3d/node_3d.cpp @@ -85,12 +85,20 @@ void Node3D::_notify_dirty() { } void Node3D::_update_local_transform() const { - if (this->get_rotation_edit_mode() != ROTATION_EDIT_MODE_BASIS) { - data.local_transform = data.local_transform.orthogonalized(); - } - data.local_transform.basis.set_euler_scale(data.rotation, data.scale); + // This function is called when the local transform (data.local_transform) is dirty and the right value is contained in the Euler rotation and scale. - data.dirty &= ~DIRTY_LOCAL; + data.local_transform.basis.set_euler_scale(data.euler_rotation, data.scale, data.euler_rotation_order); + + data.dirty &= ~DIRTY_LOCAL_TRANSFORM; +} + +void Node3D::_update_rotation_and_scale() const { + // This function is called when the Euler rotation (data.euler_rotation) is dirty and the right value is contained in the local transform + + data.scale = data.local_transform.basis.get_scale(); + data.euler_rotation = data.local_transform.basis.get_euler_normalized(data.euler_rotation_order); + + data.dirty &= ~DIRTY_EULER_ROTATION_AND_SCALE; } void Node3D::_propagate_transform_changed(Node3D *p_origin) { @@ -113,7 +121,7 @@ void Node3D::_propagate_transform_changed(Node3D *p_origin) { #endif get_tree()->xform_change_list.add(&xform_change); } - data.dirty |= DIRTY_GLOBAL; + data.dirty |= DIRTY_GLOBAL_TRANSFORM; data.children_lock--; } @@ -137,12 +145,12 @@ void Node3D::_notification(int p_what) { if (data.top_level && !Engine::get_singleton()->is_editor_hint()) { if (data.parent) { data.local_transform = data.parent->get_global_transform() * get_transform(); - data.dirty = DIRTY_VECTORS; //global is always dirty upon entering a scene + data.dirty = DIRTY_EULER_ROTATION_AND_SCALE; // As local transform was updated, rot/scale should be dirty. } data.top_level_active = true; } - data.dirty |= DIRTY_GLOBAL; //global is always dirty upon entering a scene + data.dirty |= DIRTY_GLOBAL_TRANSFORM; // Global is always dirty upon entering a scene. _notify_dirty(); notification(NOTIFICATION_ENTER_WORLD); @@ -212,12 +220,27 @@ void Node3D::set_basis(const Basis &p_basis) { set_transform(Transform3D(p_basis, data.local_transform.origin)); } void Node3D::set_quaternion(const Quaternion &p_quaternion) { - set_transform(Transform3D(Basis(p_quaternion), data.local_transform.origin)); + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + // We need the scale part, so if these are dirty, update it + data.scale = data.local_transform.basis.get_scale(); + data.dirty &= ~DIRTY_EULER_ROTATION_AND_SCALE; + } + data.local_transform.basis = Basis(p_quaternion, data.scale); + // Rotscale should not be marked dirty because that would cause precision loss issues with the scale. Instead reconstruct rotation now. + data.euler_rotation = data.local_transform.basis.get_euler_normalized(data.euler_rotation_order); + + data.dirty = DIRTY_NONE; + + _propagate_transform_changed(this); + if (data.notify_local_transform) { + notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); + } } void Node3D::set_transform(const Transform3D &p_transform) { data.local_transform = p_transform; - data.dirty |= DIRTY_VECTORS; + data.dirty = DIRTY_EULER_ROTATION_AND_SCALE; // Make rot/scale dirty. + _propagate_transform_changed(this); if (data.notify_local_transform) { notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); @@ -227,8 +250,13 @@ void Node3D::set_transform(const Transform3D &p_transform) { Basis Node3D::get_basis() const { return get_transform().basis; } + Quaternion Node3D::get_quaternion() const { - return Quaternion(get_transform().basis); + if (data.dirty & DIRTY_LOCAL_TRANSFORM) { + _update_local_transform(); + } + + return data.local_transform.basis.get_rotation_quaternion(); } void Node3D::set_global_transform(const Transform3D &p_transform) { @@ -240,7 +268,7 @@ void Node3D::set_global_transform(const Transform3D &p_transform) { } Transform3D Node3D::get_transform() const { - if (data.dirty & DIRTY_LOCAL) { + if (data.dirty & DIRTY_LOCAL_TRANSFORM) { _update_local_transform(); } @@ -249,8 +277,8 @@ Transform3D Node3D::get_transform() const { Transform3D Node3D::get_global_transform() const { ERR_FAIL_COND_V(!is_inside_tree(), Transform3D()); - if (data.dirty & DIRTY_GLOBAL) { - if (data.dirty & DIRTY_LOCAL) { + if (data.dirty & DIRTY_GLOBAL_TRANSFORM) { + if (data.dirty & DIRTY_LOCAL_TRANSFORM) { _update_local_transform(); } @@ -264,7 +292,7 @@ Transform3D Node3D::get_global_transform() const { data.global_transform.basis.orthonormalize(); } - data.dirty &= ~DIRTY_GLOBAL; + data.dirty &= ~DIRTY_GLOBAL_TRANSFORM; } return data.global_transform; @@ -314,13 +342,27 @@ void Node3D::set_rotation_edit_mode(RotationEditMode p_mode) { if (data.rotation_edit_mode == p_mode) { return; } + + bool transform_changed = false; + if (data.rotation_edit_mode == ROTATION_EDIT_MODE_BASIS && !(data.dirty & DIRTY_LOCAL_TRANSFORM)) { + data.local_transform.orthogonalize(); + transform_changed = true; + } + data.rotation_edit_mode = p_mode; - // Shearing is not allowed except in ROTATION_EDIT_MODE_BASIS. - data.dirty |= DIRTY_LOCAL; - _propagate_transform_changed(this); - if (data.notify_local_transform) { - notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); + if (p_mode == ROTATION_EDIT_MODE_EULER && (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE)) { + // If going to Euler mode, ensure that vectors are _not_ dirty, else the retrieved value may be wrong. + // Otherwise keep what is there, so switching back and forth between modes does not break the vectors. + + _update_rotation_and_scale(); + } + + if (transform_changed) { + _propagate_transform_changed(this); + if (data.notify_local_transform) { + notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); + } } notify_property_list_changed(); @@ -333,38 +375,47 @@ Node3D::RotationEditMode Node3D::get_rotation_edit_mode() const { void Node3D::set_rotation_order(RotationOrder p_order) { Basis::EulerOrder order = Basis::EulerOrder(p_order); - if (data.rotation_order == order) { + if (data.euler_rotation_order == order) { return; } ERR_FAIL_INDEX(int32_t(order), 6); + bool transform_changed = false; - if (data.dirty & DIRTY_VECTORS) { - data.rotation = data.local_transform.basis.get_euler_normalized(order); - data.scale = data.local_transform.basis.get_scale(); - data.dirty &= ~DIRTY_VECTORS; + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + _update_rotation_and_scale(); + } else if (data.dirty & DIRTY_LOCAL_TRANSFORM) { + data.euler_rotation = Basis::from_euler(data.euler_rotation, data.euler_rotation_order).get_euler_normalized(order); + transform_changed = true; } else { - data.rotation = Basis::from_euler(data.rotation, data.rotation_order).get_euler_normalized(order); + data.dirty |= DIRTY_LOCAL_TRANSFORM; + transform_changed = true; } - data.rotation_order = order; - //changing rotation order should not affect transform + data.euler_rotation_order = order; - notify_property_list_changed(); //will change rotation + if (transform_changed) { + _propagate_transform_changed(this); + if (data.notify_local_transform) { + notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); + } + } + notify_property_list_changed(); // Will change the rotation property. } Node3D::RotationOrder Node3D::get_rotation_order() const { - return RotationOrder(data.rotation_order); + return RotationOrder(data.euler_rotation_order); } void Node3D::set_rotation(const Vector3 &p_euler_rad) { - if (data.dirty & DIRTY_VECTORS) { + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + // Update scale only if rotation and scale are dirty, as rotation will be overridden. data.scale = data.local_transform.basis.get_scale(); - data.dirty &= ~DIRTY_VECTORS; + data.dirty &= ~DIRTY_EULER_ROTATION_AND_SCALE; } - data.rotation = p_euler_rad; - data.dirty |= DIRTY_LOCAL; + data.euler_rotation = p_euler_rad; + data.dirty = DIRTY_LOCAL_TRANSFORM; _propagate_transform_changed(this); if (data.notify_local_transform) { notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); @@ -372,13 +423,14 @@ void Node3D::set_rotation(const Vector3 &p_euler_rad) { } void Node3D::set_scale(const Vector3 &p_scale) { - if (data.dirty & DIRTY_VECTORS) { - data.rotation = data.local_transform.basis.get_euler_normalized(data.rotation_order); - data.dirty &= ~DIRTY_VECTORS; + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + // Update rotation only if rotation and scale are dirty, as scale will be overridden. + data.euler_rotation = data.local_transform.basis.get_euler_normalized(data.euler_rotation_order); + data.dirty &= ~DIRTY_EULER_ROTATION_AND_SCALE; } data.scale = p_scale; - data.dirty |= DIRTY_LOCAL; + data.dirty = DIRTY_LOCAL_TRANSFORM; _propagate_transform_changed(this); if (data.notify_local_transform) { notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED); @@ -390,22 +442,16 @@ Vector3 Node3D::get_position() const { } Vector3 Node3D::get_rotation() const { - if (data.dirty & DIRTY_VECTORS) { - data.scale = data.local_transform.basis.get_scale(); - data.rotation = data.local_transform.basis.get_euler_normalized(data.rotation_order); - - data.dirty &= ~DIRTY_VECTORS; + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + _update_rotation_and_scale(); } - return data.rotation; + return data.euler_rotation; } Vector3 Node3D::get_scale() const { - if (data.dirty & DIRTY_VECTORS) { - data.scale = data.local_transform.basis.get_scale(); - data.rotation = data.local_transform.basis.get_euler_normalized(data.rotation_order); - - data.dirty &= ~DIRTY_VECTORS; + if (data.dirty & DIRTY_EULER_ROTATION_AND_SCALE) { + _update_rotation_and_scale(); } return data.scale; @@ -865,14 +911,14 @@ Variant Node3D::property_get_revert(const String &p_name) { } else if (p_name == "quaternion") { Variant variant = PropertyUtils::get_property_default_value(this, "transform", &valid); if (valid && variant.get_type() == Variant::Type::TRANSFORM3D) { - r_ret = Quaternion(Transform3D(variant).get_basis()); + r_ret = Quaternion(Transform3D(variant).get_basis().get_rotation_quaternion()); } else { return Quaternion(); } } else if (p_name == "rotation") { Variant variant = PropertyUtils::get_property_default_value(this, "transform", &valid); if (valid && variant.get_type() == Variant::Type::TRANSFORM3D) { - r_ret = Transform3D(variant).get_basis().get_euler_normalized(data.rotation_order); + r_ret = Transform3D(variant).get_basis().get_euler_normalized(data.euler_rotation_order); } else { return Vector3(); } diff --git a/scene/3d/node_3d.h b/scene/3d/node_3d.h index 6d857a83eaf..cfd88585e4b 100644 --- a/scene/3d/node_3d.h +++ b/scene/3d/node_3d.h @@ -52,6 +52,9 @@ class Node3D : public Node { GDCLASS(Node3D, Node); public: + // Edit mode for the rotation. + // THIS MODE ONLY AFFECTS HOW DATA IS EDITED AND SAVED + // IT DOES _NOT_ AFFECT THE TRANSFORM LOGIC (see comment in TransformDirty). enum RotationEditMode { ROTATION_EDIT_MODE_EULER, ROTATION_EDIT_MODE_QUATERNION, @@ -68,11 +71,27 @@ public: }; private: + // For the sake of ease of use, Node3D can operate with Transforms (Basis+Origin), Quaterinon/Scale and Euler Rotation/Scale. + // Transform and Quaterinon are stored in data.local_transform Basis (so quaternion is not really stored, but converted back/forth from 3x3 matrix on demand). + // Euler needs to be kept separate because converting to Basis and back may result in a different vector (which is troublesome for users + // editing in the inspector, not only because of the numerical precision loss but because they expect these rotations to be consistent, or support + // "redundant" rotations for animation interpolation, like going from 0 to 720 degrees). + // + // As such, the system works in a way where if the local transform is set (via transform/basis/quaternion), the EULER rotation and scale becomes dirty. + // It will remain dirty until reading back is attempted (for performance reasons). Likewise, if the Euler rotation scale are set, the local transform + // will become dirty (and again, will not become valid again until read). + // + // All this is transparent from outside the Node3D API, which allows everything to works by calling these functions in exchange. + // + // Additionally, setting either transform, quaternion, Euler rotation or scale makes the global transform dirty, which will be updated when read again. + // + // NOTE: Again, RotationEditMode is _independent_ of this mechanism, it is only meant to expose the right set of properties for editing (editor) and saving + // (to scene, in order to keep the same values and avoid data loss on conversions). It has zero influence in the logic described above. enum TransformDirty { DIRTY_NONE = 0, - DIRTY_VECTORS = 1, - DIRTY_LOCAL = 2, - DIRTY_GLOBAL = 4 + DIRTY_EULER_ROTATION_AND_SCALE = 1, + DIRTY_LOCAL_TRANSFORM = 2, + DIRTY_GLOBAL_TRANSFORM = 4 }; mutable SelfList xform_change; @@ -80,8 +99,8 @@ private: struct Data { mutable Transform3D global_transform; mutable Transform3D local_transform; - mutable Basis::EulerOrder rotation_order = Basis::EULER_ORDER_YXZ; - mutable Vector3 rotation; + mutable Basis::EulerOrder euler_rotation_order = Basis::EULER_ORDER_YXZ; + mutable Vector3 euler_rotation; mutable Vector3 scale = Vector3(1, 1, 1); mutable RotationEditMode rotation_edit_mode = ROTATION_EDIT_MODE_EULER; @@ -131,6 +150,7 @@ protected: _FORCE_INLINE_ void set_ignore_transform_notification(bool p_ignore) { data.ignore_notification = p_ignore; } _FORCE_INLINE_ void _update_local_transform() const; + _FORCE_INLINE_ void _update_rotation_and_scale() const; void _notification(int p_what); static void _bind_methods();