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

Overhaul Quaternion documentation #87181

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 14, 2024

Closes godotengine/godot-docs#6271

This PR aims to completely overhaul the Quaternion type.
The reasoning and changelog are extremely similar to the "Overhaul Basis Documentation" PR, so I'm not sure I should bother repeating them twice.

More specific notes:


I started this ASAP after seeing the "overwhelmingly" positive reception of the prior PR after a few mere hours. Feedback is very, very welcome. Quaternions are a very complicated concept to digest and comprehend.

This PR may contain multiple versions of the same line I was not sure about, denoted with "??". Reviewing of these lines is heavily appreciated.

Sources:

@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from 4de70fa to e3f49d8 Compare January 14, 2024 18:34
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 14, 2024
@Zireael07
Copy link
Contributor

A great resource I just found https://github.com/iwatake2222/rotation_master

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 16, 2024

Definitely looks like a great website. However, as far as introductions go, it's extremely unfriendly. There's a lot data present all at once. The web version could be added as another link in the tutorial section, perhaps.

@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from e3f49d8 to 63d7c76 Compare January 18, 2024 12:57
@Mickeon Mickeon marked this pull request as ready for review January 18, 2024 12:59
@Mickeon Mickeon requested a review from a team as a code owner January 18, 2024 12:59
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from 63d7c76 to c52740f Compare January 18, 2024 13:04
@Zireael07
Copy link
Contributor

The web version could be added as another link in the tutorial section, perhaps.

Please do!

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 18, 2024

It's done in the current version of this PR

@0awful
Copy link

0awful commented Jan 20, 2024

Man I wish this was a thing a week ago when I took on handling quaternion code in rust's gdextension. That being said. I've got a few things I discovered that aren't covered here. #87422. I propose that I merge after yours to incorporate the nitty gritty of what happens when you break the rules.

doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
@fire
Copy link
Member

fire commented Jan 22, 2024

I wanted to mention that the JPL and HAMILTON conversions differ by x*-1,y*-1,z*-1,w. Assuming that w is the real. Sometimes x is the real component. Like one.

I am certain this is incomprehensible :(

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 22, 2024

I have understood it in part from the sources I looked up. I am still struggling to visualize the difference in practice, however, as well as providing a simpler explanation to fit in Quaternion's description. Hence I cannot tell if the provided websites use the same convention as Godot or not.

@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from c52740f to c0e4dde Compare January 22, 2024 09:24
@Mickeon Mickeon requested a review from aaronfranke February 19, 2024 14:43
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from c0e4dde to d945196 Compare February 19, 2024 17:02
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 19, 2024

I have once and for all clarified it's the Hamilton convention. Now, for more to review if there is

@Mickeon Mickeon requested a review from aaronfranke February 19, 2024 17:03
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I urge you to please test things and review your work for factual accuracy. Nicer wording is useless if the words are not correct.

doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch 2 times, most recently from f5b4853 to cfaa24c Compare February 22, 2024 17:44
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 22, 2024

I took all of the feedback at heart and addressed it. Hopefully I have not forgotten anything.
While I am not fully satisfied with the leading description it does feel good enough.

@Mickeon Mickeon requested a review from aaronfranke February 22, 2024 17:45
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from cfaa24c to 15cd23f Compare February 22, 2024 18:02
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from 15cd23f to cdfb05a Compare February 24, 2024 00:51
@Mickeon Mickeon requested a review from aaronfranke February 24, 2024 15:59
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
doc/classes/Quaternion.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-quaternion-i-keep-misspelling-it branch from cdfb05a to 38cd13c Compare February 25, 2024 00:09
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit e69cdf3 into godotengine:master Feb 25, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the doc-peeves-quaternion-i-keep-misspelling-it branch February 25, 2024 12:35
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

Godot uses JPL style quaternions instead of Hamilton
7 participants