godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Change behavior of `look_at()` and `look_at_from_position()` #11041

Open Jesusemora opened 3 weeks ago

Jesusemora commented 3 weeks ago

Describe the project you are working on

3D game

Describe the problem or limitation you are having in your project

this is a very common problem with the look_at and look_at_from_position functions

void look_at(target: Vector3, up: Vector3 = Vector3(0, 1, 0), use_model_front: bool = false)

Rotates the node so that the local forward axis (-Z, Vector3.FORWARD) points toward the target position.

The local up axis (+Y) points as close to the 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 target position cannot be the same as the node's position, the up vector cannot be zero, and the direction from the node's position to the target vector cannot be parallel to the up vector.

Operations take place in global space, which means that the node must be in the scene tree.

If use_model_front is true, the +Z axis (asset front) is treated as forward (implies +X is left) and points toward the target position. By default, the -Z axis (camera forward) is treated as forward (implies +X is right).

void look_at_from_position(position: Vector3, target: Vector3, up: Vector3 = Vector3(0, 1, 0), use_model_front: bool = false)

Moves the node to the specified position, and then rotates the node to point toward the target as per look_at. Operations take place in global space.

when the Vectors are aligned or equal to zero they throw an error. the error shows that the engine is testing for these things.

the current solution is to do something like:

if (stl - k).cross(Vector3.UP).is_zero_approx():
    tmp.look_at_from_position(k, stl, Vector3.UP)

but this means that we are performing the check twice, in gdscript and in the engine, it also adds unnecessary complexity.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

just get rid of the error, we don't need to know that it failed and doing the check twice is redundant. this would greatly simplify things.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I believe this is the code:

void Node3D::look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up, bool p_use_model_front) {
    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);
    Vector3 original_scale = get_scale();
    set_global_transform(Transform3D(lookat_basis, p_pos));
    set_scale(original_scale);
}

we could instead do a simple check

void Node3D::look_at_from_position(const Vector3 &p_pos, const Vector3 &p_target, const Vector3 &p_up, bool p_use_model_front) {
    if(!p_pos.is_equal_approx(p_target) & !p_up.is_zero_approx() & !p_up.cross(p_target - p_pos).is_zero_approx())
    {
        Vector3 forward = p_target - p_pos;
        Basis lookat_basis = Basis::looking_at(forward, p_up, p_use_model_front);
        Vector3 original_scale = get_scale();
        set_global_transform(Transform3D(lookat_basis, p_pos));
        set_scale(original_scale);
    }
}

If this enhancement will not be used often, can it be worked around with a few lines of script?

yes, but this is to reduce redundancy and improve usability.

Is there a reason why this should be core and not an add-on in the asset library?

because it seems relatively easy

nkrisc commented 3 weeks ago

I don't think removing the error case from these methods is correct, but I also do understand your point and agree to some degree as I've encountered the same issue before. There are times when if it fails I'd rather nothing happens and I don't need nor want to see the error message. I typically run into this case when creating @tool scripts where sometimes a node is aligned with the up vector just because I haven't had a chance to move them yet.

That said, I think the error message is important to indicate why the method apparently did nothing, especially for newer users.

So if there's any change to make, perhaps its a "fallible" version of these methods, sort of akin to Node.get_node_or_null where it's explicitly acceptable if the node is not found. Maybe something like try_look_at or try_look_at_from_position. where in these error conditions outlined, no error is emitted and nothing happens.