float `rotate_toward()` behaves incorrectly with negative delta

[TokageItLab]

Tested versions

4.3

System information

Any

Issue description

rotate_toward() is implemented by https://github.com/godotengine/godot/pull/80225, but set the incorrect target when delta is negative.

move_toward() when delta is negative, does not reach the original target and goes infinitely negative. It is correct. rotate_toward() can still reach the original target even if delta is negative, but it will erroneously reach the inversed target.

It makes complete no sense to reach the inverted target. If you wanted to do that, you could simply invert the to instead of delta when passing the argument in the first place. Also, if using negative delta result is reaching the inverted target, it means that you cannot use negative delta for rotating turrets with fixed rotation direction and the like.

extends Node2D

func _ready() -> void:
    var scl_pos = 0
    var scl_neg = 0
    var scr_to = 10

    var rad_pos = 0
    var rad_neg = 0
    var rad_to = PI * 0.5

    print("scl src: " + str(0))
    print("scl target: " + str(scr_to))
    print("rad src: " + str(0))
    print("rad target: " + str(rad_to))

    rad_pos = rotate_toward(rad_pos, rad_to, PI * 0.1)
    rad_neg = rotate_toward(rad_neg, rad_to, PI * -0.1)
    scl_pos = move_toward(scl_pos, scr_to, 1)
    scl_neg = move_toward(scl_neg, scr_to, -1)

    print(("scl pos by %s x1: " % str(1)) + str(scl_pos))
    print(("scl neg by %s x1: " % str(-1)) + str(scl_neg))
    print(("rad pos by %s x1: " % str(PI * 0.1)) + str(rad_pos))
    print(("rad neg by %s x1: " % str(PI * -0.1)) + str(rad_neg))

    for i in range(99):
        rad_pos = rotate_toward(rad_pos, rad_to, PI * 0.1)
        rad_neg = rotate_toward(rad_neg, rad_to, PI * -0.1)
        scl_pos = move_toward(scl_pos, scr_to, 1)
        scl_neg = move_toward(scl_neg, scr_to, -1)

    print(("scl pos by %s x100: " % str(1)) + str(scl_pos))
    print(("scl neg by %s x100: " % str(-1)) + str(scl_neg))
    print(("rad pos by %s x100: " % str(PI * 0.1)) + str(rad_pos))
    print(("rad neg by %s x100: " % str(PI * -0.1)) + str(rad_neg))

Output:

scl src: 0
scl target: 10
rad src: 0
rad target: 1.5707963267949
scl pos by 1 x1: 1
scl neg by -1 x1: -1
rad pos by 0.31415926535898 x1: 0.31415926535898
rad neg by -0.31415926535898 x1: -0.31415926535898
scl pos by 1 x100: 10
scl neg by -1 x100: -100
rad pos by 0.31415926535898 x100: 1.5707963267949
rad neg by -0.31415926535898 x100: -1.5707963267949

rad neg by -0.31415926535898 x1 is -0.31415926535898, not 0.31415926535898 it is correct. But rad neg by -0.31415926535898 x100 must be 1.5707963267949, not -1.5707963267949. cc @ettiSurreal

Steps to reproduce

Use rotate_toward() with negative delta to reach the original target.

Minimal reproduction project (MRP)

wrong_rotate_toward_calc.zip

[ettiSurreal]

Negative delta is intended to move towards the opposite angle (as stated by the docs), so this is indented behavior, it wasn't originally in the old PR though. I think it was assumed that the oscillation that occurred at the midway point would've been considered a bug so I "fixed" it after being given a different implementation by @kleonc, originally I even suggested adding 180 degrees to the target as a workaround for that in the old PR. So if the oscillation should to be the correct behavior that can be changed.

[TokageItLab]

Going in the opposite direction and reaching the wrong target are two different things. The opposite direction might mean a path that is not the shortest, but at least should not reach the wrong target in term of rotation.

I think it was assumed that the oscillation that occurred at the midway point would've been considered a bug so I "fixed" it after being given a different implementation by @kleonc

Does the oscillation mean when the shortest path switches due to target movement? If so, such a problem should not occur if positive delta is interpreted as a counterclockwise rotation statically instead of an angle toward the shortest path, although compatibility will be broken. You can consider making it an optional argument rotate_toward(float p_from, float p_to, float p_delta, bool p_shortest = true) or a separate API like rotate_toward_counterclockwise() to prevent compatibility breakdown.

[PhairZ]

This is the code snippet of rotate_toward()

static _ALWAYS_INLINE_ double rotate_toward(double p_from, double p_to, double p_delta) {
    double difference = Math::angle_difference(p_from, p_to);
    double abs_difference = Math::abs(difference);
    // When `p_delta < 0` move no further than to PI radians away from `p_to` (as PI is the max possible angle distance).
    return p_from + CLAMP(p_delta, abs_difference - Math_PI, abs_difference) * (difference >= 0.0 ? 1.0 : -1.0);
}

So I guess it is intentional. My take is that move_towards() with negative delta moves the value as far from p_to as possible, so the author wanted the angle to go as far from p_to as possible.

[TokageItLab]

@PhairZ I'm pointing out that the intent was wrong from the beginning. The direction of rotation is correct, but the destination angle is wrong.

[PhairZ]

@TokageItLab I think i disagree. -delta is intended to go as far as possible from target, not in the opposite direction. working around it is also not that much of a hassle since you can just multiply target by the sign of delta.

[TokageItLab]

@PhairZ In that case, simply pass the inverted target as an argument, instead of inverting delta, as mentioned above. This can be accomplished in a few lines of code, so it does not have much worth of utility function.

Conversely, arbitrarily rotating the longest path cannot be accomplished in a few lines of code, so the behavior makes sense as a utility function.

[PhairZ]

Alright, I'll open a PR on this issue. It's up to the contributers to merge it or not.

[ettiSurreal]

You can consider making it an optional argument rotate_toward(float p_from, float p_to, float p_delta, bool p_shortest = true) or a separate API like rotate_toward_counterclockwise() to prevent compatibility breakdown.

Negative delta usage seems like something that'd be very rarely used regardless, plus since getting the old behavior is rather easy this shouldn't be necessary. Otherwise if there are complaints that could just be addressed in a follow-up.

[TokageItLab]

@ettiSurreal What I mean by compatibility breakdown is that we are talking about oscillation issues, not inverted targets.

The behavior of a negative delta reaching an inverted target definitely should be removed, and since there is a migration way of just inverting the target instead of the delta, there is no need to add an option for that.

There are two ways to go about what to treat as "positive" delta (shortest rotation is positobe, or mathematical positive radian is positive), and I think both are valid.