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
Adding red, green, blue field support to the binary, and ascii pcd lo… #28179
Conversation
I've always struggled with such enhancements for https://pointclouds.org/documentation/tutorials/pcd_file_format.html According to this document, separate RGB fields are not supported. Considering this document, it's fair to ask why a PCD exporter has to report separate RGB fields if the exporter devs should know this approach is uncommon for PCD. Design decisions like that only make compatibility between different software harder. Unless there is a very good reason for having separate "red", "green", "blue" fields, I'm not willing to vote for this change. |
@meyerjo Could you explain where these values come from and what's the PCD ecosystem support for them? |
I find the whole situation around the PCD file definition a bit messy. I read the definition of the file format header as "Examples:" but you could always add new columns to it. In my case, I have data which is originally stored in a las file which contains many additional information such as the intensity, number of returns etc and the color specified in three different fields red, green, blue (format definitions in: https://www.asprs.org/wp-content/uploads/2010/12/asprs_las_format_v12.pdf see i.e. page 10). When processing and converting them to pcd using libraries such as
are generated. However, it is not possible at the moment to read these color information into your nice library. Thus, I decided to add this behavior to the PCDLoader and contribute it back. |
|
||
const red = dataview.getFloat64( row + offset.red , true); | ||
const green = dataview.getFloat64( row + offset.green , true); | ||
const blue = dataview.getFloat64( row + offset.blue , true); |
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.
The problem is your code is application specific since it does not implement a real standard. You are assuming here that R,G,B fields are floats but what if other tools export uint8
values?
Like mentioned above, such modifications are not reliable in library code. PCDLoader
isn't a large module so it's best to copy the module and add custom properties on application level.
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.
You are right, I can change this. But you are willing to add this capability in general aren't you?. Your first response read more like a hard no. Hence, I am asking.
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.
I'm still not in favor of this change, no. If we merge this PR and similar ones, we end up with an overly complex PCDLoader
supporting a huge number of potentially redundant fields.
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.
Um, I wonder if we eventually need a more generic approach in PCDLoader
similar to what #27901 has suggested.
The loader could parse the existing default fields like x
, y
, z
and return a geometry as before. But now the loader also parses all other fields it finds in the PCD asset. The related data could be returned as a simple map (field name -> typed array) in the userData
field of the geometry. Ideally the geometry only holds fields as attributes when they are relevant for rendering.
Some https://pdal.io/en/2.7.1/about.html |
Closing,see #28179 (comment). |
Description
This adds the ability to load the color information from pcd files which do not specify an
"rgb"
field but the color information split up to the fields"red", "green", "blue"
.I tested it with a pcd files converted from a las-file via
pdal
with compression:binary
,ascii
.