Skip to content

Conversation

Jondolf
Copy link
Owner

@Jondolf Jondolf commented Nov 28, 2024

Objective

The row and column counts in Collider::heightfield seem 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.

broken

Solution

Fix the order.

fixed

@Jondolf Jondolf added C-Bug Something isn't working A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality labels Nov 28, 2024
Copy link

@Nopey Nopey left a comment

Choose a reason for hiding this comment

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

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);
Copy link

@Nopey Nopey Jul 20, 2025

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality C-Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants