Skip to content
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.

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

Fix looking at with 180 degree arc #100991

Merged
merged 1 commit into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions core/math/basis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1049,9 +1049,10 @@ Basis Basis::looking_at(const Vector3 &p_target, const Vector3 &p_up, bool p_use
v_z = -v_z;
}
Vector3 v_x = p_up.cross(v_z);
#ifdef MATH_CHECKS
ERR_FAIL_COND_V_MSG(v_x.is_zero_approx(), Basis(), "The target vector and up vector can't be parallel to each other.");
#endif
if (v_x.is_zero_approx()) {
WARN_PRINT("Target and up vectors are colinear. This is not advised as it may cause unwanted rotation around local Z axis.");
Copy link
Member

Choose a reason for hiding this comment

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

This might spam a lot, shouldn't it be WARN_PRINT_ONCE?

In general this kind of warnings aren't super helpful to end users, as a basis may end up in this state from internal engine code or complex calculations, and the warning doesn't point at which basis / object transform is referred to. So I'm not convinced this will make end users happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

If look_at is called continuously, it may be spammed, but in that case it should not be called continuously in the first place, so spamming it is not considered excessive.

As mentioned in #100991 (comment), if the orientation is nearly the same, the user should detect it and do nothing. Well, this could be add documentation or changing a warning text.

v_x = p_up.get_any_perpendicular(); // Vectors are almost parallel.
}
v_x.normalize();
Vector3 v_y = v_z.cross(v_x);

Expand Down
11 changes: 6 additions & 5 deletions core/math/quaternion.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,15 @@ struct [[nodiscard]] Quaternion {

Quaternion(const Vector3 &p_v0, const Vector3 &p_v1) { // Shortest arc.
Vector3 c = p_v0.cross(p_v1);
real_t d = p_v0.dot(p_v1);

if (d < -1.0f + (real_t)CMP_EPSILON) {
x = 0;
y = 1;
z = 0;
if (c.is_zero_approx()) {
Vector3 axis = p_v0.get_any_perpendicular();
x = axis.x;
y = axis.y;
z = axis.z;
w = 0;
} else {
real_t d = p_v0.dot(p_v1);
real_t s = Math::sqrt((1.0f + d) * 2.0f);
real_t rs = 1.0f / s;

Expand Down
11 changes: 11 additions & 0 deletions core/math/vector3.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ struct [[nodiscard]] Vector3 {
_FORCE_INLINE_ Vector3 cross(const Vector3 &p_with) const;
_FORCE_INLINE_ real_t dot(const Vector3 &p_with) const;
Basis outer(const Vector3 &p_with) const;
_FORCE_INLINE_ Vector3 get_any_perpendicular() const;

_FORCE_INLINE_ Vector3 abs() const;
_FORCE_INLINE_ Vector3 floor() const;
Expand Down Expand Up @@ -326,6 +327,16 @@ Vector3 Vector3::direction_to(const Vector3 &p_to) const {
return ret;
}

Vector3 Vector3::get_any_perpendicular() const {
// Return the any perpendicular vector by cross product with the Vector3.RIGHT or Vector3.UP,
// whichever has the greater angle to the current vector with the sign of each element positive.
// The only essence is "to avoid being parallel to the current vector", and there is no mathematical basis for using Vector3.RIGHT and Vector3.UP,
// since it could be a different vector depending on the prior branching code Math::abs(x) <= Math::abs(y) && Math::abs(x) <= Math::abs(z).
// However, it would be reasonable to use any of the axes of the basis, as it is simpler to calculate.
ERR_FAIL_COND_V_MSG(is_zero_approx(), Vector3(0, 0, 0), "The Vector3 must not be zero.");
return cross((Math::abs(x) <= Math::abs(y) && Math::abs(x) <= Math::abs(z)) ? Vector3(1, 0, 0) : Vector3(0, 1, 0)).normalized();
}

/* Operators */

Vector3 &Vector3::operator+=(const Vector3 &p_v) {
Expand Down
3 changes: 2 additions & 1 deletion doc/classes/Basis.xml
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@
<description>
Creates a new [Basis] with a rotation such that the forward axis (-Z) points towards the [param target] position.
By default, the -Z axis (camera forward) is treated as forward (implies +X is right). If [param use_model_front] is [code]true[/code], the +Z axis (asset front) is treated as forward (implies +X is left) and points toward the [param target] position.
The up axis (+Y) points as close to the [param up] vector as possible while staying perpendicular to the forward axis. The returned basis is orthonormalized (see [method orthonormalized]). The [param target] and [param up] vectors cannot be [constant Vector3.ZERO], and cannot be parallel to each other.
The up axis (+Y) points as close to the [param up] vector as possible while staying perpendicular to the forward axis. The returned basis is orthonormalized (see [method orthonormalized]).
The [param target] and the [param up] cannot be [constant Vector3.ZERO], and shouldn't be colinear to avoid unintended rotation around local Z axis.
</description>
</method>
<method name="orthonormalized" qualifiers="const">
Expand Down
3 changes: 2 additions & 1 deletion doc/classes/Node3D.xml
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@
<description>
Rotates the node so that the local forward axis (-Z, [constant Vector3.FORWARD]) points toward the [param target] position.
The local up axis (+Y) points as close to the [param up] vector as possible while staying perpendicular to the local forward axis. The resulting transform is orthogonal, and the scale is preserved. Non-uniform scaling may not work correctly.
The [param target] position cannot be the same as the node's position, the [param up] vector cannot be zero, and the direction from the node's position to the [param target] vector cannot be parallel to the [param up] vector.
The [param target] position cannot be the same as the node's position, the [param up] vector cannot be zero.
The [param target] and the [param up] cannot be [constant Vector3.ZERO], and shouldn't be colinear to avoid unintended rotation around local Z axis.
Operations take place in global space, which means that the node must be in the scene tree.
If [param use_model_front] is [code]true[/code], the +Z axis (asset front) is treated as forward (implies +X is left) and points toward the [param target] position. By default, the -Z axis (camera forward) is treated as forward (implies +X is right).
</description>
Expand Down
1 change: 0 additions & 1 deletion scene/3d/node_3d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1066,7 +1066,6 @@ void Node3D::look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target
ERR_THREAD_GUARD;
ERR_FAIL_COND_MSG(p_pos.is_equal_approx(p_target), "Node origin and target are in the same position, look_at() failed.");
ERR_FAIL_COND_MSG(p_up.is_zero_approx(), "The up vector can't be zero, look_at() failed.");
ERR_FAIL_COND_MSG(p_up.cross(p_target - p_pos).is_zero_approx(), "Up vector and direction between node origin and target are aligned, look_at() failed.");

Vector3 forward = p_target - p_pos;
Basis lookat_basis = Basis::looking_at(forward, p_up, p_use_model_front);
Expand Down
30 changes: 30 additions & 0 deletions tests/core/math/test_quaternion.h
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,36 @@ TEST_CASE("[Quaternion] Construct Basis Axes") {
CHECK(q[3] == doctest::Approx(0.8582598));
}

TEST_CASE("[Quaternion] Construct Shortest Arc For 180 Degree Arc") {
Vector3 up(0, 1, 0);
Vector3 down(0, -1, 0);
Vector3 left(-1, 0, 0);
Vector3 right(1, 0, 0);
Vector3 forward(0, 0, -1);
Vector3 back(0, 0, 1);

// When we have a 180 degree rotation quaternion which was defined as
// A to B, logically when we transform A we expect to get B.
Quaternion left_to_right(left, right);
Quaternion right_to_left(right, left);
CHECK(left_to_right.xform(left).is_equal_approx(right));
CHECK(Quaternion(right, left).xform(right).is_equal_approx(left));
CHECK(Quaternion(up, down).xform(up).is_equal_approx(down));
CHECK(Quaternion(down, up).xform(down).is_equal_approx(up));
CHECK(Quaternion(forward, back).xform(forward).is_equal_approx(back));
CHECK(Quaternion(back, forward).xform(back).is_equal_approx(forward));

// With (arbitrary) opposite vectors that are not axis-aligned as parameters.
Vector3 diagonal_up = Vector3(1.2, 2.3, 4.5).normalized();
Vector3 diagonal_down = -diagonal_up;
Quaternion q1(diagonal_up, diagonal_down);
CHECK(q1.xform(diagonal_down).is_equal_approx(diagonal_up));
CHECK(q1.xform(diagonal_up).is_equal_approx(diagonal_down));

// For the consistency of the rotation direction, they should be symmetrical to the plane.
CHECK(left_to_right.is_equal_approx(right_to_left.inverse()));
}

TEST_CASE("[Quaternion] Get Euler Orders") {
double x = Math::deg_to_rad(30.0);
double y = Math::deg_to_rad(45.0);
Expand Down