Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rotate_toward to Vector2, Vector3, Basis and Quaternion #82926

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ettiSurreal
Copy link
Contributor

@ettiSurreal ettiSurreal commented Oct 6, 2023

closes godotengine/godot-proposals#7965
continuation of #82699

Implements rotate_toward to Vector2, Vector3, Basis and Quaternion.
Works exactly like slerp (uses it internally), but rotates by an increment (in radians) instead of a ratio.
The Vector2 and Vector3 methods additionally have a keep_length parameter, which makes the input vector only rotate around the center point, keeping its original length.

Use cases include objects that you might prefer to rotate at a constant rate like turrets or homing missiles, or redirecting velocity vectors.

C# Support will be added in a follow-up PR assuming this one gets merged at some point.

@ettiSurreal ettiSurreal requested review from a team as code owners October 6, 2023 19:40
@ettiSurreal ettiSurreal changed the title Add rotate_toward to Vector2, Vector3, Basis and Quaternion Add rotate_toward to Vector2, Vector3, Basis and Quaternion Oct 6, 2023
@Calinou Calinou added this to the 4.x milestone Oct 6, 2023
core/math/vector3.h Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

AThousandShips commented Oct 6, 2023

Please add co-authorship as the code is largely the same as #82699 and salvages it

core/math/quaternion.h Outdated Show resolved Hide resolved
core/math/quaternion.h Outdated Show resolved Hide resolved
@ettiSurreal ettiSurreal force-pushed the rotate-2ward branch 2 times, most recently from cf28b9b to 30f2611 Compare October 6, 2023 22:43
doc/classes/Vector2.xml Outdated Show resolved Hide resolved
doc/classes/Vector3.xml Outdated Show resolved Hide resolved
core/math/vector2.h Outdated Show resolved Hide resolved
core/math/vector3.h Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Vector2.xml Outdated Show resolved Hide resolved
doc/classes/Vector3.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

Please use rebasing to update your branch instead of merge commits, it messes with the history (and you'll need to squash it at the end anyway), see here

@ettiSurreal
Copy link
Contributor Author

Please use rebasing to update your branch instead of merge commits, it messes with the history (and you'll need to squash it at the end anyway), see here

This was done as last resort as I couldn't update the branch, I still struggle with git workflows.

@fire fire requested a review from a team January 23, 2024 19:13
@fire
Copy link
Member

fire commented Feb 29, 2024

I'm trying to get some review on this as this was partially my code someone else has to review.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that rotate_toward() for Vector2 and Vector3 makes sense from a convenience point of view.

However, the rotate_toward() API in Quaternion and Basis looks quite nonsensical, not only in name (These rotate classes never have move_toward()) but also in mathematical rationality. What is the difference with slerp? The slerp in Quaternion and Basis works correctly outside the 0-1 range, so what this code is trying to do does not seem to make sense.

Is this what you are trying to implement because the Quaternion and Basis slerp does not handle outside the 0-1 range the way you think it should? Can you explain what would actually be the problem?

If there really needs to be some kind of modification, then I think you should separate the PR that fix the Quaternion::slerp(). Well, as conclusion, remove the rotate_toward() API from Quaternion and Basis and this PR would be fine. At least they don't appear to be necessary in the first place, since you didn't use either Quaternion::rotate_toward() and Basis::rotate_toward() in the Vector3::rotate_toward().

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was confused when I saw the code, but after looking at the original purpose, I think I somehow understand what is needed. In conclusion, I think the of the Quaternion::rotate_toward is understandable, but then, I assume the float rotate_toward() should be added as well considering the existence of the float move_toward().

Other than that, the entire calculation (expected operation) appears to be wrong and needs to be revised.

core/math/vector2.h Outdated Show resolved Hide resolved
core/math/vector3.h Outdated Show resolved Hide resolved
core/math/quaternion.h Outdated Show resolved Hide resolved
doc/classes/Vector2.xml Outdated Show resolved Hide resolved
core/math/vector2.h Show resolved Hide resolved
@TokageItLab
Copy link
Member

TokageItLab commented Nov 11, 2024

In summary, it should be fixed the following:

  • Add float rotate_toward()
  • delta must only be treated as radian
  • Modifing vector length in Vector2::rotate_toward() and Vector3::rotate_toward() must be optional
  • Negative delta must be rotated by longest path

@aaronfranke
Copy link
Member

@TokageItLab What does it mean to rotate a float?

@ettiSurreal
Copy link
Contributor Author

ettiSurreal commented Nov 11, 2024

  • Add float rotate_toward()

I already added that last year #80225.

What does it mean to rotate a float?

The method was originally called move_toward_angle(), since it's the lerp_angle() equivalent of move_toward(). After some discussion it was renamed to rotate_toward() because it's only useful when dealing with rotations, and you "can't move towards an angle".

@TokageItLab
Copy link
Member

TokageItLab commented Nov 11, 2024

@aaronfranke It treats from, to, and delta as radians and rotates to the target in the TAU for the radian.

I already added that last year #80225.

@ettiSurreal Yes, I think this behavior is correct partially. So if delta is negative, the rotation should rotate to select the longest path, so if rotate_toward is implemented for other classes, it should be consistent with that.

@TokageItLab
Copy link
Member

@ettiSurreal I was unable to do that review #80225 at the time. The direction of rotation is correct, but the target seems to be implemented incorrectly I sent the issue, see #99083.

@ettiSurreal
Copy link
Contributor Author

Updated relevant methods based on comments and discussion in #99083. Will update the patch hopefully tomorrow when I have time to mess with git again.

func rotate_toward_vec2(from: Vector2, to: Vector2, delta: float, keep_length: bool = false) -> Vector2:
	
		var angle: float = absf(from.angle_to(to));
		
		if keep_length:
			if angle < delta:
				return to.normalized() * from.length()
			return from.normalized().slerp(to.normalized(), delta / angle) * from.length()
		if angle < delta:
			return to
		return from.slerp(to, delta / angle)
func rotate_toward_quaternion(from: Quaternion, to: Quaternion, delta: float) -> Quaternion:
		if delta < 0.0:
			from = Quaternion(-from.x, -from.y, -from.z, -from.w)
		var angle: float = from.angle_to(to)
		if angle < delta:
			return to
		return from.slerp(to, delta / angle)

After trying to find how to find the longest path in a quaternion, apparently you can achieve that by simply negating one of the inputs? This seems to work after testing, at the point where it starts to oscillate due to switching which path is the longest, the dot product between the quaternions is around 0.

Though there is an issue with both methods, when you try to pass in a negative delta when from and to are the exact same, the method will return nan on all returned components.

@TokageItLab
Copy link
Member

TokageItLab commented Nov 11, 2024

Inverting the phase may indeed indicate a rotation of the longest path, but there may be a problem if the user arbitrarily inverts the phase and passes it as an argument.

You can align the phases by converting the Basis transformation once, as we did in cubic interpolation. So you can write it -Basis(from).get_rotation_quaternion() However, from a performance standpoint, it may be better to invert only the first check of slerp.

@MarianoGnu
Copy link
Contributor

MarianoGnu commented Nov 11, 2024

@ettiSurreal Yes, I think this behavior is correct partially. So if delta is negative, the rotation should rotate to select the longest path, so if rotate_toward is implemented for other classes, it should be consistent with that.

when delta is negative, once the angle crosses the 180º limit, now the longest path is the oposite direction, so it would lock pointing to the oposite direction forever 🤔

EDIT: of course i am considering the case where this function is executed every frame, which is the usual case for this kind of methods

@TokageItLab
Copy link
Member

TokageItLab commented Nov 11, 2024

@MarianoGnu As I mentioned in #99083 (comment), if you want that behavior, just invert the target rotation, not delta.

@ettiSurreal ettiSurreal marked this pull request as draft November 13, 2024 09:46
@ettiSurreal ettiSurreal force-pushed the rotate-2ward branch 5 times, most recently from 9b074a3 to c7d3351 Compare November 13, 2024 14:00
Co-authored-by: fire <fire@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

Add a rotate_toward() method to Vector2, Vector3, Quaternion and Basis
7 participants