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 ColorHandler to color points with individual RGB values #1570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aecins
Copy link
Contributor

@aecins aecins commented Mar 28, 2016

Adds a new PointCloudColorHandlerCustomIndividual color handler to PCLVisualizer that allows to color each point in the pointcloud with a different color. Here is an example of a visualization of a cloud where first 1/3 of points are red, second 1/3 is green and last 1/3 is blue:
cloud_colored

Conflicts:

visualization/include/pcl/visualization/point_cloud_color_handlers.h
@taketwo
Copy link
Member

taketwo commented Jun 6, 2016

I'm not sure if we need an new color handler for this. Can you just change the point type to something that includes RGB field and merge the color information there?

@aecins
Copy link
Contributor Author

aecins commented Jun 6, 2016

You can definitely achieve the same effect by creating a new pointcloud that has RGB field and specifying the RGB values. But that requires a copy of the data. I guess it might not be too much of an issue. But I have found PointCloudColorHandlerCustomIndividual useful when I needed to quickly visualize some value computed over all of the points in the cloud and none of the default PCL color handlers did what I needed.

@taketwo
Copy link
Member

taketwo commented Jun 8, 2016

Well in it's current form PointCloudColorHandlerCustomIndividual also requires copying of color data (line 270).

My feeling is that this handler is superfluous. I agree that there might be a use case, but arguably it is a fairly rare case, and can be covered by using an RGB-enabled point type. So I would rather be against merging this, but if there are other opinions, please post.

@aecins
Copy link
Contributor Author

aecins commented Jun 10, 2016

You are right about the data copy. I agree with you that this handler is not strictly speaking necessary. It would be interesting to see if anyone else has an opinion on it.

@aecins
Copy link
Contributor Author

aecins commented Aug 1, 2016

It seems like this feature is non-essential. Should we close this pull request?

capable_ = true;
else
capable_ = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply

capable_ = (cloud_->points.size () == rgb_values_.size ())

@SergioRAgostinho
Copy link
Member

The use case I see for this is in case you have a colored pointcloud already but want to override its colors for visualization purposes without copying the the XYZ data. I don't really see an issue in having it as a non-essential feature, it might convenient in some situations.

Also since the PointCloudColorHandlerCustomIndividual is not precompiled, we won't be increasing the compilation time and library binary size.

@SergioRAgostinho SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Aug 20, 2016
@jspricke
Copy link
Member

I agree with @taketwo that it's more consistent to create a new cloud, even in the case @SergioRAgostinho proposes.

@SergioRAgostinho
Copy link
Member

Fair enough. Can we close this then? Sorry for the trouble @aecins :x

@aecins
Copy link
Contributor Author

aecins commented Aug 29, 2016

No problem, @SergioRAgostinho, this can be closed. The final decision is that this functionality is not needed so I will not implement it in PointCloudColorHandlerCustom as I suggested in #1627.

@taketwo
Copy link
Member

taketwo commented Aug 30, 2016

Actually, I am in favor of having this functionality as a part of PointCloudColorHandlerCustom. My original concern was about having million different handlers with not exactly intuitive names (albeit logical when you know what it's doing). Now that we agreed to have a single umbrella handler for user-provided colors, I think this pull request functionality fits nicely.

@stale
Copy link

stale bot commented Feb 22, 2020

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

@stale stale bot added the status: stale label Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: proposal Type of issue needs: author reply Specify why not closed/merged yet status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants