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

Add types for lidar visual #114

Merged
merged 17 commits into from Jul 22, 2020
Merged

Conversation

mihirk284
Copy link
Contributor

Added points and lines to the lidar visual. Additional API to display only those points that are hitting is also added.

Please see issue ignitionrobotics/ign-rendering#84.

POINTS2
POINTS1
RAY2
RAY1

@mihirk284 mihirk284 requested a review from iche033 as a code owner July 12, 2020 09:12
@ahcorde
Copy link
Contributor

ahcorde commented Jul 13, 2020

@mihirk284 can you review the history of the commits?

It looks like only the 2 or 3 last commits are not in master

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
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.

Changing the visualization type in the example works for me! I left a few inline comments.

There are some coding style errors. You can find them doing running sh tools/code_check.sh from the ign-rendering root dir.

Please also update changelog.

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 Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc Show resolved Hide resolved
examples/lidar_visual/Main.cc Show resolved Hide resolved
include/ignition/rendering/LidarVisual.hh Show resolved Hide resolved
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
ogre/src/OgreLidarVisual.cc Show resolved Hide resolved
ogre/src/OgreLidarVisual.cc 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 noticed that in point visualization mode, a point is drawn in the center of the lidar visual. Do you see that?

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@mihirk284
Copy link
Contributor Author

mihirk284 commented Jul 16, 2020

I think that happened because of our initial assumption that the number of points remains the same for the visuals. In this case, I am updating the code to clear all visuals inside the Update() when the displayNonHitting value is set to true. This will remove the older points from memory instead of placing them at the centre of the visual. This change makes the Update() slightly slower than before.

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@chapulina chapulina added this to Inbox in Core development via automation Jul 20, 2020
@chapulina chapulina added the 🔮 dome Ignition Dome label Jul 20, 2020
@chapulina chapulina moved this from Inbox to In review in Core development Jul 20, 2020
examples/lidar_visual/GlutWindow.cc Outdated Show resolved Hide resolved
examples/lidar_visual/GlutWindow.cc Outdated Show resolved Hide resolved
examples/lidar_visual/GlutWindow.cc Outdated Show resolved Hide resolved
examples/lidar_visual/Main.cc Outdated Show resolved Hide resolved
@@ -26,6 +26,8 @@ namespace ic = ignition::common;

/// \brief Run the demo and display Lidar Visual
/// \param[in] _cameras Cameras in the scene
Copy link
Contributor

Choose a reason for hiding this comment

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

Document arguments

mihirk284 and others added 3 commits July 21, 2020 22:01
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
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.

just need to fix windows build error. Otherwise, it's good to go.

#if (OGRE_VERSION <= ((1 << 16) | (10 << 8) | 7))
line->setMaterial("Lidar/LightBlueStrips");
#else
mat = Ogre::MaterialManager::getSingleton().getByName(
Copy link
Contributor

Choose a reason for hiding this comment

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

windows CI caught a build error here. This needs to be:

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

#if (OGRE_VERSION <= ((1 << 16) | (10 << 8) | 7))
line->setMaterial("Lidar/TransBlack");
#else
mat = Ogre::MaterialManager::getSingleton().getByName(
Copy link
Contributor

Choose a reason for hiding this comment

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

same change is needed here. Need to declare mat

#if (OGRE_VERSION <= ((1 << 16) | (10 << 8) | 7))
line->setMaterial("Lidar/BlueStrips");
#else
mat = Ogre::MaterialManager::getSingleton().getByName(
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

#if (OGRE_VERSION <= ((1 << 16) | (10 << 8) | 7))
line->setMaterial("Lidar/BlueRay");
#else
mat = Ogre::MaterialManager::getSingleton().getByName(
Copy link
Contributor

Choose a reason for hiding this comment

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

another one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iche033 I think only this one needs the declaration. The others above are in the same namespace so the variable can be reused. I've pushed the edit for the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

new build looks good!

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@iche033 iche033 merged commit e22fd80 into gazebosim:master Jul 22, 2020
Core development automation moved this from In review to Done Jul 22, 2020
@mihirk284 mihirk284 deleted the lidar_visual_add_types branch August 5, 2020 10:28
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants