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 LidarVisual point colors for Ogre1 #124

Merged
merged 6 commits into from
Aug 19, 2020

Conversation

mihirk284
Copy link
Contributor

@mihirk284 mihirk284 commented Aug 11, 2020

@iche033 Please refer to this PR for point color material and implementation, Thanks

EDIT:- adding image
image

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

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@mihirk284 mihirk284 marked this pull request as ready for review August 11, 2020 10:15
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.

works for me, just some minor comments

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);
Copy link
Contributor

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
Copy link
Contributor

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?

#else
Ogre::MaterialPtr mat =
Ogre::MaterialManager::getSingleton().getByName(
"Lidar/BlueRay");
"PointCloudPoint");
Copy link
Contributor

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

iche033 commented Aug 17, 2020

there are some windows build errors, can you take a look?

Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@@ -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++)
Copy link
Contributor

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)

@chapulina chapulina added the 🔮 dome Ignition Dome label Aug 18, 2020
@chapulina chapulina added this to Inbox in Core development via automation Aug 18, 2020
@chapulina chapulina moved this from Inbox to In review in Core development Aug 18, 2020
Signed-off-by: Mihir Kulkarni <mihirk284@gmail.com>
@iche033 iche033 merged commit 489bfc3 into gazebosim:master Aug 19, 2020
Core development automation moved this from In review to Done Aug 19, 2020
@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

3 participants