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

Added LidarVisual implementation for Ogre1, empty classes for Ogre2 #103

Merged
merged 34 commits into from
Jul 8, 2020

Conversation

mihirk284
Copy link
Contributor

Please see issue ignitionrobotics/ign-rendering#84.

I have added lidar visualisation for Ogre1 and an example for the same.

The following image shows an example of the LidarVisual with 3 vertical sets of rays.
image

@ahcorde Please do check the PR and let me know if there are any changes. ( This PR is different from the previous one so that it would be easier to review the code as it is separated cleanly per commit.)

@mihirk284 mihirk284 requested a review from iche033 as a code owner June 20, 2020 17:52
@ahcorde ahcorde self-requested a review June 20, 2020 18:59
@ahcorde ahcorde added enhancement New feature or request ogre1.x 🔮 dome Ignition Dome labels Jun 20, 2020
@ahcorde ahcorde linked an issue Jun 20, 2020 that may be closed by this pull request
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.

Please run bash tools/code_check.sh inside the project folder and fix the linter errors in the files that you are adding to the project.

./src/LidarVisual_TEST.cc:105:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
./include/ignition/rendering/LidarVisual.hh:36:  Tab found; better to use spaces  [whitespace/tab] [1]
./include/ignition/rendering/LidarVisual.hh:37:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
...

examples/lidar_visual/Main.cc Outdated Show resolved Hide resolved
src/LidarVisual.cc Outdated Show resolved Hide resolved
src/LidarVisual_TEST.cc Outdated Show resolved Hide resolved
include/ignition/rendering/LidarVisual.hh Show resolved Hide resolved
include/ignition/rendering/base/BaseLidarVisual.hh Outdated Show resolved Hide resolved
@mihirk284 mihirk284 changed the title Added LidarVisual for Ogre1 Added LidarVisual implementation for Ogre1, empty classes for Ogre2 Jun 21, 2020
@mihirk284 mihirk284 force-pushed the temp_lidar_visual branch from cee20c0 to a16a92f Compare June 21, 2020 11:39
Signed-off-by: Mihir Kulkarni <[email protected]>
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.

I can still see some linter errors in:

  • include/ignition/rendering/base/BaseLidarVisual.hh
  • include/ignition/rendering/LidarVisual.hh

Please add an entry in the Changelog.md

@mihirk284 mihirk284 force-pushed the temp_lidar_visual branch from ae11b6e to 7221f7e Compare June 27, 2020 15:23
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.

minor fixes

include/ignition/rendering/LidarVisual.hh Outdated Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
src/LidarVisual_TEST.cc Show resolved Hide resolved
test/integration/lidar_visual.cc Show resolved Hide resolved
test/integration/lidar_visual.cc Show resolved Hide resolved
Signed-off-by: Mihir Kulkarni <[email protected]>
@mihirk284 mihirk284 force-pushed the temp_lidar_visual branch from 5b5e49f to 9115a53 Compare June 29, 2020 11:59
Signed-off-by: Mihir Kulkarni <[email protected]>
@mihirk284
Copy link
Contributor Author

Added example of a GPU rays sensor with data collected lidar data being visualised
image

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.

I made a few comments but overall the lidar_visual example works great for me. Good job.

examples/lidar_visual/GlutWindow.hh Outdated Show resolved Hide resolved
examples/lidar_visual/Main.cc Outdated Show resolved Hide resolved
examples/lidar_visual/Main.cc Outdated Show resolved Hide resolved
examples/lidar_visual/README.md Outdated Show resolved Hide resolved
include/ignition/rendering/LidarVisual.hh Outdated Show resolved Hide resolved
include/ignition/rendering/LidarVisual.hh Outdated Show resolved Hide resolved
include/ignition/rendering/LidarVisual.hh Outdated Show resolved Hide resolved
test/integration/lidar_visual.cc Show resolved Hide resolved
test/integration/lidar_visual.cc Outdated Show resolved Hide resolved
examples/lidar_visual/GlutWindow.cc Outdated Show resolved Hide resolved
Signed-off-by: Mihir Kulkarni <[email protected]>
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.

alright just did a second pass review and only have some minor comments

ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
include/ignition/rendering/base/BaseLidarVisual.hh Outdated Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
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.

I left a couple comments related to CI builds. Let's try and make CI green before merging

ogre/src/OgreLidarVisual.cc Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Outdated Show resolved Hide resolved
ogre/include/ignition/rendering/ogre/OgreScene.hh Outdated Show resolved Hide resolved
line->setMaterial("Lidar/BlueStrips");
#else
line->setMaterial(
this->Scene()->Material("Lidar/BlueStrips")->Clone());
Copy link
Contributor

@iche033 iche033 Jul 6, 2020

Choose a reason for hiding this comment

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

This returns an ignition::rendering::MaterialPtr object but setMaterial expects an Ogre::MateiralPtr object. So windows build is still failing, see console output

You'll need to get the material using ogre's material manager:

Ogre::MaterialPtr mat = Ogre::MaterialManager::getSingleton().getByName("Lidar/BlueStrips");

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.

thanks for iterating!

@iche033 iche033 merged commit e445c9b into gazebosim:master Jul 8, 2020
@iche033
Copy link
Contributor

iche033 commented Jul 10, 2020

It looks like this change introduced four new test failures on Windows:

hmm unfortunately that's expected when new tests are added since windows build is not working properly. There is an attempt to fix windows CI in pull request #109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request ogre1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create lidar visualization
5 participants