Skip to content
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

Fix behaviour of BoxClipper3D #5769

Merged
merged 1 commit into from
Aug 3, 2023
Merged

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Jul 22, 2023

The constructor (as well as the setTransformation function) that takes a rotation, translation, and scaling as input, says in the documentation that theses transformations are applied to the box. But actually they are applied to the points. This commit changes the behaviour of the code to match the documentation. I considered changing the documentation to match the behaviour of the code, but

  • in my opinion it is useful to have a constructor/function to describe the box (rotation, box center, box dimensions)
  • the current code (which applies first scaling, then rotation, then translation to the points) is not very useful or intuitive. Imagine for example a box, one side twice as long as the others, and rotated by 45 degrees around some axis. Then it is impossible to find a scaling transformation and rotation to transform all points inside the box to the interval [-1; +1]³, considering the scaling is applied first and in each axis separately. It would always result in a parallelepiped.
  • if one specifically wants to rotate/translate/whatever the points, it is easy to do so by using the constructor/function that takes an Affine3f and construct for example Eigen::Affine3f transform = Eigen::Translation3f(...) * Eigen::AngleAxisf(..., Eigen::Vector3f(...).normalized());

This also fixes incorrect references to a unit cube, which is a cube with edge length 1, or sometimes specifically a cube in the interval [0; 1]. However, BoxClipper3D uses a cube in the interval [-1; +1] (edge length 2).

Fixes #3914

The constructor (as well as the setTransformation function) that takes a rotation, translation, and scaling as input, says in the documentation that theses transformations are applied to the box. But actually they are applied to the points. This commit changes the behaviour of the code to match the documentation. I considered changing the documentation to match the behaviour of the code, but
- in my opinion it is useful to have a constructor/function to describe the box (rotation, box center, box dimensions)
- the current code (which applies first scaling, then rotation, then translation to the points) is not very useful or intuitive. Imagine for example a box, one side twice as long as the others, and rotated by 45 degrees around some axis. Then it is impossible to find a scaling transformation and rotation to transform all points inside the box to the interval [-1; +1]³, considering the scaling is applied first and in each axis separately. It would always result in a parallelepiped.
- if one specifically wants to rotate/translate/whatever the points, it is easy to do so by using the constructor/function that takes an Affine3f and construct for example `Eigen::Affine3f transform=Eigen::Translation3f(...)*Eigen::AngleAxisf(..., Eigen::Vector3f(...).normalized());`

This also fixes incorrect references to a unit cube, which is a cube with edge length 1, or sometimes specifically a cube in the interval [0; 1]. However, BoxClipper3D uses a cube in the interval [-1; +1] (edge length 2).
@mvieth mvieth added changelog: behavior change Meta-information for changelog generation module: filters labels Jul 22, 2023
@mvieth mvieth mentioned this pull request Jul 22, 2023
@mvieth mvieth marked this pull request as ready for review July 26, 2023 18:07
@mvieth mvieth merged commit de6d715 into PointCloudLibrary:master Aug 3, 2023
13 checks passed
@mvieth mvieth deleted the boxclipper3d branch August 3, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: behavior change Meta-information for changelog generation module: filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BoxClipper3D] Incorrect documentation for constructor
2 participants