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

Complete the docs for Quaternion #84140

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

HexagonNico
Copy link
Contributor

Added documentation for some methods in the Quaternion class where it was missing.

@HexagonNico HexagonNico requested a review from a team as a code owner October 29, 2023 14:33
@AThousandShips AThousandShips added enhancement documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Oct 29, 2023
@AThousandShips AThousandShips added this to the 4.3 milestone Oct 29, 2023
@@ -79,6 +79,7 @@
<method name="exp" qualifiers="const">
<return type="Quaternion" />
<description>
Returns the [url=https://en.wikipedia.org/wiki/Quaternion#Exponential,_logarithm,_and_power_functions]exponential[/url] of this quaternion.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about linking to Wikipedia here, the link is not guaranteed to be preserved, and we don't do it for the other ones here, I'd suggest providing an explanation here instead, as the equations aren't very complicated

Copy link
Member

@AThousandShips AThousandShips Oct 29, 2023

Choose a reason for hiding this comment

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

Especially considering this is the code, which is trivial to explain:

Quaternion Quaternion::log() const {
	Quaternion src = *this;
	Vector3 src_v = src.get_axis() * src.get_angle();
	return Quaternion(src_v.x, src_v.y, src_v.z, 0);
}

Quaternion Quaternion::exp() const {
	Quaternion src = *this;
	Vector3 src_v = Vector3(src.x, src.y, src.z);
	real_t theta = src_v.length();
	src_v = src_v.normalized();
	if (theta < CMP_EPSILON || !src_v.is_normalized()) {
		return Quaternion(0, 0, 0, 1);
	}
	return Quaternion(src_v, theta);
}

And at least I can't work out what the method is doing from the Wiki but the code is very clear

@HexagonNico
Copy link
Contributor Author

I thought it could be explained with a link since I saw it is done in Vector3. I changed the docs to explain the code better.

@fire fire requested a review from a team October 30, 2023 21:41
@fire
Copy link
Member

fire commented Oct 30, 2023

The use of exponential and log of quaternion is that if you randomly pick a value it will be on the rotation sphere, which then you can use then extract into a quaternion rotation.

Any quaternion that isn't on the rotation sphere is not a rotation.

http://www.jp.square-enix.com/tech/library/pdf/Implementation_Details_20170730_SIGGRAPH%2717_David.pdf

https://web.archive.org/web/20170825184056/http://number-none.com/product/Understanding%20Slerp,%20Then%20Not%20Using%20It/

See the review of the three methods of rotation interpolation by Jonathan Blow:

image

See also https://web.mit.edu/2.998/www/QuaternionReport1.pdf for an exhaustive textbook.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

While doing so, please amend the resulting commit message to be more explicit (e.g. like the PR title).

@akien-mga akien-mga changed the title Finish the docs for Quaternion Complete the docs for Quaternion Jan 4, 2024
@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 4, 2024
@HexagonNico
Copy link
Contributor Author

Commits squashed!

@akien-mga akien-mga merged commit e59b4de into godotengine:master Jan 5, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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

Successfully merging this pull request may close these issues.

5 participants