Add move_toward_angle and angle_difference methods

[ettiSurreal]

Closes https://github.com/godotengine/godot-proposals/issues/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 https://github.com/godotengine/godot-proposals/issues/5596#issuecomment-1282661563 and https://github.com/godotengine/godot/pull/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

[AThousandShips]

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

[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. :upside_down_face: 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))? :thinking:

[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 https://github.com/godotengine/godot/pull/69081#issuecomment-1601200040 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.

[ettiSurreal]

Wrapped the angle between [-PI, PI], since that's how lerp_angle seems to work. For now decided to keep negative delta as "unsupported" for optimization (let me know if i should do otherwise), clarifying it in the docs, which I also just rewrote.

[kleonc]

Wrapped the angle between [-PI, PI], since that's how lerp_angle seems to work.

@ettiSurreal lerp_angle doesn't do any wrapping (and its behavior should remain unchanged as it would be a compatibility breaking change). Hence I think leaving move_toward_angle without wrapping the result to a specific range is the way to go. If desired, user can always wrap it manually, e.g. wrapf(move_toward_angle(from, to, delta), -PI, PI).

For now decided to keep negative delta as "unsupported" for optimization (let me know if i should do otherwise), clarifying it in the docs, which I also just rewrote.

Performance shouldn't differ much, I think the implementation would be as simple as:

static _ALWAYS_INLINE_ double move_toward_angle(double p_from, double p_to, double p_delta) {
	double difference = angle_difference(p_from, p_to);
	double abs_difference = abs(difference);
	// When `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(delta, abs_difference - Math_PI, abs_difference) * SIGN(difference);
}

---

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

@PrecisionRender Agree consistency is good but the thing is these already work differently as lerp_angle is not limiting the result in any direction. You can extrapolate by passing either weight > 1 or weight < -1, e.g. lerp_angle(0, PI, 99) returns 99 * PI. To me move_toward_angle conceptually behaves differently, it's main purpose is to perform logic limiting the movement against the to parameter. Hence when moving away it makes sense to me to not move past 180 degrees away from to. So I'd limit it like that. But if others disagree and it's more desired to not limit moving away anyhow (possibly going full circles), it would be fine to me too. :upside_down_face:

[ettiSurreal]

Wrapped the angle between [-PI, PI], since that's how lerp_angle seems to work.

@ettiSurreal lerp_angle doesn't do any wrapping (and its behavior should remain unchanged as it would be a compatibility breaking change). Hence I think leaving move_toward_angle without wrapping the result to a specific range is the way to go. If desired, user can always wrap it manually, e.g. wrapf(move_toward_angle(from, to, delta), -PI, PI).

I must've mistested it, I thought lerp_angle by itself stayed between [-PI, PI] unless you overloaded the weight.

For now decided to keep negative delta as "unsupported" for optimization (let me know if i should do otherwise), clarifying it in the docs, which I also just rewrote.

Performance shouldn't differ much, I think the implementation would be as simple as:

static _ALWAYS_INLINE_ double move_toward_angle(double p_from, double p_to, double p_delta) {
	double difference = angle_difference(p_from, p_to);
	double abs_difference = abs(difference);
	// When `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(delta, abs_difference - Math_PI, abs_difference) * SIGN(difference);
}

Since that's seemingly not a concern I can add your implementation then. My own implementation would've been something like this.

static _ALWAYS_INLINE_ double move_toward_angle(double p_from, double p_to, double p_delta) {
	if (p_delta > 0.0) {
		double difference = Math::angle_difference(p_from, p_to);
		return Math::wrapf(p_from + MIN(Math::abs(difference), p_delta) * SIGN(difference), -Math_PI, Math_PI);
	} else {
		double difference = Math::angle_difference(p_from, p_to + Math.PI);
		return Math::wrapf(p_from + MIN(Math::abs(difference), Math::abs(p_delta)) * SIGN(difference), -Math_PI, Math_PI);		

[PrecisionRender]

@PrecisionRender Agree consistency is good but the thing is these already work differently as lerp_angle is not limiting the result in any direction. You can extrapolate by passing either weight > 1 or weight < -1, e.g. lerp_angle(0, PI, 99) returns 99 * PI. To me move_toward_angle conceptually behaves differently, it's main purpose is to perform logic limiting the movement against the to parameter. Hence when moving away it makes sense to me to not move past 180 degrees away from to. So I'd limit it like that. But if others disagree and it's more desired to not limit moving away anyhow (possibly going full circles), it would be fine to me too. 🙃

I've never run into a scenario where I need to do that moving away from the desired angle, so I imagine either implementation would work. I'm in the same boat, whatever the majority want is good for me.

[AncientSion]

can you add this to 3.5/6, i need it badly.

[AThousandShips]

This being a new feature it can be considered for cherry-picking, unsure if that will be done though

While this might be trivial to backport there's no guarantee that any new feature gets backported, as it involves extra work and testing

[timothyqiu]

Any reason why the function is named move_toward_angle instead of rotate_toward? I think it's technically not possible to move toward an angle :P

[PrecisionRender]

Any reason why the function is named move_toward_angle instead of rotate_toward? I think it's technically not possible to move toward an angle :P

I don't disagree, but I think these are the current reasons:

a. To be consistent with the lerp_angle() function b. To be consistent with other engines

[timothyqiu]

a. To be consistent with the lerp_angle() function

Make sense. But to be fair, "lerp" is one term (same goes for "cubic_interpolate"). The only choice for naming a rotation variant is adding a prefix or suffix :stuck_out_tongue:

b. To be consistent with other engines

Unity has inconsistent naming: the standalone version is named MoveTowardsAngle, while the member version is named RotateTowards.