Add support for QQuaternion #1314
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1314 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 75 75
Lines 12772 12784 +12
=========================================
+ Hits 12772 12784 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thank you @jnbooth ! Really appreciate it :-) |
There was a problem hiding this comment.
Hi @jnbooth, thank you for for the contribution. Again, please excuse the delayed review.
The QQuaternion looks good. QGenericMatrix still has some room for improvement.
| /// The QGenericMatrix class represents a quaternion consisting of a vector and scalar. | ||
| /// | ||
| /// Qt Documentation: [QGenericMatrix](https://doc.qt.io/qt/qgenericmatrix.html#details) | ||
| #[repr(transparent)] |
There was a problem hiding this comment.
| #[repr(transparent)] | |
| #[repr(C)] |
I think this needs to be repr(C), as otherwise it's not necessarily guaranteed that the layout matches the layout in C++, even with repr(transparent).
There was a problem hiding this comment.
The idea is for its layout to be that of [[f32; M]; N], rather than a struct with one [[f32; M]; N] member.
|
|
||
| impl<const N: usize, const M: usize> QGenericMatrix<N, M> { | ||
| /// Constructs a matrix from floating-point `values` in row-major order. | ||
| pub const fn new(values: &[[f32; N]; M]) -> Self { |
There was a problem hiding this comment.
Any reason this takes the data by-reference and not by-value?
Taking it by-value would reduce the amount of copies needed.
| let mut data = [[0.0; M]; N]; | ||
| let mut i = 0; | ||
| let size = if M < N { M } else { N }; | ||
| while i < size { |
There was a problem hiding this comment.
Consider replacing the while with:
for i in 0..size {
}There was a problem hiding this comment.
Currently unsupported in const fn. We could un-const this function, but it seems useful to be able to declare a matrix as a constant.
| pub const fn transposed(&self) -> QGenericMatrix<M, N> { | ||
| let mut transposed = [[0.0; N]; M]; | ||
| let mut col = 0; | ||
| while col < N { |
There was a problem hiding this comment.
Please use for col in 0..N and for row in 0..M instead of while loops.
There was a problem hiding this comment.
Likewise unsupported by const fn. I generally default to making functions const if they can be, because people might find it useful, but I can get rid of it if you prefer.
| pub const fn data(&self) -> &[f32] { | ||
| // TODO: Replace with `array::as_flattened` once MSRV is 1.80.0. | ||
| unsafe { slice::from_raw_parts(self.data.as_ptr().cast(), N * M) } | ||
| } |
There was a problem hiding this comment.
| pub const fn data(&self) -> &[f32] { | |
| // TODO: Replace with `array::as_flattened` once MSRV is 1.80.0. | |
| unsafe { slice::from_raw_parts(self.data.as_ptr().cast(), N * M) } | |
| } | |
| pub const fn data(&self) -> &[f32; { N * M }] { | |
| unsafe { *self.data.as_ptr().cast() } | |
| } |
| pub fn data_mut(&mut self) -> &mut [f32] { | ||
| // TODO: Replace with `array::as_flattened_mut` once MSRV is 1.80.0. | ||
| unsafe { slice::from_raw_parts_mut(self.data.as_mut_ptr().cast(), N * M) } | ||
| } |
There was a problem hiding this comment.
| pub fn data_mut(&mut self) -> &mut [f32] { | |
| // TODO: Replace with `array::as_flattened_mut` once MSRV is 1.80.0. | |
| unsafe { slice::from_raw_parts_mut(self.data.as_mut_ptr().cast(), N * M) } | |
| } | |
| pub fn data_mut(&mut self) -> &mut [f32; { N * M }] { | |
| unsafe { *self.data.as_mut_ptr().cast() } | |
| } |
There was a problem hiding this comment.
Turns out this does not work because "cannot move out of a raw pointer"
Co-authored-by: Leon Matthes <leon.matthes@kdab.com>
…entity Co-authored-by: Leon Matthes <leon.matthes@kdab.com>
…aw_parts" This reverts commit 5a53669.
Closes #1308.