Quaternion Documentation: Describe edge cases #87422
Open
+8
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
See also: |
Co-authored-by: A Thousand Ships <[email protected]>
|
Having become aware of #87181. I'm not seeing any specific discrepancies between the two. However one is a significantly larger changeset. I believe this PR, being the smaller one, should bear the responsibility of handling merge conflicts. |
Mickeon
reviewed
Jan 20, 2024
| @@ -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] | |||
There was a problem hiding this comment.
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.
- 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:
There was a problem hiding this comment.
I appreciate the feedback. I'll make those modifications. Great work on your documentation improvements by the way.
Mickeon
reviewed
Jan 20, 2024
| @@ -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. | |||
There was a problem hiding this comment.
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.
|
#87181 has been merged a while ago. Is this PR still necessary? Could it be rebased? |
mhilbrunner
reviewed
Apr 7, 2024
| @@ -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] | |||
There was a problem hiding this comment.
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]. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Working on the Rust GDExt library I became aware of how the edge cases for quaternions are handled. I believe exposing this may help future library authors/maintainers work with the engine.