-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Gaussian Splat SPZ Support #12582
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
base: main
Are you sure you want to change the base?
Gaussian Splat SPZ Support #12582
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
I'm taking an additional pass on code and testing now. Just to make sure everyone is on the same page, this is my current understanding of the plan for this PR. @keyboardspecialist @weegeekps, please correct me if I misunderstood anything.
|
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 @keyboardspecialist!
I took a pass through the source code and did some initial testing. While everything appears to be working well, I think I need clarity on two things related to the code:
- I think I'm missing some context around having both a "model" code path and a separate "primitive" code path– Would you mind filling me in? Is the plan to support both? If so why?
- One of the lesson we learned from Voxels was that a separate API for specific data types rather than the existing
Cesium3DTileset
API was probably not the right move (see Voxel API should conform to Cesium3DTileset interface #12297). While we don't need to reply on a model implementation if it's not needed, ideally splats should still go through theCesium3DTileset
API. What's preventing that at the moment?
The rest of my comments mostly revolve around architecture, inline documentation, and code cleanup.
- I did not deeply review
GaussianSplatPrimitive.js
yet. I think I'm missing some context around the above points. - This is running well for me in Sandcastle. But occasionally, I do see a red flash on startup. Is that expected?
window.startup = async function (Cesium) { | ||
"use strict"; | ||
//Sandcastle_Begin | ||
Cesium.Ion.defaultAccessToken = |
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.
Typically, we add any Sandcastle assets to the CesiumJS
ion account. That way, the default token will work to access them, and there's no need to manually set the token.
Reach out if you need access or any help getting assets uploaded to that ion account.
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.
@ggetz, is it alright if we hold off until we've settled on exactly what asset we're going to use as the demo asset before uploading to the CesiumJS account? It could be this tower, or it could be something else. Of course, we won't merge to main
without having the sandcastle asset in the correct place.
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.
Yep 👍
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 answer your questions:
- The model path was the old pathway. We definitely don't want to support both. This code has since been stripped out from this branch. That implementation highlighted the need for our new approach.
- This was simply a misunderstanding on my part. Nothing is preventing it. It does rely on Cesium3DTileset events for handling tile loading. It's wrapping the load calls from Cesium3DTileset which it shouldn't do. I do see rectifying this as high priority for this release as we don't want to incur unnecessary tech debt as we continue to add features and any bug fixes.
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.
- Got it. Thanks for the cleanup!
- Cool, that would definitely be the best move from the user's workflow perspective as well. Happy to help talk through how to best handle this if needed. When you say this release, we expect that to be the June 6th-ish timeframe, correct?
{ | ||
maximumScreenSpaceError: 1, | ||
}, |
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.
Is setting this to 1
necessary for everything to work right now?
It's pretty atypical to set maximumScreenSpaceError
to something other than the default, let alone as low as 1
.
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.
Yes, but we are working on fixing this in the tiler, and it will be fixed before launch.
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.
What's the plan with both of these Sandcastles? Is the plan to ship them both, or only the SPZ version?
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.
SPZ will be the only shipped sandcastle. This one will be removed in another PR.
RED_INTEGER: WebGLConstants.RED_INTEGER, | ||
RG_INTEGER: WebGLConstants.RG_INTEGER, | ||
RGB_INTEGER: WebGLConstants.RGB_INTEGER, | ||
RGBA_INTEGER: WebGLConstants.RGBA_INTEGER, |
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 all need their own JSDoc comments.
|
||
highp vec4 discardVec = vec4(0.0, 0.0, 2.0, 1.0); | ||
|
||
void gaussianSplatStage(ProcessedAttributes attributes, inout vec4 positionClip) { |
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 entry point (I think this function) should be documented.
fixed typo causing cache error
I think it should happen in this PR so that old code isn't going into main and it's easier to review. |
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 @keyboardspecialist! I took a pass on GaussianSplatPrimitive
. My biggest concern (beyond making Cesium3dTileset
the top-level entry point in the API) is making sure we account for potential race conditions as pat of the asynchronous sorting operations.
|
||
frameState.afterRender.push(() => true); | ||
|
||
if (!defined(loader) || this._resourcesLoaded) { |
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.
if loader
is not defined, then the following two lines will throw an error. Was that check supposed to be sooner in this function?
this._tile.destroy(); | ||
this._tileset.destroy(); |
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 don't think this should destroy the tile or the tileset, as these could still be referenced somewhere else.
}; | ||
|
||
GaussianSplat3DTilesContent.prototype.destroy = function () { | ||
this._gsplatData = undefined; |
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.
On the other hand, GaussianSplatPrimitive.js
does have resources that will need to be destroyed. It'll need to be explicitly destroyed here by calling gsplatData.destroy()
.
}); | ||
|
||
GaussianSplatPrimitive.prototype.onTileLoaded = function (tile) { | ||
console.log(`Tile loaded: ${tile._contentResource.url}`); |
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.
As part of cleanup, make sure all debug logs are removed.
/**@type {boolean} | ||
@private | ||
*/ |
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.
/**@type {boolean} | |
@private | |
*/ | |
/** | |
* @type {boolean} | |
* @private | |
*/ |
As part of cleanup, make sure comments are formatted correctly.
}); | ||
promise.then((sortedData) => { | ||
this._indexes = sortedData; | ||
GaussianSplatPrimitive.buildGSplatDrawCommand(this, frameState); |
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.
Since this happens when the promise is resolved, this very likely will be called in a different frame then when the sorting process was kicked off. Is that all accounted for?
Keep in mind things can occur such as an older call could potentially resolve after a more recent sort resolves, meaning the new command will be overwritten by old data.
is3DTileContent: { | ||
get: function () { | ||
return true; | ||
}, | ||
}, |
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.
Is this used anywhere?
isTilesetContent: { | ||
get: function () { | ||
return true; | ||
}, | ||
}, |
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.
Is this used anywhere?
return GaussianSplatTextureGenerator._textureTaskProcessor; | ||
}; | ||
|
||
GaussianSplatTextureGenerator.generateFromAttrs = function (parameters) { |
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.
GaussianSplatTextureGenerator.generateFromAttrs = function (parameters) { | |
GaussianSplatTextureGenerator.generateFromAttributes = function (parameters) { |
tileset._selectedTiles.forEach((tile) => { | ||
const gsplatData = tile.content._gsplatData; | ||
this.pushSplats({ | ||
positions: new Float32Array( | ||
ModelUtility.getAttributeBySemantic( | ||
gsplatData, | ||
VertexAttributeSemantic.POSITION, | ||
).typedArray, | ||
), | ||
scales: new Float32Array( | ||
ModelUtility.getAttributeBySemantic( | ||
gsplatData, | ||
VertexAttributeSemantic.SCALE, | ||
).typedArray, | ||
), | ||
rotations: new Float32Array( | ||
ModelUtility.getAttributeBySemantic( | ||
gsplatData, | ||
VertexAttributeSemantic.ROTATION, | ||
).typedArray, | ||
), | ||
colors: new Uint8Array( | ||
ModelUtility.getAttributeByName(gsplatData, "COLOR_0").typedArray, | ||
), | ||
}); | ||
|
||
this._numSplats += gsplatData.attributes[0].count; | ||
}); | ||
|
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.
Consider batching these operation together.
tileset._selectedTiles.forEach((tile) => { | |
const gsplatData = tile.content._gsplatData; | |
this.pushSplats({ | |
positions: new Float32Array( | |
ModelUtility.getAttributeBySemantic( | |
gsplatData, | |
VertexAttributeSemantic.POSITION, | |
).typedArray, | |
), | |
scales: new Float32Array( | |
ModelUtility.getAttributeBySemantic( | |
gsplatData, | |
VertexAttributeSemantic.SCALE, | |
).typedArray, | |
), | |
rotations: new Float32Array( | |
ModelUtility.getAttributeBySemantic( | |
gsplatData, | |
VertexAttributeSemantic.ROTATION, | |
).typedArray, | |
), | |
colors: new Uint8Array( | |
ModelUtility.getAttributeByName(gsplatData, "COLOR_0").typedArray, | |
), | |
}); | |
this._numSplats += gsplatData.attributes[0].count; | |
}); | |
const tiles = tileset._selectedTiles; | |
const totalElements = tiles.reduce((total, tile) => tile.content.pointsLength, 0); | |
const aggregateAttributeValues(componentDatatype, getAttributeCallback) => { | |
const aggregate = ComponentDatatype.createTypedArray(componentDatatype, totalElements); | |
let index = 0; | |
for (const tile of tiles) { | |
const primitive = tile.content._gsplatData; | |
const attribute = getAttributeCallback(primitive); | |
aggregate.set(attribute.typedArray, index); | |
index = aggregate.length; | |
} | |
return aggregate; | |
}; | |
this._positions = aggregateAttributeValues(ComponentDatatype.FLOAT, (primitive) => ModelUtility.getAttributeBySemantic( | |
primitive, | |
VertexAttributeSemantic.POSITION, | |
)); | |
// etc. | |
this._numSplats = totalElements; |
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 with SPZ compression using a proposed draft extension KhronosGroup/glTF#2490
Another PR will remove old Model pipeline code
Affected files
Known Issues
View matrix projection is currently incorrect. Splats are rendered in wrong orientation in relation to the camera.FixedIssue number and link
Testing plan
Will need to gather some single and multi-tile glTF SPZ assets to incorporate into the general testing suite.
Updated sandcastle, possibly a new asset with more rendering controls.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change