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

Refine Hashable and Equatable on Quaternion #124

Merged
merged 13 commits into from
Jul 14, 2020

Conversation

markuswntr
Copy link
Contributor

@markuswntr markuswntr commented Jun 14, 2020

  • Canonical Representations
    Add canonicalized and canonicalizedTransform properties

  • Equatable
    Update type and header documentation and mention the alternative for transformations in

  • Hashable
    This PR changes the hashing behavior of Quaternion, so that any finite quaternion q clears the sign bit of its real component before hashing.

  • Transform Equality
    Added a new transformEquals() method on Quaternion to test for 3D transformation equality.

return self.multiplied(by: 1)
}

/// A "canonical transformation" representation of the value.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to make this more precise. If I wasn't concerned about jargon, I would say "the canonical representative of the equivalence class of quaternions representing the same 3d transformation."

Copy link
Contributor Author

@markuswntr markuswntr Jun 25, 2020

Choose a reason for hiding this comment

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

"The canonical representative of quaternions with non-negative real components representing the same 3D transformation."
Does that make sense? :)

I guess that's another task to think about over the weekend.

/// - Parameter other: The value to compare.
/// - Returns: True if the transformation of this quaternion equals `other`.
@_transparent
public func transformEquals(_ other: Quaternion) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

How do we feel about how this reads at a use site?

if q.transformEquals(p) { /* ... */ }

I don't love that "transform" may read as a verb if you don't know what you're looking at. What if we used something like this instead:

if q.equals(as3DTransform: p) { /* ... */ }

I'm not totally set on this either way, but let's give it some thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we feel about how this reads at a use site?

if q.transformEquals(p) { /* ... */ }

Yeah, you are right. This can easily be misread. We could maybe use transformationEquals to make it more explicit.

if q.equals(as3DTransform: p) { /* ... */ }

I think, I do prefer this spelling over transformEquals/transformationEquals. If we want to (somewhat) reuse existing naming patterns, we could utilize append(contentsOf:) and insert(contentsOf:at:) on Array and use:

if q.equals(transformationOf: p) { /* ... */ }

Though, I think it is a little less obvious and does not mention 3D explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Let's think about this over the weekend and see if we can make a provisional decision on Monday.

Copy link
Member

Choose a reason for hiding this comment

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

Any further thoughts on this? If not, let's go with equals(as3DTransform:) for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps q.isSame3DTransform(as: p) or some such?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to equals(as3DTransform:) for now. I think, it reads a little nicer at the call site.

@stephentyrone
Copy link
Member

@markuswntr is this ready to merge from your perspective, or do you want to iterate on it more first?

@markuswntr
Copy link
Contributor Author

Hi @stephentyrone, as you mentioned previously, the canonicalizedTransform property needs a more precise header comment. Other than that I think it is ready to merge.
If it is okay with you, I will provide an updated documentation later - than you can merge it as is.

@stephentyrone stephentyrone merged commit 843cf27 into apple:Quaternions Jul 14, 2020
@markuswntr markuswntr deleted the quaternion/hashable branch July 16, 2020 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants