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.

Sign up for GitHub

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

Jump to bottom

Quaternion Documentation: Describe edge cases #87422

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+8 −3
Open

Quaternion Documentation: Describe edge cases #87422

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
354b270
Update Quaternion.xml
0awful Jan 20, 2024
3e09194
Update Quaternion.xml
0awful Jan 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions doc/classes/Quaternion.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
<param index="0" name="axis" type="Vector3" />
<param index="1" name="angle" type="float" />
<description>
Constructs a quaternion that will rotate around the given axis by the specified angle. The axis must be a normalized vector.
Constructs a quaternion that will rotate around the given axis by the specified angle.
[b]Note:[/b] The axis must be a normalized vector. Returns [code](0, 0, 0, 1)[/code] if your vector is not normalized in Debug mode. Returns the quaternion that would have been generated by normalizing the axis in release mode. If the axis has length [code]0[/code] and therefore cannot be normalized returns [code](0, 0, 0, 0)[/code]
Copy link
Contributor

Choose a reason for hiding this comment

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

I do trust that this is all correct, as it is way out of my expertise. I heavily appreciate the extra detail.

However, the way this entire note and others are structured in this PR is extremely inconsistent and kind of "not good" for several reasons:

  • (0, 0, 0, 1) is the IDENTITY Quaternion and should be referred to as such (Returns [constant IDENTITY]) to point users in the right direction, as it is not an arbitrary, magic value.
  • "Debug" in "debug mode" is never capitalized across the entire class reference. Saying "debug builds" is also more common in this context.
  • The whole note is extremely verbose. "Debug build" and "release build" are mutually exclusive, so there's almost no need to define both.
    • It also feels like the behavior in debug builds is the exception and not the norm, as the following sentence implies nothing wrong really happens when the axis is not normalized:
      "Returns the quaternion that would have been generated by normalizing the axis in release mode."
      This means that it's only worth noting the more "unusual" behavior in debug builds.
    • There's also little point in saying "and therefore cannot be normalized" because users interacting with a Quaternion should already have a decent understanding of vector math.

Copy link
Author

Choose a reason for hiding this comment

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

I appreciate the feedback. I'll make those modifications. Great work on your documentation improvements by the way.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[b]Note:[/b] The axis must be a normalized vector. Returns [code](0, 0, 0, 1)[/code] if your vector is not normalized in Debug mode. Returns the quaternion that would have been generated by normalizing the axis in release mode. If the axis has length [code]0[/code] and therefore cannot be normalized returns [code](0, 0, 0, 0)[/code]
[b]Note:[/b] The axis must be a normalized vector. Returns [code](0, 0, 0, 1)[/code] if your vector is not normalized in Debug mode. Returns the quaternion that would have been generated by normalizing the axis in release mode. If the axis has length [code]0[/code] and therefore cannot be normalized returns [code](0, 0, 0, 0)[/code].

</description>
</constructor>
<constructor name="Quaternion">
Expand Down Expand Up @@ -93,7 +94,7 @@
<return type="float" />
<description>
Returns the angle of the rotation represented by this quaternion.
[b]Note:[/b] The quaternion must be normalized.
[b]Note:[/b] The quaternion must be normalized. Returns [code]0[/code] if the quaternion has a length and is not normalized. Returns [code]PI[/code] if the quaternion has no length and is not normalized.
</description>
</method>
<method name="get_axis" qualifiers="const">
Expand Down Expand Up @@ -156,6 +157,7 @@
<return type="Quaternion" />
<description>
Returns a copy of the quaternion, normalized to unit length.
[b]Note:[/b] Must have a length that does not equal [code]0[/code]. Returns [code](nan, nan, nan, nan)[/code] if you normalize a quaternion with no length.
</description>
</method>
<method name="slerp" qualifiers="const">
Expand All @@ -164,7 +166,7 @@
<param index="1" name="weight" type="float" />
<description>
Returns the result of the spherical linear interpolation between this quaternion and [param to] by amount [param weight].
[b]Note:[/b] Both quaternions must be normalized.
[b]Note:[/b] Both quaternions must be normalized. Returns [code](0, 0, 0, 1)[/code] if any quaternion is not normalized in debug mode. Returns [code]self[/code] if any quaternion is not normalized in release mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

self is a GDScript-exclusive context and it only applies to Object, because all other Variant types cannot be extended.

In this situation the class reference likes to say, for example "Returns this quaternion unchanged" or "Returns the quaternion without changes" or similar.

</description>
</method>
<method name="slerpni" qualifiers="const">
Expand All @@ -173,6 +175,7 @@
<param index="1" name="weight" type="float" />
<description>
Returns the result of the spherical linear interpolation between this quaternion and [param to] by amount [param weight], but without checking if the rotation path is not bigger than 90 degrees.
[b]Note:[/b] Both quaternions must be normalized. Returns [code](0, 0, 0, 1)[/code] if any quaternion is not normalized in debug mode. Returns [code]self[/code] if any quaternion is not normalized in release mode.
</description>
</method>
<method name="spherical_cubic_interpolate" qualifiers="const">
Expand All @@ -183,6 +186,7 @@
<param index="3" name="weight" type="float" />
<description>
Performs a spherical cubic interpolation between quaternions [param pre_a], this vector, [param b], and [param post_b], by the given amount [param weight].
[b]Note:[/b] All quaternions must be normalized. Returns [code](0, 0, 0, 1)[/code] if any quaternion is not normalized in debug mode. Returns [code]self[/code] if any quaternion is not normalized in release mode.
</description>
</method>
<method name="spherical_cubic_interpolate_in_time" qualifiers="const">
Expand All @@ -197,6 +201,7 @@
<description>
Performs a spherical cubic interpolation between quaternions [param pre_a], this vector, [param b], and [param post_b], by the given amount [param weight].
It can perform smoother interpolation than [method spherical_cubic_interpolate] by the time values.
[b]Note:[/b] All quaternions must be normalized. Returns [code](0, 0, 0, 1)[/code] if any quaternion is not normalized in debug mode. Returns [code]self[/code] if any quaternion is not normalized in release mode.
</description>
</method>
</methods>
Expand Down
Loading