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

Additions to Point Cloud spec #733

Merged
merged 3 commits into from
Feb 7, 2023
Merged

Additions to Point Cloud spec #733

merged 3 commits into from
Feb 7, 2023

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Feb 3, 2023

The Point Cloud guide doesn't mention anything about its colors being stored in sRGB color space. This leads to unexpected rendering differences when migrating from pnts to glTF, because a glTF stores colors in linear RGB.

To minimize future confusion, I added some lines about point clouds storing colors in sRGB, and how they must be converted to linear RGB for glTFs.

I also added a line requiring POINTS_LENGTH to be greater than zero, as suggested in #711 (comment).

@javagl could you review? Feel free to rewrite my lines if they are not clear enough.

@javagl
Copy link
Contributor

javagl commented Feb 4, 2023

Looks fine for me. I'll read a little bit more about the context (and what e.g. CesiumJS is doing with the vertex colors), and probably merge soon.

Interstingly, a quick search for srgb does not bring up any mention in this repo. And websearching for 3d+tiles+"srgb" brings up a fairly recent presentation with the lines

Point clouds

Vertex color sRGB (?)

so I guess that "(?)" may become a "(!)" soon...)

Copy link
Contributor

@javagl javagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked a little bit at the handling of the color spaces in CesiumJS, and it seems like there is a dedicated step for converting the sRGB colors from point clouds into the linear color space. One could zoom pretty deeply into that, and ask questions about the 'Styling' section of the specification, but that may be unrelated to this PR.

@javagl javagl merged commit 254ca23 into draft-1.1 Feb 7, 2023
@j9liu j9liu deleted the specify-srgb-in-pnts branch February 7, 2023 17:06
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