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

Point cloud visualization plugin #346

Merged
merged 13 commits into from
Jan 26, 2022
Merged

Point cloud visualization plugin #346

merged 13 commits into from
Jan 26, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jan 7, 2022

🎉 New feature

Summary

Porting the point cloud visualization tool from osrf/lrauv.

This needs to target Garden because it requires per-point coloring of point clouds:

Features:

  • Display hundreds of thousands of points with different colors
  • Point positions in 3D come from a PointCloudPacked message
  • Optionally, users can choose a separate FloatV topic to get values for each point
  • Point size and color can be changed from the GUI
  • Multiple point clouds can be displayed side-by-side, just instantiate the plugin multiple times

Known issues:

Future work:

There are many improvements that can be made to the plugin. Here are some improvements I can think of for the future:

  • Use a palette with more than 2 colors to have a richer gradient
  • Optionally take color from a channel on the point cloud, instead of another topic
  • Support a custom reference frame, so we can properly visualize LIDAR data - right now all points must be expressed in the world frame

Test it

See the added example and its instructions:

pc_viz_example

Here's an examples using it to display maritime science data for LRAUV:

pc_viz

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

First pass. I looked at the code, but I need to finish rebuilding Ignition G to test this.

I know we've looked at this together a handful of times already, but I'm trying to wear a stricter hat here as it's going into Ignition, whereas in the application-specific project it's more like "anything goes."

src/plugins/point_cloud/PointCloud.hh Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.qml Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.hh Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.hh Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.hh Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.hh Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.cc Outdated Show resolved Hide resolved
src/plugins/point_cloud/PointCloud.cc Outdated Show resolved Hide resolved
@caguero caguero mentioned this pull request Jan 19, 2022
39 tasks
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina marked this pull request as ready for review January 22, 2022 01:59
@chapulina
Copy link
Contributor Author

Thanks for the review, @mabelzhang . I've addressed your comments and added an example. This is now open for review.

Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@mabelzhang mabelzhang 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 example! That made it a lot easier to test. Just two typos.

examples/standalone/point_cloud/README.md Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Just pending green CI. Do we care about the doxygen warnings in the Bionic CI?

@chapulina
Copy link
Contributor Author

Do we care about the doxygen warnings in the Bionic CI?

Ouch oh yeah we do, on it

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Just pending green CI.

Ok, doxygen should be fixed with edecdb8 . Besides that:

Signed-off-by: Louise Poubel <[email protected]>
@mabelzhang
Copy link
Contributor

Okay re Jenkins ones.

Are these ones on the GitHub Actions intermittent?

  	 23 - UNIT_Publisher_TEST (Failed)
  	 35 - INTEGRATION_ExamplesBuild_TEST (Failed)

UNIT_Publisher_TEST is about X, failing only on the PR Action

  [GUI] [Wrn] [Application.cc:674] [QT] QXcbConnection: Could not connect to display :1.0
  [GUI] [Err] [Application.cc:678] [QT] Could not connect to any X display.

INTEGRATION_ExamplesBuild_TEST is failing on both the PR and push Actions.

@chapulina
Copy link
Contributor Author

UNIT_Publisher_TEST is about X, failing only on the PR Action

Yup, these flaky failures due to X are all captured here:

It makes life very complicated as we have to check each test and see if the failure was due to X 😢

@chapulina
Copy link
Contributor Author

Humm now I'm confused about the Helper test failure on Homebrew. It doesn't fail on any of our stable branches, but I can't see why this PR would affect it.

@mabelzhang
Copy link
Contributor

I'm looking at the test results again, a different part of the log, and I'm seeing this related to the ExamplesBuild test that was failing:

  /github/workspace/examples/standalone/point_cloud/point_cloud.cc:31:40: error: variable 'std::atomic<bool> g_terminatePub' has initializer but incomplete type
   static std::atomic<bool> g_terminatePub(false);
                                          ^
  CMakeFiles/point_cloud.dir/build.make:62: recipe for target 'CMakeFiles/point_cloud.dir/point_cloud.cc.o' failed
  make[3]: *** [CMakeFiles/point_cloud.dir/point_cloud.cc.o] Error 1
  CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/point_cloud.dir/all' failed
  make[2]: *** [CMakeFiles/point_cloud.dir/all] Error 2
  Makefile:83: recipe for target 'all' failed
  make[1]: *** [all] Error 2

I think this is a real error?

@chapulina
Copy link
Contributor Author

I think this is a real error?

Thanks for spotting, yes! On it

Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #346 (147a5e4) into main (ebbb88a) will decrease coverage by 1.92%.
The diff coverage is 17.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #346      +/-   ##
==========================================
- Coverage   64.41%   62.49%   -1.93%     
==========================================
  Files          36       37       +1     
  Lines        4952     5164     +212     
==========================================
+ Hits         3190     3227      +37     
- Misses       1762     1937     +175     
Impacted Files Coverage Δ
src/plugins/point_cloud/PointCloud.cc 16.82% <16.82%> (ø)
include/ignition/gui/Helpers.hh 77.77% <50.00%> (-22.23%) ⬇️

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 ebbb88a...147a5e4. Read the comment docs.

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I'm confused about the Helper test failure on Homebrew. It doesn't fail on any of our stable branches, but I can't see why this PR would affect it.

I pushed some bumpers to see if it prevents the test from segfaulting in a8ab1c0. It feels completely unrelated to this PR, but since the test consistently fails on this branch and consistently passes on main (search for "helper" here), I can't test if it works targeting a PR at main 🤷‍♀️

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

I pushed some bumpers to see if it prevents the test from segfaulting in a8ab1c0.

It looks like they worked 🙏

Ok, I think we're in good shape now, phew. Thanks for being thorough!

Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Thank you for making all the fixes!

(If the bumpers aren't enough, welp, a future PR will see it :D)

@chapulina chapulina merged commit 167f80c into main Jan 26, 2022
@chapulina chapulina deleted the chapulina/7/pc branch January 26, 2022 21:39
@scpeters
Copy link
Member

scpeters commented Mar 1, 2022

I'm confused about the Helper test failure on Homebrew. It doesn't fail on any of our stable branches, but I can't see why this PR would affect it.

I pushed some bumpers to see if it prevents the test from segfaulting in a8ab1c0. It feels completely unrelated to this PR, but since the test consistently fails on this branch and consistently passes on main (search for "helper" here), I can't test if it works targeting a PR at main 🤷‍♀️

sadly this test is failing on main, though I don't think it was caused by this PR: #360

@dakejahl
Copy link

dakejahl commented Nov 9, 2023

Support a custom reference frame, so we can properly visualize LIDAR data - right now all points must be expressed in the world frame

I am trying to visualize a point cloud from a camera sensor attached to a drone but the data is in body frame. Has this been implemented and can someone steer me to the right place to add this?

@mabelzhang
Copy link
Contributor

There should be a frame ID in the header... I would think. Line 48 in examples/standalone/point_cloud/point_cloud.cc in this PR seems to suggest it's been implemented.

There's usage example in the gz-sim integration tests e.g.
https://github.com/gazebosim/gz-sim/blob/gz-sim8/test/integration/environment_preload_system.cc
https://github.com/gazebosim/gz-sim/blob/gz-sim8/test/integration/environmental_sensor_system.cc

gz-sim environment system implementation is here
https://github.com/gazebosim/gz-sim/tree/gz-sim8/src/systems/environment_preload
https://github.com/gazebosim/gz-sim/tree/gz-sim8/src/systems/environmental_sensor_system

There's a tutorial for using the environment system, in a PR gazebosim/gz-sim#1806

@dakejahl
Copy link

dakejahl commented Nov 10, 2023

@mabelzhang thanks for the reply. I took a look at those files and it's not clear to me how to access reference frame information of PointCloud data within a plugin. When I search for "frame" or "model" in the src/plugins/ I don't see any other examples of converting between reference frames for sensor data attached to a moving model. The goal is to visualize obstacle detect and avoid on PX4. Thanks for the help!

Here's the model I'm using for the drone, you can see how the camera is attached
https://github.com/PX4/PX4-Autopilot/blob/main/Tools/simulation/gz/models/x500_depth/model.sdf

I have some example code where I am downsampling a camera point cloud and republishing it
dakejahl/PX4-Autopilot@02ef528

Here's a pic of a drone with a depth camera looking at a block some distance from the origin
image

@mabelzhang
Copy link
Contributor

Hey, okay, so first, we really shouldn't be starting a new topic in a merged PR. The more correct thing to do would be to open an issue and tag this PR as the relevant source code. I replied with links to the source code because they depend on code in this PR. But with the new details you posted, it should really be in a new issue ticket for better visibility.

Please keep that in mind and open a new ticket if you post new information.

@arjo129 can people set frame ID when visualizing point clouds? That would be part of the environment system right?

@arjo129
Copy link
Contributor

arjo129 commented Nov 10, 2023

To be fair this has nothing to do with the environment system. We should open a ticket, I suspect point clouds don't follow frame_ids set 😱 .

@dakejahl
Copy link

Sorry about that! I figured posting here was the best way to get the attention of the person who knows! 😁

Ticket here
#595

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants