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

C#: Fix Vector3 Slerp normalization error #94766

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Z0rb14n
Copy link
Contributor

@Z0rb14n Z0rb14n commented Jul 26, 2024

Closes #94049 by adding a check of the rotation axis being the zero vector, and linearly interpolating if so.
The check already exists in the C++ implementation of Slerp, so this PR ports it to C#:

godot/core/math/vector3.h

Lines 238 to 257 in e343dbb

Vector3 Vector3::slerp(const Vector3 &p_to, real_t p_weight) const {
// This method seems more complicated than it really is, since we write out
// the internals of some methods for efficiency (mainly, checking length).
real_t start_length_sq = length_squared();
real_t end_length_sq = p_to.length_squared();
if (unlikely(start_length_sq == 0.0f || end_length_sq == 0.0f)) {
// Zero length vectors have no angle, so the best we can do is either lerp or throw an error.
return lerp(p_to, p_weight);
}
Vector3 axis = cross(p_to);
real_t axis_length_sq = axis.length_squared();
if (unlikely(axis_length_sq == 0.0f)) {
// Colinear vectors have no rotation axis or angle between them, so the best we can do is lerp.
return lerp(p_to, p_weight);
}
axis /= Math::sqrt(axis_length_sq);
real_t start_length = Math::sqrt(start_length_sq);
real_t result_length = Math::lerp(start_length, Math::sqrt(end_length_sq), p_weight);
real_t angle = angle_to(p_to);
return rotated(axis, angle * p_weight) * (result_length / start_length);

Sample problematic C# code I used for testing:

public override void _Ready()
{
  Vector3 vecA = new Vector3(0.12f, 0.12f, 0.12f);
  Vector3 vecB = new Vector3(0.512f, 0.512f, 0.512f);
  Vector3 vecC = new Vector3(-0.512f, -0.512f, -0.512f);
  GD.Print(vecA.Slerp(vecB, 0.5f));
  GD.Print(vecA.Slerp(vecC, 0.5f));
  Vector3 v1 = new Vector3(1,2,3);
  Vector3 v2 = new Vector3(2,4,6);
  Vector3 v3 = new Vector3(-3,-6,-9);
  GD.Print(v1.Slerp(v2, 0.5f));
  GD.Print(v1.Slerp(v3, 0.5f));
}

Console output (previously, it would crash on any of the print statements):

(0.316, 0.316, 0.316)
(-0.19600001, -0.19600001, -0.19600001)
(1.5, 3, 4.5)
(-1, -2, -3)

@Z0rb14n Z0rb14n requested a review from a team as a code owner July 26, 2024 01:58
@AThousandShips AThousandShips changed the title Fix Vector3 Slerp Normalizing Error C#: Fix Vector3 Slerp Normalizing Error Jul 26, 2024
@AThousandShips AThousandShips added bug topic:dotnet cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 26, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 26, 2024
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the .NET module! The changes look good to me, just a minor nit-pick.

Ported the existing zero length check in C++ into C#.
@Z0rb14n Z0rb14n force-pushed the fix-vector3-slerp branch from 9debbb0 to 0d6e9de Compare July 26, 2024 16:05
@Z0rb14n Z0rb14n requested a review from raulsntos July 26, 2024 16:06
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@akien-mga akien-mga changed the title C#: Fix Vector3 Slerp Normalizing Error C#: Fix Vector3 Slerp normalization error Aug 16, 2024
@akien-mga akien-mga merged commit 4bd33df into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@aaronfranke
Copy link
Member

I know it's already merged but I just wanted to comment that I also approve of this ✅

@Z0rb14n Z0rb14n deleted the fix-vector3-slerp branch August 25, 2024 04:53
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# Vector3.cs Slerp() Method Normalize Error
5 participants