Introduce DualQuaternion type #810
Conversation
chinedufn
force-pushed
the
dual-quaternion
branch
from
10da458
to
2017356
Compare
chinedufn
force-pushed
the
dual-quaternion
branch
3 times, most recently
from
3ed1452
to
61f0bc5
Compare
|
@sebcrozet anything blocking this from landing? Happy to make any changes that you suggest. |
This commit introduces the `DualQuaternion` type, in line with the plan laid out in [dimforge#487]. [dimforge#487]: dimforge#487
chinedufn
force-pushed
the
dual-quaternion
branch
from
cbab6de
to
8036c56
Compare
There was a problem hiding this comment.
@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.
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.
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
AsRefandAsMut. 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.
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.
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. |
|
@sebcrozet feedback addressed! |
|
Great, thanks! |
This commit introduces the
DualQuaterniontype, in line with the planlaid out in #487.