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 lidar heightmap detection #760

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Fix lidar heightmap detection #760

merged 3 commits into from
Nov 15, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Nov 12, 2022

Signed-off-by: Ian Chen [email protected]

🦟 Bug fix

Fixes #712

Summary

Fixes lidar heightmap detection which was broken when visibility mask / flags were added. It seems to only have affected ogre2 implementation. The new integration test passes with ogre 1.x locally for me without making any changes to the ogre implementation.

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

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

@iche033 iche033 changed the base branch from gz-rendering7 to ign-rendering6 November 12, 2022 00:34
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Nov 12, 2022
@iche033 iche033 added 🏯 fortress Ignition Fortress bug Something isn't working labels Nov 12, 2022
Signed-off-by: Ian Chen <[email protected]>
@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Merging #760 (f16cee6) into ign-rendering6 (0410533) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@                Coverage Diff                 @@
##           ign-rendering6     #760      +/-   ##
==================================================
- Coverage           77.82%   77.81%   -0.01%     
==================================================
  Files                 146      146              
  Lines               13397    13397              
==================================================
- Hits                10426    10425       -1     
- Misses               2971     2972       +1     
Impacted Files Coverage Δ
ogre2/src/Ogre2GpuRays.cc 95.51% <100.00%> (ø)
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor suggestions

test/integration/gpu_rays.cc Outdated Show resolved Hide resolved
test/integration/gpu_rays.cc Outdated Show resolved Hide resolved
Signed-off-by: Ian Chen <[email protected]>
@ahcorde ahcorde merged commit 913efa6 into ign-rendering6 Nov 15, 2022
@ahcorde ahcorde deleted the lidar_heightmap branch November 15, 2022 08:38
iche033 added a commit that referenced this pull request Dec 12, 2022
Calling Sensor::SetVisibilityMask in ogre2 with bits 31 or 30 set (which are reserved by OgreNext) would cause wrong rendering.

PR #760 already tries to fix this issue on a localized problem: heightmap in lidar.

This is the general solution.

This PR simply uses PassSceneDef::setVisibilityMask instead of directly setting PassSceneDef::mVisibilityMask to mask out the reserved flags.

Ogre2Camera::setVisibilityMask has been modified to warn when the reserved flags are set.

Other cameras (e.g. Lidar's Ogre2GpuRays) won't warn though, since they don't derive from Ogre2Camera. I cannot add such warning without breaking the ABI.

Signed-off-by: Matias N. Goldberg <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
KristjanDeepX added a commit to DeepX-inc/gz-rendering that referenced this pull request Feb 3, 2023
This is a backport of gazebosim#760
which is a fix to gazebosim#712
KristjanDeepX added a commit to DeepX-inc/gz-rendering that referenced this pull request Feb 3, 2023
This is a backport of gazebosim#760
which is a fix to gazebosim#712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

GPU lidar sensor misses heightmap visuals
2 participants