Godot: 2d is_on_floor() breaks when pass non-unit vector to move_and_slide() as up direction

[revanj]

Godot version:

3.2.1

OS/device including version:

Windows10 64, Quadro M2000M, GLES3

Issue description:

If pass a non-unit vector to KinematicBody2D move_and_slide() as up direction, such as Vector2(0,-10), is_on_floor() breaks. I believe this is because line 1270 in physics_body_2d.cpp:

Which relies on acos() getting a value in [-1, 1]
This can easily be fixed by normalizing the p_up_direction before calculation.
Steps to reproduce:
As detailed above.

Minimal reproduction project:
MoveAndSlideBug.zip

Note:
I believe simply adding a .normalized() can fix this bug.
This might be the desired behavior. If so, it just seem like a nice feature to have such that even non-unit vectors can be passed in as up direction.
The documentation didn't mention this behavior.
And I'm not sure if similar things happen in 3D as well.

[Calinou]

I believe simply adding a .normalized() can fix this bug.
This might be the desired behavior. If so, it just seem like a nice feature to have such that even non-unit vectors can be passed in as up direction.

Normalizing a vector has a cost, even if the vector passed is already normalized. This is why most engine methods won't do it for you - they'll just check whether the vector is already normalized and report an error if it isn't.

If error checking is missing, we should add it.

[revanj]

I believe simply adding a .normalized() can fix this bug.
This might be the desired behavior. If so, it just seem like a nice feature to have such that even non-unit vectors can be passed in as up direction.

Normalizing a vector has a cost, even if the vector passed is already normalized. This is why most engine methods won't do it for you - they'll just check whether the vector is already normalized and report an error if it isn't.

If error checking is missing, we should add it.

I realized that checking for normal has basically the same amount of calculation as normalization. I'm not sure if this is a good idea anymore. Maybe changing the docs would suffice. Still leaving this as open.

[avencherus]

@JiRuifanCR Depends on if the check is something that gets removed in a release build.

[lawnjelly]

Either way the acos should be a safe_acos to prevent NaNs. In fact probably most uses in the engine should be a safe acos.

static _ALWAYS_INLINE_ float safe_acos(float p_x)
{
if (p_x < -1.0f)
    p_x = -1.0f;
if (p_x > 1.0f)
    p_x = 1.0f;

 return ::acosf(p_x);
}

Even when fed 'correct' data, floating point error will occasionally give values slightly above 1.0 or below 1.0 resulting in a NaN.

Edit : It might be better to make the standard acos safe, and offer another optional unsafe_acos.