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

feat: remove broken assertions, add color test #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

m-clare
Copy link
Contributor

@m-clare m-clare commented Oct 27, 2024

  • Added a test for checking a few point color values.

@dancergraham
Copy link
Owner

dancergraham commented Oct 27, 2024

Thanks for this - On this one I wonder whether we should follow the same approach as the recent Intensity value fix, disabling color normalisation.

When I export these color values into a text file (X, Y, Z, R, G, B, Intensity attached) using cloud compare I get Integer values. I think it makes sense for this library that the color values read back out should be as close as possible to the original written values or written using a common format and not normalised from 0 to 1 based on the range of values present in this pointcloud.

-0.32225147 -4.96385098 1.69413996 70 79 50 0.451430
-0.32057047 -4.93772888 1.69172692 65 74 45 0.473617
-0.31987011 -4.92671251 1.69447327 65 74 47 0.483413
-0.31996414 -4.92777967 1.70134044 68 75 44 0.492416
... 

@dancergraham
Copy link
Owner

pumpsA.txt

@dancergraham
Copy link
Owner

I think simply calling .normalise_color(false) after the normalise_intensity call should fix this behaviour

@dancergraham
Copy link
Owner

This would again be a breaking change bu I think it's the right choice for this lib

@dancergraham
Copy link
Owner

I think Actions should now run automatically on future PRs as you have already contributed !

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

Successfully merging this pull request may close these issues.

2 participants