Basis::slerp inefficiency

Godot version

3.4 stable

System information

All

Issue description

The Basis::slerp() function has a couple of related problems:

Basis Basis::slerp(const Basis &p_to, const real_t &p_weight) const {
    //consider scale
    Quat from(*this);
    Quat to(p_to);

    Basis b(from.slerp(to, p_weight));
    b.elements[0] *= Math::lerp(elements[0].length(), p_to.elements[0].length(), p_weight);
    b.elements[1] *= Math::lerp(elements[1].length(), p_to.elements[1].length(), p_weight);
    b.elements[2] *= Math::lerp(elements[2].length(), p_to.elements[2].length(), p_weight);

    return b;
}

1) Because it relies on conversion of both Basis to Quats, this conversion routine flags an error if the basis is not normalized. This means that the slerp routine is not usable except for normalized basis, whereas a slerp can actually be done provided both basis are orthogonal (scaling can be dealt with with some jiggery pokery, see #52846) (and are non zero).

2) The second problem is that although the first means that by definition both input basis are normalized, this means that the output basis by definition will also be normalized. This means that the second step of lerping each basis to interpolate between them is redundant, and wasting performance.

Steps to reproduce

    # basis test
    var b0 : Basis = Basis(Vector3(0, 1, 0), 0.3)
    var b1 : Basis = Basis(Vector3(0, 1, 0), 0.6)
    b0 = b0.scaled(Vector3(0.2, 0.2, 0.2))
    var b2 = b0.slerp(b1, 0.5)

Flags an error:

E 0:00:00.421   get_quat: Basis must be normalized in order to be casted to a Quaternion. Use get_rotation_quat() or call orthonormalized() if the Basis contains linearly independent vectors.
  <C++ Error>   Condition "!is_rotation()" is true. Returned: Quat()
  <C++ Source>  core/math/basis.cpp:768 @ get_quat()
  <Stack Trace> PS.gd:21 @ _ready()

Discussion

I noticed these problems while writing some custom slerping code for the fixed timestep interpolation.

Imo there are two ways of addressing this: 1) We fix Basis::slerp() to work with scaled basis (or provide an alternative more general purpose robust version). 2) We remove the lerping code.

Another related issue is that the epsilons used for all the checks involved (basis to quat etc etc) are rather arbitrary. This makes them useless for production code such as the fixed timestep interpolation as it has to deal with much larger floating point error.

We could consider making the epsilons overridable via default parameters. We use this for example in a few is_equal_approximate functions elsewhere. This is a general problem in godot, there is a gulf between the ideal world when one epsilon fits all, and the real world, where the epsilon is situation dependent. For now I'm duplicating this functionality to some extent in the new TransformInterpolator code, but it seems a bit redundant.

Related issue #43320.

godotengine / godot