Skip to content

feat: remove broken assertions, add color test#30

Open
m-clare wants to merge 2 commits into
dancergraham:mainfrom
m-clare:maryanne/feat-color-value-test
Open

feat: remove broken assertions, add color test#30
m-clare wants to merge 2 commits into
dancergraham:mainfrom
m-clare:maryanne/feat-color-value-test

Conversation

@m-clare

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

Copy link
Copy Markdown
Contributor
  • Added a test for checking a few point color values.

@dancergraham

dancergraham commented Oct 27, 2024

Copy link
Copy Markdown
Owner

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
Copy Markdown
Owner

pumpsA.txt

@dancergraham

Copy link
Copy Markdown
Owner

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

@dancergraham

Copy link
Copy Markdown
Owner

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

@dancergraham

Copy link
Copy Markdown
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