Visualize Lidar Plugin for ign-gazebo #301

mihirk284 · 2020-08-20T13:17:05Z

@ahcorde Can you please review it.

This PR integrates the LidarVisual from ignition rendering into ign-gazebo. Options are provided to select the visual type, the subscribed topic and to display or ignore lidar rays that do not hit an obstacle.

src/gui/plugins/visualize_lidar/VisualizeLidar.hh

examples/worlds/visualize_lidar.sdf

iche033

one small feature request is to add a checkbox for toggling visibility of the lidar visual in case user has multiple plugins running and they want to selectively disable/enable visualizations

iche033 · 2020-08-20T22:11:09Z

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

+#include <ignition/common/MeshManager.hh>
+#include <ignition/common/Profiler.hh>
+#include <ignition/common/Uuid.hh>
+#include <ignition/common/VideoEncoder.hh>


most of these headers are not needed. Can you remove the ones not used?

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

iche033 · 2020-08-20T22:28:40Z

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

+            {
+              parent = child;
+              foundChild = true;
+              if (i+1 == lidarURIVec.size()-1)


I think the logic in this block assumes the model > link > sensor hierarchy?

We also support nested models, i.e.g model > model > link > sensor, in which case the logic above may not work?

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

iche033 · 2020-08-20T22:42:11Z

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

+  igndbg << "Creating lidar visual" << std::endl;
+
+  auto root = scene->RootVisual();
+  this->dataPtr->lidar = scene->CreateLidarVisual();


I don't see the visual destroyed? we need to be careful not to leave any dangling visuals around when the plugin exits

following up on this, we should destroy the visual when the user closes the plugin, i.e. by clicking on X in the corner of the plugin window. Currently I see that lidar visual remains in the scene after exiting the plugin, and when I open the same plugin again, a duplicate lidar visual gets created.

iche033

there are a couple of minor code check errors:

./src/gui/plugins/visualize_lidar/VisualizeLidar.cc:323:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
./src/gui/plugins/visualize_lidar/VisualizeLidar.cc:374:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

iche033 · 2020-08-24T21:00:54Z

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

+  if (!_value)
+  {
+    ignmsg << "Lidar Visual Display OFF." << std::endl;
+    this->dataPtr->lidar->SetType(rendering::LidarVisualType::LVT_NONE);


you should be able to call this->dataPtr->lidar->SetVisible(false) to make it invisible, which is a faster operation than clearing all the lidar data. Same for re-enabling the lidar visual display below

iche033 · 2020-08-24T21:05:55Z

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

+  igndbg << "Creating lidar visual" << std::endl;
+
+  auto root = scene->RootVisual();
+  this->dataPtr->lidar = scene->CreateLidarVisual();


following up on this, we should destroy the visual when the user closes the plugin, i.e. by clicking on X in the corner of the plugin window. Currently I see that lidar visual remains in the scene after exiting the plugin, and when I open the same plugin again, a duplicate lidar visual gets created.

iche033 · 2020-08-25T20:23:20Z

one small issue:

uncheck Display Lidar Visual -> lidar becomes invisible - correct
uncheck Show Non Hitting Rays -> lidar becomes visible again when it should stay invisible

mihirk284 · 2020-08-26T07:56:06Z

one small issue:

uncheck Display Lidar Visual -> lidar becomes invisible - correct

uncheck Show Non Hitting Rays -> lidar becomes visible again when it should stay invisible

@iche033 Thanks for bringing it to my attention. I think this is happening whenever the DynamicRenderables inside the visual are being deleted and new renderables are being used to display the visual. This happens in the case when there is a change in the LidarVisualType or if we have displayNonHitting = false.
This is happening despite having lidar->SetVisible(false). The new DynamicRenderables generated after this operation always cause the visual to show up.

I can solve this by not updating the visual in the plugin if the display option is off.
Or should we change something in the rendering part of the code?

iche033 · 2020-08-26T22:57:09Z

Or should we change something in the rendering part of the code?

sounds like once we clear the visual data and create new ones in ign-rendering, we need to call SetVisible again with the current visible value so that the visibility is correct.

iche033 · 2020-08-31T21:12:26Z

sounds like once we clear the visual data and create new ones in ign-rendering, we need to call SetVisible again with the current visible value so that the visibility is correct.

fixed in gazebosim/gz-rendering#133

iche033 · 2020-08-31T21:14:49Z

changes look good. Can you merge with / rebase on master?

Signed-off-by: Mihir Kulkarni <[email protected]>

…aying visual Signed-off-by: Mihir Kulkarni <[email protected]>

ahcorde

LGTM, two small comments

examples/worlds/visualize_lidar.sdf

src/gui/plugins/visualize_lidar/VisualizeLidar.cc

Signed-off-by: Mihir Kulkarni <[email protected]>

chapulina

The Mac build failed, I think we need to bump the ign-rendering revision on homebrew-simulation:

/Users/jenkins/workspace/ignition_gazebo-ci-pr_any-homebrew-amd64/ign-gazebo/src/gui/plugins/visualize_lidar/VisualizeLidar.cc:49:10: fatal error: 'ignition/rendering/LidarVisual.hh' file not found
#include "ignition/rendering/LidarVisual.hh"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

chapulina · 2020-09-10T16:56:18Z

@osrf-jenkins run tests, let's see what happens now

chapulina · 2020-09-21T23:36:46Z

@osrf-jenkins run tests 🤞

src/gui/plugins/visualize_lidar/VisualizeLidar.hh

Co-authored-by: Louise Poubel <[email protected]>

mihirk284 requested a review from chapulina as a code owner August 20, 2020 13:17

mihirk284 force-pushed the visualise_lidar branch from c980c5e to 7eb3efe Compare August 20, 2020 13:18

ahcorde requested changes Aug 20, 2020

View reviewed changes

chapulina added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 20, 2020

iche033 reviewed Aug 20, 2020

View reviewed changes

iche033 reviewed Aug 24, 2020

View reviewed changes

mihirk284 mentioned this pull request Aug 28, 2020

Adding option for visibility gazebosim/gz-rendering#133

Merged

mihirk284 force-pushed the visualise_lidar branch 2 times, most recently from 20c1799 to c185c8d Compare September 1, 2020 08:59

mihirk284 added 5 commits September 1, 2020 16:17

visualize lidar plugin

db9b7f4

Signed-off-by: Mihir Kulkarni <[email protected]>

addressed comments

0b83885

Signed-off-by: Mihir Kulkarni <[email protected]>

remove white line

cb9e9a7

Signed-off-by: Mihir Kulkarni <[email protected]>

moved ClearPoints() to rendering thread. other minor changes

43f9b6b

Signed-off-by: Mihir Kulkarni <[email protected]>

deleting visual during plugin destruction, and SetVisible() for displ…

bd34996

…aying visual Signed-off-by: Mihir Kulkarni <[email protected]>

mihirk284 force-pushed the visualise_lidar branch from c185c8d to bd34996 Compare September 1, 2020 10:47

Merge branch 'master' into visualise_lidar

65baf21

iche033 approved these changes Sep 3, 2020

View reviewed changes

iche033 requested a review from ahcorde September 3, 2020 21:24

ahcorde approved these changes Sep 4, 2020

View reviewed changes

examples/worlds/visualize_lidar.sdf Outdated Show resolved Hide resolved

src/gui/plugins/visualize_lidar/VisualizeLidar.cc Outdated Show resolved Hide resolved

mihirk284 and others added 3 commits September 4, 2020 22:03

minor changes

42d221d

Signed-off-by: Mihir Kulkarni <[email protected]>

Merge branch 'master' into visualise_lidar

b95e986

Merge branch 'master' into visualise_lidar

da0f3e0

chapulina reviewed Sep 6, 2020

View reviewed changes

chapulina added needs upstream release Blocked by a release of an upstream library beta Targeting beta release of upcoming collection labels Sep 6, 2020

Merge branch 'master' into visualise_lidar

f646ef2

chapulina removed the needs upstream release Blocked by a release of an upstream library label Sep 18, 2020

chapulina reviewed Sep 22, 2020

View reviewed changes

src/gui/plugins/visualize_lidar/VisualizeLidar.hh Outdated Show resolved Hide resolved

mihirk284 and others added 2 commits September 22, 2020 22:57

remove override from eventFilter

310a417

Co-authored-by: Louise Poubel <[email protected]>

Merge branch 'master' into visualise_lidar

c8f241a

chapulina merged commit 7cac3e4 into gazebosim:master Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualize Lidar Plugin for ign-gazebo #301

Visualize Lidar Plugin for ign-gazebo #301

mihirk284 commented Aug 20, 2020 •

edited

Loading

iche033 left a comment

iche033 Aug 20, 2020

iche033 Aug 20, 2020

iche033 Aug 20, 2020

iche033 Aug 24, 2020

iche033 left a comment

iche033 Aug 24, 2020

iche033 Aug 24, 2020

iche033 commented Aug 25, 2020

mihirk284 commented Aug 26, 2020

iche033 commented Aug 26, 2020 •

edited

Loading

iche033 commented Aug 31, 2020

iche033 commented Aug 31, 2020

ahcorde left a comment

chapulina left a comment

chapulina commented Sep 10, 2020

chapulina commented Sep 21, 2020

Visualize Lidar Plugin for ign-gazebo #301

Visualize Lidar Plugin for ign-gazebo #301

Conversation

mihirk284 commented Aug 20, 2020 • edited Loading

iche033 left a comment

Choose a reason for hiding this comment

iche033 Aug 20, 2020

Choose a reason for hiding this comment

iche033 Aug 20, 2020

Choose a reason for hiding this comment

iche033 Aug 20, 2020

Choose a reason for hiding this comment

iche033 Aug 24, 2020

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

iche033 Aug 24, 2020

Choose a reason for hiding this comment

iche033 Aug 24, 2020

Choose a reason for hiding this comment

iche033 commented Aug 25, 2020

mihirk284 commented Aug 26, 2020

iche033 commented Aug 26, 2020 • edited Loading

iche033 commented Aug 31, 2020

iche033 commented Aug 31, 2020

ahcorde left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

chapulina commented Sep 10, 2020

chapulina commented Sep 21, 2020

mihirk284 commented Aug 20, 2020 •

edited

Loading

iche033 commented Aug 26, 2020 •

edited

Loading