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

Check if key exists in gpu lidar's user data #396

Merged
merged 3 commits into from
Sep 14, 2021
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Sep 7, 2021

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

🦟 Bug fix

Partially fixes #394

Summary

The problem is described in #394. Summary is that the gpu lidar is flooding the console with Error casting user data: Unexpected index message because because objects in the scene do not have laser_retro set. This is actually ok as not all objects contain laser_retro values. This property is set in ign-gazebo.

This PR adds a HasUserData API to check if a UserData key exists or not. This avoid additional parsing in gpu lidar to figure out if the user data contains valid value or not.

The second issue identified in #394 is that the visibility flags set to objects with laser_retro do not have effect.

the intended path is that items whose retroValue is not set should not be rendered; however they are being rendered.

That's found to be the case. This issue is not fixed in this PR as that turns out to require more work. I've removed the visibility mask/flag logic as they are not doing their job, and added a todo in the code.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 7, 2021
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #396 (603246b) into main (55a3e43) will decrease coverage by 0.02%.
The diff coverage is 77.77%.

❗ Current head 603246b differs from pull request most recent head 4cacd59. Consider uploading reports for the commit 4cacd59 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   58.67%   58.64%   -0.03%     
==========================================
  Files         174      174              
  Lines       17276    17278       +2     
==========================================
- Hits        10137    10133       -4     
- Misses       7139     7145       +6     
Impacted Files Coverage Δ
include/ignition/rendering/Node.hh 100.00% <ø> (ø)
ogre2/src/Ogre2GpuRays.cc 95.30% <76.00%> (-0.84%) ⬇️
include/ignition/rendering/base/BaseNode.hh 81.81% <100.00%> (+0.18%) ⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a3e43...4cacd59. Read the comment docs.

@chapulina
Copy link
Contributor

It's worth pointing out that since #371, UserData will return an invalid variant if it's unset. So downstream callers should be able to check something like if (!userData.index() == 0). I don't think that's very user friendly though, so an explicit HasUserData function sounds like a good addition.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@iche033
Copy link
Contributor Author

iche033 commented Sep 8, 2021

So downstream callers should be able to check something like if (!userData.index() == 0)

true, I added a check in the test in bade294 as a reminder that this also works.

@adlarkin adlarkin self-requested a review September 14, 2021 14:46
@chapulina chapulina merged commit 993bb0b into main Sep 14, 2021
@chapulina chapulina deleted the check_user_data branch September 14, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants