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

Fix Quaternion arc constructor to check dot & Add test for same Vector3s #101797

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

TokageItLab
Copy link
Member

We removed the dot check in the review for the performance, but did not consider the case of the same vector. So fix to add the dot check. Also, add a test for the same vectors.

@TokageItLab TokageItLab requested review from a team as code owners January 19, 2025 11:49
@TokageItLab TokageItLab force-pushed the fix-quat-arc branch 3 times, most recently from 2ee22e1 to 650d019 Compare January 19, 2025 11:52
@TokageItLab TokageItLab added this to the 4.4 milestone Jan 19, 2025
@TokageItLab TokageItLab changed the title Fix Quaternion arc constructor to check dot & Add test for same vec3s Fix Quaternion arc constructor to check dot & Add test for same Vector3s Jan 19, 2025
@TokageItLab TokageItLab force-pushed the fix-quat-arc branch 4 times, most recently from 190f944 to a6a3b8f Compare January 19, 2025 12:37
@TokageItLab TokageItLab requested a review from lawnjelly January 19, 2025 13:17
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks fine for fixing the bug, although would be good to have someone else check with another set of eyes.

There's a few things that could be checked out if going to town for performance, but probably not necessary.

For instance you could evaluate whether checking both conditions with:

if (ABS(d) > ALMOST_ONE)

Made a difference. You could then do a second check for polarity.
The idea being ABS cheaper than a comparison. But that would require looking at assembly / profiles and I don't think it's necessary unless someone is super keen.

By the way, can confirm compiler uses two separate constants for the positive and negative, I checked the assembly.

@fire
Copy link
Member

fire commented Jan 19, 2025

I liked the unit test added for the error case.

@TokageItLab
Copy link
Member Author

Adopted abs with comments on the second comparison.

@lawnjelly
Copy link
Member

lawnjelly commented Jan 20, 2025

It was fine before, but the pattern I was suggesting is more:

real_t d = n0.dot(n1);

if (Math::abs(d) > ALMOST_ONE) {
	if (d >= 0) {
		return;
	}
	
	Vector3 axis = n0.get_any_perpendicular();
	x = axis.x;
	y = axis.y;
	z = axis.z;
	w = 0;
}
else
{
	Vector3 c = n0.cross(n1);
	real_t s = Math::sqrt((1.0f + d) * 2.0f);
	real_t rs = 1.0f / s;

	x = c.x * rs;
	y = c.y * rs;
	z = c.z * rs;
	w = s * 0.5f;
}

This is thus using ABS and a single branch for most cases, instead of two branches.

But this isn't really hugely important unless you've profiled the two in bottlenecks (the original could be faster for instance), and the math checks will likely dwarf this. Going to this level to make sensible decisions you would need to be looking at assembly and benchmarks, and I don't think this is warranted at this stage, so either the previous implementation you did (where we approved) or the above would be fine, there will likely be a hair's breadth between them.

The optimizer may well do this from your current PR anyway, but in the current commit it is less clear what you are asking the compiler to do.

@Repiteo Repiteo merged commit ba73831 into godotengine:master Jan 20, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 20, 2025

Thanks!

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

Successfully merging this pull request may close these issues.

[4.4beta1] Quaternion arc constructor gives wrong rotation when vectors are the same
4 participants