Skip to content

Added Logic to flag pointcloud if invalid point is Detected#180

Merged
chapulina merged 5 commits intogazebosim:ign-sensors6from
carlos-m159:bugfix/179-correct-is-dense-pointcloud-flag
Dec 21, 2021
Merged

Added Logic to flag pointcloud if invalid point is Detected#180
chapulina merged 5 commits intogazebosim:ign-sensors6from
carlos-m159:bugfix/179-correct-is-dense-pointcloud-flag

Conversation

@carlos-m159
Copy link
Contributor

@carlos-m159 carlos-m159 commented Dec 18, 2021

🦟 Bug fix

Fixes #179

Summary

Currently, when filling pointcloud message, the is_dense field is always set to true. According to the definition used by PCL, a pointcloud should be considered dense if all point are valid (i.e. not NaN nor +/-INF). Please refer to the linked issue to understand the common problem that this can cause when using PCL lib.

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@carlos-m159 carlos-m159 requested a review from iche033 as a code owner December 18, 2021 02:49
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 18, 2021
@carlos-m159 carlos-m159 force-pushed the bugfix/179-correct-is-dense-pointcloud-flag branch from cd1ac9e to 821bd79 Compare December 18, 2021 03:04
Signed-off-by: Carlos Mendes <carlos.mendes@mov.ai>
@carlos-m159 carlos-m159 force-pushed the bugfix/179-correct-is-dense-pointcloud-flag branch from 821bd79 to e44ad43 Compare December 18, 2021 03:13
@chapulina chapulina added the bug Something isn't working label Dec 18, 2021
Signed-off-by: Carlos Mendes <carlos.mendes@mov.ai>
@carlos-m159 carlos-m159 requested a review from ahcorde December 20, 2021 12:59
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks for the fix.

@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #180 (cb38b1b) into ign-sensors6 (8ae7167) will increase coverage by 0.02%.
The diff coverage is 100.00%.

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

@@               Coverage Diff                @@
##           ign-sensors6     #180      +/-   ##
================================================
+ Coverage         76.01%   76.04%   +0.02%     
================================================
  Files                27       27              
  Lines              2760     2763       +3     
================================================
+ Hits               2098     2101       +3     
  Misses              662      662              
Impacted Files Coverage Δ
src/GpuLidarSensor.cc 88.53% <100.00%> (+0.22%) ⬆️

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 8ae7167...c78c77e. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks! Should we update the migration guide explaining that the behaviour will change in the next 6.X release?

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Carlos Mendes <carlos.mendes@mov.ai>
@carlos-m159 carlos-m159 force-pushed the bugfix/179-correct-is-dense-pointcloud-flag branch from 9f738ae to c7956da Compare December 21, 2021 14:40
@carlos-m159
Copy link
Contributor Author

@chapulina will you update afterwards? Regarding the merge, will you handle it?

@chapulina
Copy link
Contributor

@carlos-m159 , do you mind adding a note to Migration.md? You can add a new section "Ignition Sensors 6.0.1 to 6.X.X", and we update the X when we trigger a release. Thanks 🙇‍♀️

Once you do it I can merge it

Signed-off-by: Carlos Mendes <carlos.mendes@mov.ai>
@carlos-m159
Copy link
Contributor Author

@chapulina Migration doc updated.

@chapulina chapulina enabled auto-merge (squash) December 21, 2021 20:07
@chapulina chapulina merged commit 16e52cc into gazebosim:ign-sensors6 Dec 21, 2021
@carlos-m159 carlos-m159 deleted the bugfix/179-correct-is-dense-pointcloud-flag branch December 22, 2021 09:56
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3D Pointcloud is flagged as dense containing INF

5 participants