search

daavoo / pyntcloud

pyntcloud is a Python library for working with 3D point clouds.
http://pyntcloud.readthedocs.io
MIT License
1.39k stars 221 forks source link

Bug in convert_color_to_dtype() in read_las() #331

Closed SBCV closed 9 months ago

SBCV commented 2 years ago

Describe the bug When running PyntCloud.from_file(las_file_path) I receive a point cloud with black color values. But it works when running laspy directly. This bug is caused by https://github.com/daavoo/pyntcloud/blob/4d7928c09a81a59be308ed4bd10b3431dc812127/pyntcloud/io/las.py#L39-L40

The reason is that laspy 2.0 and pylas use color values between 0 and 256, but represent them with uint16. Therefore, the division by 256 leads to 0 values.

I used some of the laspy examples (such as simple.las) for testing. I do not know, if there are las / laz files, that actually use 2^16 values for colors (in which case the division by 256 would make sense).

Any suggestions how to address this issue? If wanted I can provide a PR for this.

To Reproduce Steps to reproduce the behavior:

  1. Download https://github.com/laspy/laspy/blob/master/tests/data/simple.las
  2. Debug the call of pyntcloud_result = PyntCloud.from_file(las_file_path)
  3. Examine values color values before and after data["points"].loc[:, column_names] /= 256
SBCV commented 2 years ago

According to one of the author of laspy:

I checked the specification, the lines

NOTE: Red, Green, Blue values should always be normalized to
16 bit values. For example, when encoding an 8 bit per channel
pixel, multiply each channel value by 256 prior to storage in these
fields. This normalization allows color values from different camera
bit depths to be accurately merged.

was added in 1.3, maybe we can find a way to properly and cleanly enforce that ins laspy 🤔

So I guess, the most reasonable approach would be (like he proposed) to adjust the values according to their distribution (max value). What do you think?

daavoo commented 2 years ago

So I guess, the most reasonable approach would be (like he proposed) to adjust the values according to their distribution (max value). What do you think?

Sounds reasonable. Thanks for looking into this 🙏

SBCV commented 2 years ago

Want me to create a PR? Or are you fixing this yourself?

daavoo commented 2 years ago

Want me to create a PR? Or are you fixing this yourself?

If you can create PR would be great. I don't have the time right now for it