Fix non-square Collider::heightfield row and column count order#568
Fix non-square Collider::heightfield row and column count order#568
Collider::heightfield row and column count order#568Conversation
There was a problem hiding this comment.
DMatrix::from_vec expects column-major order.
Collider::heightfield does not currently document whether heights is a vector of column vectors (equivalently, that it is column major) or (if a transpose is added in heightfield) that it is a vector of row vectors (equiv. row major), could this PR also address that?
If you'd rather spend your time on something other than rewriting this PR, I could make one of the two changes described below (either documenting that heightfield is column-major and fixing the var names, or breaking-change changing it to be row-major)
Thanks, and I hope this review is welcome.
| ); | ||
|
|
||
| let heights = nalgebra::DMatrix::from_vec(row_count, column_count, data); | ||
| let heights = nalgebra::DMatrix::from_vec(column_count, row_count, data); |
There was a problem hiding this comment.
Please do not pass column_count to the nrows argument.
DMatrix::from_vec's first argument is nrows, and second is ncol; the bug is in the calculation of column and row count, and the general confusion of order.
Instead of switching the order of the arguments, the two variables' assignments should be switched.
EDIT: the above assertion "Each row in heights must have the same amount of points" also insists that heights/data is row-major, which doesn't match what's being done with nalgebra below. Adding a transpose would preserve the original intention of ::heightfield, but may be considered a breaking change.
EDIT 2: DMatrix::from_row_iterator is the only row-major DMatrix constructor I see that wouldn't necessitate an additional allocation.
|
Marking as Waiting-on-Author due to CI failure and review to address |
Objective
The row and column counts in
Collider::heightfieldseem to be the wrong way around! This makes heightfields with a different number of rows and columns behave incorrectly in comparison to the documented representation.The heightfield below should slope up smoothly left to right.
Solution
Fix the order.