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

.pnts loader for ModelExperimental #9978

Merged
merged 50 commits into from
Jan 6, 2022
Merged

.pnts loader for ModelExperimental #9978

merged 50 commits into from
Jan 6, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Dec 14, 2021

Fixes #9730

Similar to #9968, this PR adds support for loading point clouds with ModelExperimental, each part of #9836

This one was more involved, as pnts does not contain a glTF. Instead, the PntsLoader generates a ModelComponents object (our new runtime data structure for representing a 3D model) with a POINTS primitive.

This PR Does NOT cover the following. These will be future PRs:

  • Eye dome lighting
  • Attenuation

This PR is most of the way there, but the following things need to be updated:

  • Fix existing unit tests for PointCloud3DTIleContent
  • Fix existing unit tests for TimeDynamicPointCloud
  • Handle oct-encoded normals in PntsLoader
  • decode RGB565 colors on the CPU in PntsLoader
  • Add more unit tests for PntsLoader
  • Add unit tests for PntsParser
  • I just came across this issue: PointCloud.js should not call CesiumMath.setRandomNumberSeed #9730, this impacts this PR too so I should look into it.
  • Investigate bounding spheres that aren't in the right spot

@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ptrgags
Copy link
Contributor Author

ptrgags commented Dec 15, 2021

Most of the way done, though still need to figure out why the time dynamic point cloud tests are failing (it renders, but then after the camera.moveRight(10) call it still renders... that shouldn't be happening.)

Oh right, I remember at one point turning on the bounding sphere draw commands and one model was showing up incorrectly. I'll add that to the checklist above.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 4, 2022

I talked with @lilleyse about the sRGB handling a bit so now I have a better understanding of what the goal here is. To summarize:

  • .pnts files are assumed to store sRGB colors. So in the material stage, always convert from sRGB to linear space when computing material.diffuse.
  • when scene.highDynamicRange is true, no tonemapping or sRGB conversion should be applied since the frame buffer will store linear float values.
  • when scene.highDynamicRange is false, tonemapping is applied (only for PBR) to reduce the range (still linear), followed by linear->sRGB conversion at the end.

Other notes:

  • We should rename some of the builtin shader functions to be lowercase, e.g. HSBToRGB -> hsbToRgb to be consistent with our style guide.
  • Adding LinearToSrgb and srgbToLinear functions would also make sense in the same PR. These functions should have no #ifdefs, but the calling code should.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 5, 2022

Fixed the sRGB handling, but I'm noticing a few aditional things after playing with an updated Sandcastle

  • Translucency is not enabled for a constant color with alpha
  • If the model has RGBA colors the model is not translucent
  • DracoBatched is not rendering
  • NoColor - before it defaults to dark gray, here I'm defaulting to white. I should change it to grey
  • oct encoded normals don't seem to be used in the PBR shader, I see the default
  • quantized models seem to be jittering (is something using world coordinates by mistake?)
  • styling - I had issues with the style that uses the temperature property (per-point properties)

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 5, 2022

For DracoBatched, haven't fully investigated why it wasn't rendering but a warning in the console gives a hint: it says "vertex fetch requires 1000, but attribs only supply 500" - sounds like a mismatch of data types when uploading.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 6, 2022

Still investigating the crash when I try to apply a style. So far what I see happening:

  • draw commands are built on the first frame
  • the style is applied
  • draw commands are rebuilt
  • but somehow the attribute vertexBuffer was already destroyed so an error is thrown

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 6, 2022

I figured out the crash - I overlooked that the vertex attribute buffers need to be marked as "not destroyable" so they don't get double destroyed when the style changes.

Now I'm noticing:

  • looks like I have a bit more work to make styleable properties work correctly, even the PointCloud3DTileContent version isn't rendering correctly (should be red and blue). Probably a line or two missing in PntsParser.
    image

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 6, 2022

Turns out it wasn't that, there was a syntax error in the style that was being swallowed

Updated Sandcastle

PointCloudBatched now renders correctly (the difference in color is due to PBR vs Lambert shading)
image

Per-point properties will not work, but that falls under #9944.

@lilleyse at this point this PR is ready for review again!

@lilleyse
Copy link
Contributor

lilleyse commented Jan 6, 2022

Thanks @ptrgags. I tested HDR on/off and spot checked some other sandcastles. glTF point clouds work fine as well.

This pull request was closed.
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.

PointCloud.js should not call CesiumMath.setRandomNumberSeed
3 participants