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.

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

Add is_z_forward option to looking_at() & look_at() and some constants for glTF space #72795

Closed
wants to merge 1 commit into from
Closed

Add is_z_forward option to looking_at() & look_at() and some constants for glTF space #72795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions core/math/basis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,12 +1016,12 @@ void Basis::rotate_sh(real_t *p_values) {
p_values[8] = d4 * s_scale_dst4;
}

Basis Basis::looking_at(const Vector3 &p_target, const Vector3 &p_up) {
Basis Basis::looking_at(const Vector3 &p_target, const Vector3 &p_up, bool p_is_z_forward) {
#ifdef MATH_CHECKS
ERR_FAIL_COND_V_MSG(p_target.is_zero_approx(), Basis(), "The target vector can't be zero.");
ERR_FAIL_COND_V_MSG(p_up.is_zero_approx(), Basis(), "The up vector can't be zero.");
#endif
Vector3 v_z = -p_target.normalized();
Vector3 v_z = p_is_z_forward ? p_target.normalized() : -p_target.normalized();
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.");
Expand Down
2 changes: 1 addition & 1 deletion core/math/basis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ struct _NO_DISCARD_ Basis {

operator Quaternion() const { return get_quaternion(); }

static Basis looking_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0));
static Basis looking_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0), bool p_is_z_forward = false);

Basis(const Quaternion &p_quaternion) { set_quaternion(p_quaternion); };
Basis(const Quaternion &p_quaternion, const Vector3 &p_scale) { set_quaternion_scale(p_quaternion, p_scale); }
Expand Down
8 changes: 4 additions & 4 deletions core/math/transform_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,20 @@ void Transform3D::rotate_basis(const Vector3 &p_axis, real_t p_angle) {
basis.rotate(p_axis, p_angle);
}

Transform3D Transform3D::looking_at(const Vector3 &p_target, const Vector3 &p_up) const {
Transform3D Transform3D::looking_at(const Vector3 &p_target, const Vector3 &p_up, bool p_is_z_forward) const {
#ifdef MATH_CHECKS
ERR_FAIL_COND_V_MSG(origin.is_equal_approx(p_target), Transform3D(), "The transform's origin and target can't be equal.");
#endif
Transform3D t = *this;
t.basis = Basis::looking_at(p_target - origin, p_up);
t.basis = Basis::looking_at(p_target - origin, p_up, p_is_z_forward);
return t;
}

void Transform3D::set_look_at(const Vector3 &p_eye, const Vector3 &p_target, const Vector3 &p_up) {
void Transform3D::set_look_at(const Vector3 &p_eye, const Vector3 &p_target, const Vector3 &p_up, bool p_is_z_forward) {
#ifdef MATH_CHECKS
ERR_FAIL_COND_MSG(p_eye.is_equal_approx(p_target), "The eye and target vectors can't be equal.");
#endif
basis = Basis::looking_at(p_target - p_eye, p_up);
basis = Basis::looking_at(p_target - p_eye, p_up, p_is_z_forward);
origin = p_eye;
}

Expand Down
4 changes: 2 additions & 2 deletions core/math/transform_3d.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ struct _NO_DISCARD_ Transform3D {
void rotate(const Vector3 &p_axis, real_t p_angle);
void rotate_basis(const Vector3 &p_axis, real_t p_angle);

void set_look_at(const Vector3 &p_eye, const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0));
Transform3D looking_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0)) const;
void set_look_at(const Vector3 &p_eye, const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0), bool p_is_z_forward = false);
Transform3D looking_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0), bool p_is_z_forward = false) const;

void scale(const Vector3 &p_scale);
Transform3D scaled(const Vector3 &p_scale) const;
Expand Down
16 changes: 14 additions & 2 deletions core/variant/variant_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2083,7 +2083,7 @@ static void _register_variant_builtin_methods() {
bind_method(Basis, is_equal_approx, sarray("b"), varray());
bind_method(Basis, is_finite, sarray(), varray());
bind_method(Basis, get_rotation_quaternion, sarray(), varray());
bind_static_method(Basis, looking_at, sarray("target", "up"), varray(Vector3(0, 1, 0)));
bind_static_method(Basis, looking_at, sarray("target", "up", "is_z_forward"), varray(Vector3(0, 1, 0), false));
bind_static_method(Basis, from_scale, sarray("scale"), varray());
bind_static_method(Basis, from_euler, sarray("euler", "order"), varray((int64_t)EulerOrder::YXZ));

Expand Down Expand Up @@ -2126,7 +2126,7 @@ static void _register_variant_builtin_methods() {
bind_method(Transform3D, scaled_local, sarray("scale"), varray());
bind_method(Transform3D, translated, sarray("offset"), varray());
bind_method(Transform3D, translated_local, sarray("offset"), varray());
bind_method(Transform3D, looking_at, sarray("target", "up"), varray(Vector3(0, 1, 0)));
bind_method(Transform3D, looking_at, sarray("target", "up", "is_z_forward"), varray(Vector3(0, 1, 0), false));
bind_method(Transform3D, interpolate_with, sarray("xform", "weight"), varray());
bind_method(Transform3D, is_equal_approx, sarray("xform"), varray());
bind_method(Transform3D, is_finite, sarray(), varray());
Expand Down Expand Up @@ -2512,6 +2512,12 @@ static void _register_variant_builtin_methods() {
_VariantCall::add_variant_constant(Variant::VECTOR3, "DOWN", Vector3(0, -1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "FORWARD", Vector3(0, 0, -1));
_VariantCall::add_variant_constant(Variant::VECTOR3, "BACK", Vector3(0, 0, 1));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_LEFT", Vector3(1, 0, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_RIGHT", Vector3(-1, 0, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_UP", Vector3(0, 1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_DOWN", Vector3(0, -1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_FORWARD", Vector3(0, 0, 1));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_BACK", Vector3(0, 0, -1));
Copy link
Member

Choose a reason for hiding this comment

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

I am in favor of the looking_at changes, but not these. Perhaps this should be a separate PR? I think if we had a PR just for the looking_at changes this would be easier to approve.

Copy link
Member Author

@TokageItLab TokageItLab Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose this is necessary as a basis/reasoning for the is_z_forward option. The documentation also describes glTF forward (+Z).

The +Z concept remains when it comes to importing/exporting assets. For example, it feels strange to specify BACK if you want the model to face forward for retargeting/exporting glTF.

Copy link
Member

@aaronfranke aaronfranke Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, it's not tied to glTF. Here's a PR with just the look_at changes: #75875

You could have +Z forward when importing an FBX or Collada file for example. Or you could have -Z forward in glTF if you are controlling a glTF camera which has its forward pointing -Z.

Copy link
Member Author

@TokageItLab TokageItLab Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or you could have -Z forward in glTF if you are controlling a glTF camera which has its forward pointing -Z.

Well, even if the enum GLTF_FORWARD is an exaggeration, it is a SPECIFICATION that all of the assets as a GLTF are +Z FORWARD. A GLTF with a -Z FORWARD is not a GLTF. It is what is called a FAKE GLTF. GLTF cameras are out of scope here. The term "asset" may include the camera, but let's assume here that it generally refers to the object that the camera is shooting at. In other words, it means that the camera takes a picture of the camera.

There may be a better enum (e.g. ASSET_FORWARD), but even if we can't find it, we must document somewhere that as long as Godot uses the GLTF importer for all formats and has a GLTF exporter, the asset must be +Z faced. Even if the word "FORWARD" is not used, it must be made clear that they are to be faced to +Z. (Root motion is also needed that document.)

If you want to argue that assets should be -Z FORWARD in Godot, then the Godot importer and exporter must correctly convert the +Z FORWARD GLTF as -Z FORWARD, but unfortunately this is not accomplished in Godot 4.x was.


_VariantCall::add_constant(Variant::VECTOR4, "AXIS_X", Vector4::AXIS_X);
_VariantCall::add_constant(Variant::VECTOR4, "AXIS_Y", Vector4::AXIS_Y);
Expand Down Expand Up @@ -2555,6 +2561,12 @@ static void _register_variant_builtin_methods() {
_VariantCall::add_variant_constant(Variant::VECTOR3I, "DOWN", Vector3i(0, -1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "FORWARD", Vector3i(0, 0, -1));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "BACK", Vector3i(0, 0, 1));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_LEFT", Vector3i(1, 0, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_RIGHT", Vector3i(-1, 0, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_UP", Vector3i(0, 1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_DOWN", Vector3i(0, -1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_FORWARD", Vector3i(0, 0, 1));
_VariantCall::add_variant_constant(Variant::VECTOR3I, "GLTF_BACK", Vector3i(0, 0, -1));

_VariantCall::add_constant(Variant::VECTOR2, "AXIS_X", Vector2::AXIS_X);
_VariantCall::add_constant(Variant::VECTOR2, "AXIS_Y", Vector2::AXIS_Y);
Expand Down
2 changes: 2 additions & 0 deletions doc/classes/Basis.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,11 @@
<return type="Basis" />
<param index="0" name="target" type="Vector3" />
<param index="1" name="up" type="Vector3" default="Vector3(0, 1, 0)" />
<param index="2" name="is_z_forward" type="bool" default="false" />
<description>
Creates a Basis with a rotation such that the forward axis (-Z) points towards 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 resulting Basis is orthonormalized. The [param target] and [param up] vectors cannot be zero, and cannot be parallel to each other.
If [param is_z_forward] is [code]true[/code], the glTF forward axis (+Z) points towards the [param target] position.
</description>
</method>
<method name="orthonormalized" qualifiers="const">
Expand Down
3 changes: 3 additions & 0 deletions doc/classes/Node3D.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,18 +112,21 @@
<return type="void" />
<param index="0" name="target" type="Vector3" />
<param index="1" name="up" type="Vector3" default="Vector3(0, 1, 0)" />
<param index="2" name="is_z_forward" type="bool" default="false" />
<description>
Rotates the node so that the local forward axis (-Z) 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.
Operations take place in global space, which means that the node must be in the scene tree.
If [param is_z_forward] is [code]true[/code], the glTF forward axis (+Z) points towards the [param target] position.
</description>
</method>
<method name="look_at_from_position">
<return type="void" />
<param index="0" name="position" type="Vector3" />
<param index="1" name="target" type="Vector3" />
<param index="2" name="up" type="Vector3" default="Vector3(0, 1, 0)" />
<param index="3" name="is_z_forward" type="bool" default="false" />
<description>
Moves the node to the specified [param position], and then rotates the node to point toward the [param target] as per [method look_at]. Operations take place in global space.
</description>
Expand Down
2 changes: 2 additions & 0 deletions doc/classes/Transform3D.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,11 @@
<return type="Transform3D" />
<param index="0" name="target" type="Vector3" />
<param index="1" name="up" type="Vector3" default="Vector3(0, 1, 0)" />
<param index="2" name="is_z_forward" type="bool" default="false" />
<description>
Returns a copy of the transform rotated such that the forward axis (-Z) points towards 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 resulting transform is orthonormalized. The existing rotation, scale, and skew information from the original transform is discarded. The [param target] and [param up] vectors cannot be zero, cannot be parallel to each other, and are defined in global/parent space.
If [param is_z_forward] is [code]true[/code], the glTF forward axis (+Z) points towards the [param target] position.
</description>
</method>
<method name="orthonormalized" qualifiers="const">
Expand Down
18 changes: 18 additions & 0 deletions doc/classes/Vector3.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,24 @@
<constant name="BACK" value="Vector3(0, 0, 1)">
Back unit vector. Represents the local direction of back, and the global direction of south.
</constant>
<constant name="GLTF_LEFT" value="Vector3(1, 0, 0)">
Left unit vector in glTF space.
</constant>
<constant name="GLTF_RIGHT" value="Vector3(-1, 0, 0)">
Right unit vector in glTF space.
</constant>
<constant name="GLTF_UP" value="Vector3(0, 1, 0)">
Up unit vector in glTF space.
</constant>
<constant name="GLTF_DOWN" value="Vector3(0, -1, 0)">
Down unit vector in glTF space.
</constant>
<constant name="GLTF_FORWARD" value="Vector3(0, 0, 1)">
Forward unit vector in glTF space.
</constant>
<constant name="GLTF_BACK" value="Vector3(0, 0, -1)">
Back unit vector in glTF space.
</constant>
</constants>
<operators>
<operator name="operator !=">
Expand Down
18 changes: 18 additions & 0 deletions doc/classes/Vector3i.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,24 @@
<constant name="BACK" value="Vector3i(0, 0, 1)">
Back unit vector. Represents the local direction of back, and the global direction of south.
</constant>
<constant name="GLTF_LEFT" value="Vector3i(1, 0, 0)">
Left unit vector in glTF space.
</constant>
<constant name="GLTF_RIGHT" value="Vector3i(-1, 0, 0)">
Right unit vector in glTF space.
</constant>
<constant name="GLTF_UP" value="Vector3i(0, 1, 0)">
Up unit vector in glTF space.
</constant>
<constant name="GLTF_DOWN" value="Vector3i(0, -1, 0)">
Down unit vector in glTF space.
</constant>
<constant name="GLTF_FORWARD" value="Vector3i(0, 0, 1)">
Forward unit vector in glTF space.
</constant>
<constant name="GLTF_BACK" value="Vector3i(0, 0, -1)">
Back unit vector in glTF space.
</constant>
</constants>
<operators>
<operator name="operator !=">
Expand Down
12 changes: 6 additions & 6 deletions scene/3d/node_3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,18 +809,18 @@ void Node3D::set_identity() {
set_transform(Transform3D());
}

void Node3D::look_at(const Vector3 &p_target, const Vector3 &p_up) {
void Node3D::look_at(const Vector3 &p_target, const Vector3 &p_up, bool p_is_z_forward) {
ERR_FAIL_COND_MSG(!is_inside_tree(), "Node not inside tree. Use look_at_from_position() instead.");
Vector3 origin = get_global_transform().origin;
look_at_from_position(origin, p_target, p_up);
look_at_from_position(origin, p_target, p_up, p_is_z_forward);
}

void Node3D::look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up) {
void Node3D::look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up, bool p_is_z_forward) {
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.");

Transform3D lookat = Transform3D(Basis::looking_at(p_target - p_pos, p_up), p_pos);
Transform3D lookat = Transform3D(Basis::looking_at(p_target - p_pos, p_up, p_is_z_forward), p_pos);
Vector3 original_scale = get_scale();
set_global_transform(lookat);
set_scale(original_scale);
Expand Down Expand Up @@ -1056,8 +1056,8 @@ void Node3D::_bind_methods() {
ClassDB::bind_method(D_METHOD("orthonormalize"), &Node3D::orthonormalize);
ClassDB::bind_method(D_METHOD("set_identity"), &Node3D::set_identity);

ClassDB::bind_method(D_METHOD("look_at", "target", "up"), &Node3D::look_at, DEFVAL(Vector3(0, 1, 0)));
ClassDB::bind_method(D_METHOD("look_at_from_position", "position", "target", "up"), &Node3D::look_at_from_position, DEFVAL(Vector3(0, 1, 0)));
ClassDB::bind_method(D_METHOD("look_at", "target", "up", "is_z_forward"), &Node3D::look_at, DEFVAL(Vector3(0, 1, 0)), DEFVAL(false));
ClassDB::bind_method(D_METHOD("look_at_from_position", "position", "target", "up", "is_z_forward"), &Node3D::look_at_from_position, DEFVAL(Vector3(0, 1, 0)), DEFVAL(false));

ClassDB::bind_method(D_METHOD("to_local", "global_point"), &Node3D::to_local);
ClassDB::bind_method(D_METHOD("to_global", "local_point"), &Node3D::to_global);
Expand Down
4 changes: 2 additions & 2 deletions scene/3d/node_3d.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,8 @@ class Node3D : public Node {
void global_scale(const Vector3 &p_scale);
void global_translate(const Vector3 &p_offset);

void look_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0));
void look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0));
void look_at(const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0), bool p_is_z_forward = false);
void look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up = Vector3(0, 1, 0), bool p_is_z_forward = false);

Vector3 to_local(Vector3 p_global) const;
Vector3 to_global(Vector3 p_local) const;
Expand Down