Skip to content

Correct calculation in ProjectPointToPlane#702

Merged
scpeters merged 3 commits intogazebosim:gz-math9from
srmainwaring:prs/project-point-to-plane-fix
Oct 17, 2025
Merged

Correct calculation in ProjectPointToPlane#702
scpeters merged 3 commits intogazebosim:gz-math9from
srmainwaring:prs/project-point-to-plane-fix

Conversation

@srmainwaring
Copy link
Copy Markdown
Contributor

@srmainwaring srmainwaring commented Oct 16, 2025 •
edited
Loading

🦟 Bug fix

Summary

Use unit normal for projection in gz::math::ProjectPointToPlane.

Before this change the function used the unnormalised normal to calculated the projection of the interpolation point onto one of the grid parallelpiped surfaces. This caused the gz::math::TrilinearInterpolate to return an incorrect interpolated value. These functions are used in the TimeVaryingVolumetricGrid, which in turn is used by the Environment system. The upshot is that this system incorrectly interpolates data whenever the trilinear interpolant is used. It is apparent when examining volumetric data using the PointCloud GUI widget, as the ranges displayed in the widget do not correspond to the underlying data provided to the Environment preload system.

After this change the PointCloud display is consistent with the input data.

The unit test InterpolationPoint_TEST does not catch this case, because the data is provided for a unit cube, and the normals calculated for this cube all have unit length, so the effect of scaling the normal is not tested. This is also the case for the environmental system tests in gz-sim. It may be better to use non-unit grids for testing systems that involve geometric calculations involving vector operations that depend on normalisation.

There is a work-in-progress example here (https://github.com/srmainwaring/gz-math/tree/prs/volumetric-grid-example) that highlights the issue for a 4 x 4 x 4 grid sampled from a wind field.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

@srmainwaring
Correct calculation in ProjectPointToPlane
fc8cb3d 
- Use unit normal for projection.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@github-actions github-actions bot added the 🪵 jetty Gazebo Jetty label Oct 16, 2025
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 16, 2025

The unit test InterpolationPoint_TEST does not catch this case, because the data is provided for a unit cube, and the normals calculated for this cube all have unit length, so the effect of scaling the normal is not tested. This is also the case for the environmental system tests in gz-sim. It may be better to use non-unit grids for testing systems that involve geometric calculations involving vector operations that depend on normalisation.

There is a work-in-progress example here (https://github.com/srmainwaring/gz-math/tree/prs/volumetric-grid-example) that highlights the issue for a 4 x 4 x 4 grid sampled from a wind field.

if we scaled the unit cube in InterpolationPoint_TEST.cc to a larger or smaller size would that be enough to illustrate the bug? I'd like to capture this effect in our tests if possible

@srmainwaring
Copy link
Copy Markdown
Contributor Author

srmainwaring commented Oct 17, 2025 •
edited
Loading

if we scaled the unit cube in InterpolationPoint_TEST.cc to a larger or smaller size would that be enough to illustrate the bug? I'd like to capture this effect in our tests if possible

Yes, that should capture it. Uniform scaling will change the magnitude of the normal but not the expected interpolated values.

Edit

Seems that we also need to add a gradient to the field to ensure that the normalisation is fully checked. Test updated in b6c56dc. It will fail before this change, pass after.

@srmainwaring
Update InterpolatePointTest to capture projection edges cases
b6c56dc 
- Resize the sample grid and add a gradient to the field to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
scpeters
scpeters approved these changes Oct 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

thanks for adding the test!

@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development Oct 17, 2025
@scpeters scpeters merged commit 06d73ac into gazebosim:gz-math9 Oct 17, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Core development Oct 17, 2025
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 17, 2025

https://github.com/Mergifyio backport main

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 17, 2025 •
edited
Loading

backport main

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @mergify
Correct calculation in ProjectPointToPlane (#702)
f4a2e74 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
@mergify mergify bot mentioned this pull request Oct 17, 2025
Merged
9 tasks
scpeters pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @scpeters
Correct calculation in ProjectPointToPlane (#702)
0b34f6d 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 17, 2025

https://github.com/Mergifyio backport gz-math8 gz-math7

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 17, 2025 •
edited
Loading

backport gz-math8 gz-math7

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @mergify
Correct calculation in ProjectPointToPlane (#702)
d1e3481 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
@mergify mergify bot mentioned this pull request Oct 17, 2025
Merged
9 tasks
mergify bot pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @mergify
Correct calculation in ProjectPointToPlane (#702)
e8be117 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
@mergify mergify bot mentioned this pull request Oct 17, 2025
Merged
9 tasks
scpeters pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @scpeters
Correct calculation in ProjectPointToPlane (#702)
866fab7 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
scpeters pushed a commit that referenced this pull request Oct 17, 2025
@srmainwaring @scpeters
Correct calculation in ProjectPointToPlane (#702)
981387a 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
C88-YQ pushed a commit to C88-YQ/gz-math that referenced this pull request Oct 28, 2025
@srmainwaring @C88-YQ
Correct calculation in ProjectPointToPlane (gazebosim#702)
28a20ba 
Use unit normal for projection.

Update InterpolatePointTest to capture projection edges cases
- Resize the sample grid and add a gradient to the field
  to check projection normalisation.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
(cherry picked from commit 06d73ac)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters scpeters approved these changes

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

No one assigned

Labels

🪵 jetty Gazebo Jetty

Projects

Core development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@srmainwaring @scpeters @azeey