Add move_toward_angle and angle_difference methods

[ettiSurreal]

Closes godotengine/godot-proposals#2074

Adds:

• angle_difference, which finds a value between 0 and TAU, finding the shortest path.
• move_toward_angle, moves towards an angle by an increment, wrapping around TAU and also finding the shortest path, similar to lerp_angle.
• modifies lerp_angle code to avoid repetition.

Implementation based on godotengine/godot-proposals#5596 (comment) and #37078.

Tested on a custom build, everything seems to be working as it should.

This is my first time making a PR, so let me know what to do if i messed something up!!

[ettiSurreal]

Since 4.0 is in feature freeze and since .NET 6 isn't mentioned in the compiling docs i assume building the C# version isn't fully documented yet, i'll most likely just wait for 4.0 to go in stable before updating the PR again.
EDIT: I thought i absolutely had to use the doctool for something, but turns out i just didn't know the docs have to be ordered alphabetically.

[adamscoble]

@ettiSurreal Oh, this is great to see! Though to stay in line with other functions, should angle_difference just be called angle?

Also one other consideration is that in the move_toward_angle method, I offset the start value instead of the end, so that the end value is actually reached. Most people will just be testing for the angle anyway, but I do like that idea that the target value is the result of the method, rather than a possible multiple of TAU from it?

Here's the plugin I made that implements it in case you're curious!

[PrecisionRender]

To make things easy to track, make sure to squash your commits into a single commit. 👍

[Zireael07]

Bump. What's the state of this? Can this go into 4.0 or is this something that's for 4.1?

[Calinou]

Bump. What's the state of this? Can this go into 4.0 or is this something that's for 4.1?

Any non-critical change is for 4.1 at the earliest now.

[ettiSurreal]

I couldn't squash the commits on this branch since Git Bash stopped working for me, and Github Desktop just keeps throwing an "Unable to squash" error.
I hope that's not big of an issue, sorry.

[PrecisionRender]

Hopefully this can get added by 4.2.

[AThousandShips]

I hope that's not big of an issue, sorry.

You will have to squash before this can be merged once approved, hopefully you can get it to work, a maintainer can attempt to do so however

[ettiSurreal]

I hope that's not big of an issue, sorry.

You will have to squash before this can be merged once approved, hopefully you can get it to work, a maintainer can attempt to do so however

I feel like i may have just mishandled doing it, so if approved I'll probably just redo the PR/Branch.

[AThousandShips]

I'd suggest either trying to fix it or do that before any approval, rather than waiting as reviewing would also depend on it being up to date and possible to test properly against the current codebase

[ettiSurreal]

After attempting to rebase your code and fix the issues I am at a loss, I think you just need to start a new branch, and make sure in the future to not merge the master branch into your branch but to rebase, see the PR workflow

I wanted to try doing something myself one more time but I guess you beat me to it due to my atm painfully slow internet, other than that i only used Github's Sync Fork because it seemed to do the job, and I'm still very new to this.
I'll make the new branch probably after 4.1 releases.

[AThousandShips]

It's hard, so no worries, I've struggled at the start too!

[kleonc]

I'll try to rebase. 🙃 Edit: Rebased. I think I haven't changed anything besides the commit name.

Generally the PR looks ok, I'd say the docs need some more clarification. But I'm not fully sure what's the desired behavior in the first place. E.g. for the current implementation:

move_toward_angle(deg_to_rad(3600), deg_to_rad(90), deg_to_rad(45)) # Returns: deg_to_rad(3645)
move_toward_angle(deg_to_rad(90), deg_to_rad(3600), deg_to_rad(45)) # Returns: deg_to_rad(45)

move_toward_angle(deg_to_rad(0), deg_to_rad(90), deg_to_rad(3600)) # Returns: deg_to_rad(90)
move_toward_angle(deg_to_rad(0), deg_to_rad(90), deg_to_rad(-3600)) # Returns: deg_to_rad(-3600)

Is this expected/desired? Or do we maybe want to somehow limit the results to some range (like [0, TAU))? 🤔

[ettiSurreal]

I'll try to rebase. 🙃 Edit: Rebased. I think I haven't changed anything besides the commit name.

Thanks for the help!

Generally the PR looks ok, I'd say the docs need some more clarification. But I'm not fully sure what's the desired behavior in the first place. E.g. for the current implementation:

move_toward_angle(deg_to_rad(3600), deg_to_rad(90), deg_to_rad(45)) # Returns: deg_to_rad(3645)
move_toward_angle(deg_to_rad(90), deg_to_rad(3600), deg_to_rad(45)) # Returns: deg_to_rad(45)

move_toward_angle(deg_to_rad(0), deg_to_rad(90), deg_to_rad(3600)) # Returns: deg_to_rad(90)
move_toward_angle(deg_to_rad(0), deg_to_rad(90), deg_to_rad(-3600)) # Returns: deg_to_rad(-3600)

Is this expected/desired? Or do we maybe want to somehow limit the results to some range (like [0, TAU))? 🤔

As for the docs i am not the best at writing those, I think someone might rewrite this sometime.
As for the behavior, it's the same logic as lerp_angle, I'll do some tests tomorrow to see if i messed something up.

[adamscoble]

As for the docs i am not the best at writing those, I think someone might rewrite this sometime. As for the behavior, it's the same logic as lerp_angle, I'll do some tests tomorrow to see if i messed something up.

In case it helps, I wrote my function docs (and names) to mirror Godot's, so feel free to pinch them if you'd like!

Also regarding @kleonc's comment, in my GDScript implementation the result of move_toward_angle is always to, which I believe is more intuitive than to + TAU * x.

[PrecisionRender]

@adamscoble I just straight-up copied the lerp_angle function code in my implementation.

static func move_toward_angle(from: float, to: float, delta: float) -> float:
	var difference = fmod(to - from, TAU)
	var distance = fmod(2.0 * difference, TAU) - difference
	return move_toward(from, from + distance, delta)

[adamscoble]

@adamscoble I just straight-up copied the lerp_angle function code in my implementation.

Oh, well there you go! I don't use the lerp functions often for this stuff — I created a float smoothing class in that same repository to avoid the framerate-dependent smoothing of lerp. So it seems you're being consistent with Godot!

[kleonc]

Also regarding @kleonc's comment, in my GDScript implementation the result of move_toward_angle is always to, which I believe is more intuitive than to + TAU * x.

@adamscoble Your implementation behaves the same as the one in this PR, note the examples I gave in #69081 (comment) are kinda edge cases (like why would you want to move away by 3600 degrees?). For both implementations if delta < 0 the result is not being clamped whatsoever.

For the move_toward/Vector2.move_toward/Vector3.move_toward it makes sense that delta < 0 results in moving away from to without any clamping/limiting. The relevant 1D/2D/3D spaces are conceptually unlimited, meaning you can always move further away.
But "angle space" is different, it's periodic. You can't really move further away than 180 degrees from the given angle. So maybe move_toward_angle for delta < 0 should be clamped/limited to [to - PI, to + PI] range (whether it should be wrapped to a range like [0, TAU) or [-PI, PI) is a different question).

As a simple example, what should be the result of:

move_toward_angle(deg_to_rad(90), deg_to_rad(0), deg_to_rad(-180))

Which can be interpreted as: we're at 90 degrees and we're moving 180 degrees away from 0 degrees.
Currently the result would be 270 degrees. I say maybe it should be clamped so the result would be 180 degrees (because when moving past 180 degrees we would start getting closer to the 0 degrees we were meant to be moving away from).

But maybe the current behavior is desired, I don't know. What I'm saying is that firstly the expected behavior should be well defined for all possible inputs. Tweaking the implementation/docs accordingly is the next step.

[PrecisionRender]

But maybe the current behavior is desired, I don't know. What I'm saying is that firstly the expected behavior should be well defined for all possible inputs. Tweaking the implementation/docs accordingly is the next step.

I'd say it should work the same as lerp_angle, for the sake of consistency.

[ettiSurreal]

Tested some things out in a GDScript port of the functions.
Clamping the value between [0.0, TAU]/[-PI, PI] seems to be as simple as using a wrap function on the return, I guess i should add that?
Negative values are seem to be an issue. Currently (if the wrap function is added),

move_toward_angle(deg_to_rad(90), deg_to_rad(0), deg_to_rad(-180))

would return -90 degrees, since negative values currently do not support stopping at at the opposite angle. Which would mean if you run it per frame it will oscillate.
However, if you wanna move toward to the opposite angle you can just add 180 degrees to p_to and use a positive delta. I found a simple way to make it so the function does it automatically when you input a negative value, but it requires an If statement at the start of the function. Unity's equivalent function omits automatically doing it for optimization reasons, should we as well?

Also I'm sorry, I'm still unable to squash the commits, I get an error when i attempt to use $ git rebase -i upstream/master and $ git fetch upstream master.