-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Splat shader #12364
base: main
Are you sure you want to change the base?
Splat shader #12364
Conversation
interesting scaling decomposed covariance
fragment shader impl
need to get instanced drawarrays working
…loading. Setup for instanced rendering of quads
Attempting to fix splat scaling
new uniforms for camera data, no more computing in vertex shader renamed splat stage define
some clean up
correctly uses viewProjection when sorting
prop cleanup
shader tweaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this fixed up after the prettier updates @keyboardspecialist! It really helped with the diff.
- I notice there are several failing unit tests in CI.
- I didn't see any unit tests for the new functionality or files. These will need to be added before the PR can be merged. We typically aim for code coverage of ~90% or more for new code.
@@ -0,0 +1,84 @@ | |||
<div align="center"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm my understanding, all of temp_wasm/
is intended to go away once CesiumGS/cesium-wasm-utils#1 is merged and published, correct?
If so, I think we'll want to add a TODO item for
- Configuration to use
@cesium/wasm-utils
once published
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I added the TODO item to the PR description.
@@ -52,7 +52,8 @@ | |||
], | |||
"dependencies": { | |||
"@cesium/engine": "^13.1.0", | |||
"@cesium/widgets": "^10.1.0" | |||
"@cesium/widgets": "^10.1.0", | |||
"@cesium/wasm-splats": "file:./temp_wasm/pkg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root dependencies here should only include the packages in this repo.
The @cesium/wasm-splats
dependency should be moved to @cesium/engine/package.json
, since that's the code which depends on the wasm module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this when I move to using the published package.
gulpfile.js
Outdated
@@ -373,6 +373,11 @@ export async function prepare() { | |||
"Tools/jsdoc/cesium_template/static/styles/prism.css", | |||
); | |||
|
|||
copyFileSync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nit-picky, but can we move this line up with the other source code dependencies up around line 351? It will help it not get lost among the other dev processes like building documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. I moved it up.
scripts/build.js
Outdated
@@ -1133,6 +1137,7 @@ export async function buildCesium(options) { | |||
outbase: "packages/widgets/Source", | |||
}); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These appear to be unneeded whitespace changes. Do you anticipate needing to update this file? If not, can we checkout the version in main
to avoid the noise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded. I merged the version from main.
/> | ||
<meta | ||
name="description" | ||
content="Use Viewer to start building new applications or easily embed Cesium into existing applications." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description should be updated to a summary of the example. Any key words here will also be helpful, as they will help this example show up when a user searches.
Something along the lines of "Load a gaussian splat dataset of a cell tower as a 3D tileset." would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to a relevant description
"Load 3D Tiles 1.1 Gaussian Splats dataset of a cell tower"
|
||
gl_PointSize *= show; | ||
#ifdef HAS_CUSTOM_VERTEX_SHADER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually indent nested compiler directives for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation was lost at some point, and I don't know why. Re-added.
@@ -0,0 +1,34 @@ | |||
import createTaskProcessorWorker from "./createTaskProcessorWorker.js"; | |||
//import defaultValue from "../Core/defaultValue.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -0,0 +1,54 @@ | |||
import createTaskProcessorWorker from "./createTaskProcessorWorker.js"; | |||
//import defaultValue from "../Core/defaultValue.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
return radix_sort_gaussians_indexes( | ||
primitive.positions, | ||
primitive.modelView, | ||
2048, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does 2048 come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was picked as a maximally compatible texture width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Perhaps this should be defined as a constant somewhere then to make this a bit more explanatory.
@@ -0,0 +1,187 @@ | |||
#ifndef HAS_SPLAT_TEXTURE | |||
|
|||
void calcCov3D(vec3 scale, vec4 rot, out float[6] cov3D) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilleyse Should probably be the one to do a pass on the shaders.
updates geometry pipeline stage to use modelutility to find attributes
…g as it's only used on the instanced quad Simplified updateGaussianSplatting in Model.js, camera check caused more issues than it solved. Index sorting is fast enough, and resolves some edge cases where fast rotations with lots of tiles would orphan some and leave them unsorted
Thanks for all the updates here @keyboardspecialist! What is the current status of this PR and where can we help with reviews?
|
Description
Opening as a draft as some things are still in progress, and this is a long running feature branch.
This adds support for rendering Gaussian splats from 3D Tiles 1.1 using a proposed draft extension here
description in progress
PixelFormat, PixelDatatype - Support added for integer textures with pixel formats R32UI/R32I, RG32UI/RG32I, RGB32UI/RGB32I.
GaussianSplatPipelineStage - This is the old pipeline. May be removed in the future, but doesn't rely on any web assembly.
GaussianSplatTexturePipelineStage - New pipeline. Renders from an attribute texture. Uses web assembly for texture generation and sorting the indexes for retrieving attributes from the texture. Texture format is RGBAUI.
Both use PrimitiveLoadPlan to prep their attributes. The only pipeline simply uses this to keep attributes in CPU memory, so they can sorted later. The new pipeline generates it's texture here, and inserts an index attribute to read from that texture.
GeometryPipelineStage - This is where we determine if we are actually rendering splats. Since we use a point primitive in the glTF we can switch between them at runtime. If disabled, we follow the point cloud pipeline as usual.
Model.js - Sorting updates occur here. We check for camera movement then sort and rebuild the command list.
Model, Cesium3DTileStyle - Styling for scale and toggling splats/point rendering
TODO
@cesium/wasm-utils
once publishedIssue number and link
Testing plan
TBD. Will include a sandcastle.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change