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
Introduce DualQuaternion type #810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions that you laid out were very helpful, thanks a lot
10da458
to
2017356
Compare
3ed1452
to
61f0bc5
Compare
@sebcrozet anything blocking this from landing? Happy to make any changes that you suggest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for this PR.
This commit introduces the `DualQuaternion` type, in line with the plan laid out in [dimforge#487]. [dimforge#487]: dimforge#487
cbab6de
to
8036c56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebcrozet thanks for the helpful review - I've addressed all of your feedback.
src/geometry/dual_quaternion.rs
Outdated
#[cfg_attr(feature = "serde-serialize", derive(Serialize, Deserialize))] | ||
pub struct DualQuaternion<N: SimdRealField> { | ||
// [real(w, i, j, k), dual(w, i, j, k)] | ||
pub(crate) dq: [N; 8], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to store internally as an array (instead of storing real and dual part separately) so that we don't need an if
statement when indexing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you did before was better since it made it more explicit that it is composed of two quaternions. And these two quaternions could be public fields for easier access.
Regarding indexing, you can simply:
- Add
#[repr(C)]
toDualQuaternion
. - Add an implementation of
AsRef
andAsMut
. Something like:
impl<N: SimdRealField> AsRef<[N; 8]> for DualQuaternion<N> {
#[inline]
fn as_ref(&self) -> &[N; 8] {
unsafe { mem::transmute(self) }
}
}
// Do something similar for AsMut.
and use these in the implementation of Index
and IndexMut
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I like your suggestion better, thanks will change.
Question though - why are quaternions stored internally at [i,j,k,w]? I would've expected [w,i,j,k] given that Quaternion::new(w,i,j,k)
.
No big deal, just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question though - why are quaternions stored internally at [i,j,k,w]? I would've expected [w,i,j,k] given that Quaternion::new(w,i,j,k).
By similarity with the Eigen library, mostly.
Thank you for addressing the feedbacks. Though keep in mind that it is generally better to push new commits instead of overwriting the previous one(s). That way it is easier to review your changes because I can see easily what changed since my last review. |
Ah, I thought that the PR was small enough that it was fine. Okay great, noted for next time. Thank you |
@sebcrozet thanks again for the help, all feedback addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! There are a few things that can be removed here:
@sebcrozet feedback addressed! |
Great, thanks! |
This commit introduces the
DualQuaternion
type, in line with the planlaid out in #487.