-
Notifications
You must be signed in to change notification settings - Fork 53
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 LidarVisual point colors for Ogre1 #124
Conversation
Signed-off-by: Mihir Kulkarni <[email protected]>
Signed-off-by: Mihir Kulkarni <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me, just some minor comments
ogre/src/OgreLidarVisual.cc
Outdated
this->dataPtr->pointColors.clear(); | ||
for (u_int64_t i = 0; i < this->dataPtr->lidarPoints.size(); i++) | ||
{ | ||
this->dataPtr->pointColors.push_back(ignition::math::Color::White); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep the color consistent and set it to the same color as Lidar/BlueRay
.
@@ -60,6 +60,11 @@ namespace ignition | |||
public: virtual void SetPoints( | |||
const std::vector<double> &_points) override; | |||
|
|||
// Documentation inherited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a note here that it currently only affects lidar visuals with type LVT_POINTS
?
ogre/src/OgreLidarVisual.cc
Outdated
#else | ||
Ogre::MaterialPtr mat = | ||
Ogre::MaterialManager::getSingleton().getByName( | ||
"Lidar/BlueRay"); | ||
"PointCloudPoint"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization suggestion: can you move the Ogre::MaterialManager::getSingleton().getByName
calls outside the for loop so we don't have to search for the material every iteration of the loop.
same for the other materials like Lidar/BlueStrips
, Lidar/TransBlack
, Lidar/LightBlueStrips
Signed-off-by: Mihir Kulkarni <[email protected]>
Signed-off-by: Mihir Kulkarni <[email protected]>
there are some windows build errors, can you take a look? |
Signed-off-by: Mihir Kulkarni <[email protected]>
ogre/src/OgreLidarVisual.cc
Outdated
@@ -149,6 +152,26 @@ void OgreLidarVisual::ClearVisualData() | |||
void OgreLidarVisual::SetPoints(const std::vector<double> &_points) | |||
{ | |||
this->dataPtr->lidarPoints = _points; | |||
this->dataPtr->pointColors.clear(); | |||
for (auto i = 0ul; i < this->dataPtr->lidarPoints.size(); i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need an unsigned long? vector size()
returns size_t
which is an unsigned integer
this should work
for (unsigned int i = 0u; i < this->dataPtr->lidarPoints.size(); ++i)
Signed-off-by: Mihir Kulkarni <[email protected]>
@iche033 Please refer to this PR for point color material and implementation, Thanks
EDIT:- adding image

To use this you can do the following :-
lidarPointer->SetPoints(points_vector, colors_vector);