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

Document the RGBA structure #6128

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

Conversation

rurban
Copy link

@rurban rurban commented Sep 9, 2024

No description provided.

common/include/pcl/impl/point_types.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/point_types.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/point_types.hpp Outdated Show resolved Hide resolved
@larshg
Copy link
Contributor

larshg commented Sep 10, 2024

Shouldn't it be RGBA all places, since the alpha part is always present? I couldn't find a RGB only struct at least?

@rurban
Copy link
Author

rurban commented Sep 12, 2024

Shouldn't it be RGBA all places, since the alpha part is always present? I couldn't find a RGB only struct at least?

No they are separate, even if they share the same uint32_t storage. pcl::PointXYZRGB != pcl::PointXYZRGBA

common/include/pcl/impl/point_types.hpp Outdated Show resolved Hide resolved
common/include/pcl/impl/point_types.hpp Outdated Show resolved Hide resolved
@larshg
Copy link
Contributor

larshg commented Sep 19, 2024

Ahh, but there is only a single pcl::RGB struct - which expose the A channel with default 255 value. There are no pcl::RGBA struct.

But indeed there is PointXYZRGB and PointXYZRGBA.
And what I can see PointXYZRGB only has constructors that accept XYZ and RGB - ie. hides the alpha channel.

I don't see the necessary of having PointXYZRGB, since it still has the same size as PointXYZRGBA - but I don't see us removing it now anyways...?
And pcl::RGB should be pcl::RGBA, since it does contain both the space and expose the alpha channel.

I would vote that the documentation explains this for the pcl::RGB struct though?

What do you think @mvieth ?

@mvieth
Copy link
Member

mvieth commented Sep 26, 2024

Ahh, but there is only a single pcl::RGB struct - which expose the A channel with default 255 value. There are no pcl::RGBA struct.

But indeed there is PointXYZRGB and PointXYZRGBA. And what I can see PointXYZRGB only has constructors that accept XYZ and RGB - ie. hides the alpha channel.

I don't see the necessary of having PointXYZRGB, since it still has the same size as PointXYZRGBA - but I don't see us removing it now anyways...? And pcl::RGB should be pcl::RGBA, since it does contain both the space and expose the alpha channel.

I would vote that the documentation explains this for the pcl::RGB struct though?

What do you think @mvieth ?

Agreed -- removing or renaming point types is not possible any more, but the documentation should say that the alpha value can be stored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants