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, Quaternion and Basis. #7965

Open
ettiSurreal opened this issue Oct 2, 2023 · 16 comments · May be fixed by godotengine/godot#82926
Open

Add rotate_toward to Vector2, Vector3, Quaternion and Basis. #7965

ettiSurreal opened this issue Oct 2, 2023 · 16 comments · May be fixed by godotengine/godot#82926

Comments

@ettiSurreal
Copy link

ettiSurreal commented Oct 2, 2023

Describe the project you are working on

Small platformer game.

Describe the problem or limitation you are having in your project

There is currently no built-in way to rotate Vectors, Quaternions and Basis by an increment.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Like the newly added GlobalScope rotate_toward is the equivalent of lerp_angle, this would be the equivalent of slerp.
The Vector implementation should also let you move toward the lengths/magnitudes of a vector, which the proposed implementation has implemented.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

With some help in the contributors chat we came to this implementation, just need to get someone to translate this to C++, because I cannot do that myself.

Quaternion

func quaternion_rotate_toward(from: Quaternion, to: Quaternion, delta: float) -> Quaternion:
	if is_zero_approx(delta):
		return from
	if delta < 0.0: # Invert to if delta is negative to prevent oscillation
		delta = -delta
		to = to.inverse()
	var angle: float = from.angle_to(to)
	if angle < delta:
		return to # Avoid division by 0
	return from.slerp(to, delta / angle)

Basis

func basis_rotate_toward(from: Basis, to: Basis, delta: float) -> Basis:
	return Basis(quaternion_rotate_toward(from.get_rotation_quaternion(), to.get_rotation_quaternion(), delta))

Vector2 and Vector3 (Same code should work for both, just need to swap the types)

func vector_rotate_toward(from: Vector2, to: Vector2, rotation_delta: float, length_delta: float = 0.0) -> Vector2:
	
	var from_length: float = from.length()
	from = from.normalized()
	var to_length: float = to.length()
	var length: float = move_toward(from_length, to_length, length_delta)
	if length < 0.0:
		length = 0.0 # prevent oscillation if moving inwards
	
	if is_zero_approx(rotation_delta):
		return from * length # Should still take length_delta into account if rotation_delta is 0.0
	
	to = to.normalized()
	
	if rotation_delta < 0.0: # Invert to if delta is negative to prevent oscillation
		rotation_delta = -rotation_delta
		to = -to
	var angle: float = from.angle_to(to)
	if angle < rotation_delta:
		return to * length # Avoid division by 0
	return from.slerp(to, rotation_delta / angle) * length

If this enhancement will not be used often, can it be worked around with a few lines of script?

The functions can be recreated (or copied from here) as static functions. But some implementations of it I saw can cause a division by zero near the end.

Is there a reason why this should be core and not an add-on in the asset library?

To me the main use case of this would be looking at a target while rotating at a constant value, which would be useful for something like a turret or homing missile off the top of my head. I think these would share the use cases with slerp, with this being mostly preferential.

@fire
Copy link
Member

fire commented Oct 3, 2023

I am not able to get it past the finishing but here's a quick port godotengine/godot#82699

@fire
Copy link
Member

fire commented Oct 3, 2023

Please suggest better use cases for why this is not a few lines of script or is very common.

@mathphye
Copy link

mathphye commented Oct 3, 2023

As suggested by @kitbdev in chat.godotengine.org:

Using vec = vec.slerp(target, delta) could be better, as dividing by angle could lead to infinite for an angle close to zero... This is because by dividing the angle it gets 1 radian in percentage... which means for 0.5 radian angle would try a 200% step. Making it unstable for radians less than 1.

okno, thinking more. I am wrong.

@mathphye
Copy link

mathphye commented Oct 3, 2023

In theory angle < rotation_delta guarantees (rotation_delta / angle)<1...

and angle < rotation_delta with is_zero_approx(rotation_delta) guarantees both being greater than epsilon.

However dividing by 10eps or even 100eps, could cause an outbound of floating points, I won't be sure if it is sufficient to avoid infinite results when both angle and rotation_delta are really tinny. Fortunately, we won't expect a user writing something like 100*epsilon

@nlupugla
Copy link

nlupugla commented Oct 3, 2023

I would recommend making use of the following formula:

Let $\vec{v}_1$ be the initial vector and $\vec{v}_2$ the vector we want to rotate towards. Let $\vec{v}$ be a vector whose magnitude is equal to $\vec{v}_1$ and is rotated $\theta$ radians towards $\vec{v}_2$. A formula for $\vec{v}$ is

$$\vec{v} = \cos(\theta) \vec{v}_1 + \sin(\theta) \frac{|\vec{v}_1|^2 \ \vec{v}_2 - (\vec{v}_1 \cdot \vec{v}_2) \vec{v}_1}{\sqrt{|\vec{v}_1|^2 |\vec{v}_2|^2 - (\vec{v}_1 \cdot \vec{v}_2)^2}}$$

I don't have a source for this formula, but it's easy enough to prove it, you just need to show that

$$\vec{v}_3 = \frac{|\vec{v}_1|^2 \ \vec{v}_2 - (\vec{v}_1 \cdot \vec{v}_2) \vec{v}_1}{\sqrt{|\vec{v}_1|^2 |\vec{v}_2|^2 - (\vec{v}_1 \cdot \vec{v}_2)^2}}$$

is perpendicular to $\vec{v}_1$ and has the same magnitude.

There are probably cancelations you can apply to make the expression for $\vec{v}_3$ more numerically stable, but the idea is there.

@fire
Copy link
Member

fire commented Oct 3, 2023

Can you check this formula translated by machine?

# Assuming v1 and v2 are Vector2 or Vector3 objects in Godot, and theta is the angle in radians
func rotate_vector(v1, v2, theta):
    var v1_dot_v2 = v1.dot(v2)
    var v1_mag_sq = v1.length_squared()
    var v2_mag_sq = v2.length_squared()

    var v3 = (v1_mag_sq * v2 - v1_dot_v2 * v1) / sqrt(v1_mag_sq * v2_mag_sq - v1_dot_v2 * v1_dot_v2)

    var v = cos(theta) * v1 + sin(theta) * v3

    return v

@nlupugla
Copy link

nlupugla commented Oct 3, 2023

It looks right enough to me. I'd recommend writing out a few unit tests. Not necessarily you fire, but perhaps @ettiSurreal would be up to the task.

By the way, if you value numerical stability more than having an intuitive interpretation of the parameter that controls the magnitude of the rotation, you can also use the following formula.

$$\vec{v}_3 = e^{t(\vec{v}_2 \vec{v}_1^T - \vec{v}_1 \vec{v}_2^T)} \ \vec{v}_1$$.

In this version of the formula, $t$ is the rotation parameter rather than $\theta$. The two are related by

$\theta = |\vec{v}_1| |\vec{v}_2| \sin(\phi) t$

where $\phi$ is the angle in radians between $\vec{v}_1$ and $\vec{v}_2$.

As you can see, $t$ doesn't have units of radians. However, you don't need to do any special case handling for when $\vec{v}_1$ and $\vec{v}_2$ are nearly parallel, which you do need to do in my first formula to avoid dividing by zero.

I don't really have any preference for which one gets implemented. It depends on whether this function will more likely be used in a context where you actually want to rotate by a specific number of radians or you just want to rotate some amount in the right direction.

@fire
Copy link
Member

fire commented Oct 3, 2023

So like turn rotations?

@mathphye
Copy link

mathphye commented Oct 3, 2023

This is the formula for slerp(), isn't? However, I don't recommend using such a black-box formula (which is not intuitive) as it would be hard to scale or maintain. Maybe the previous code is "larger" but scalable through time thanks to the easy compressibility open to new contributors

I would recommend making use of the following formula:

Let v→1 be the initial vector and v→2 the vector we want to rotate towards. Let v→ be a vector whose magnitude is equal to v→1 and is rotated θ radians towards v→2. A formula for v→ is

v→=cos⁡(θ)v→1+sin⁡(θ)|v→1|2 v→2−(v→1⋅v→2)v→1|v→1|2|v→2|2−(v→1⋅v→2)2

I don't have a source for this formula, but it's easy enough to prove it, you just need to show that

v→3=|v→1|2 v→2−(v→1⋅v→2)v→1|v→1|2|v→2|2−(v→1⋅v→2)2

is perpendicular to v→1 and has the same magnitude.

There are probably cancelations you can apply to make the expression for v→3 more numerically stable, but the idea is there.

@nlupugla
Copy link

nlupugla commented Oct 3, 2023

So like turn rotations?

Not really sure what you mean by that.

@nlupugla
Copy link

nlupugla commented Oct 3, 2023

This is the formula for slerp(), isn't? However, I don't recommend using such a black-box formula (which is not intuitive) as it would be hard to scale or maintain. Maybe the previous code is "larger" but scalable through time thanks to the easy compressibility open to new contributors.

It's not quite the formula for slerp. The main difference is that in slerp, you specify the rotation magnitude as a fraction of a complete rotation from $\vec{v}_1$ to $\vec{v}_2$ whereas in the first formula I give, you specify the rotation magnitude in radians. However, the second formula I gave is basically slerp I realize now, having thought about it a bit more.

The formula I gave above is a bit black-boxy, so maybe I can explain a bit better where it's coming from.

The idea is to express the rotated vector as a combination of the initial vector $\vec{v}_1$ and a vector $\vec{v}_3$ which is perpendicular to it and has the same magnitude:

$$ \vec{v} = \cos(\theta) \vec{v}_1 + \sin(\theta) \vec{v}_3 $$

Really, the only tricky part is in how $\vec{v}_3$ is determined from $\vec{v}_1$ and $vec{v}_2$. This can be thought of as its own subroutine, which might be called something like get_perp_towards. The idea is to subtract off of $\vec{v}_2$ the component that is parallel to $\vec{v}_1$, then rescale the remaining vector to have a magnitude equal to $\vec{v}_1$.

That said, I have no strong opinions about whether this formula goes in the engine or not. I need it for any project that I'm working on, so I don't really know how much more convenient a function like this compared to just slerp would be in practice.

@ettiSurreal
Copy link
Author

It looks right enough to me. I'd recommend writing out a few unit tests. Not necessarily you fire, but perhaps @ettiSurreal would be up to the task.

Quaternion and Basis slerp don't have tests, and it seems like it would be very difficult to write tests for those for me. I can attempt on writing tests for Vector2 and 3 though.

@ettiSurreal
Copy link
Author

Quick idea from the quick port PR:
Separating rotation and length in the vector method doesn't seem very practical, so it will most likely be taken out. A small suggestion is replacing it with a normalized rotation bool, that when true normalizes the two vectors, then multiplies the result by the length of the first vector, letting you rotate toward two vectors with different magnitudes while keeping the length of the first one.

@JoNax97
Copy link

JoNax97 commented Oct 3, 2023

A small suggestion is replacing it with a normalized rotation bool,

I agree with the idea but the name can be a little misleading. Perhaps something like preserve_length would be clearer

@ettiSurreal
Copy link
Author

I agree with the idea but the name can be a little misleading. Perhaps something like preserve_length would be clearer

Yes i forgot to specify I was looking for a better name.

@mathphye
Copy link

mathphye commented Oct 4, 2023

define SLERP(*args, normalized=true) and implement this rotate_toward() as SLERP with parameter normalized=false.

As one approach is changes by percentages and the other by deltas... may be better to merge both names as similar, like:

LERP -> percentual change
LERD -> delta change (overrated from move_toward)

SLERP -> percentual change
SLERD -> delta change (overrated from rotate_toward)

Or using a single one with a flag, percentual=true or with_delta=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants