Extract Vector glTF tile data into BufferPrimitiveCollection#13271
Extract Vector glTF tile data into BufferPrimitiveCollection#13271danielzhong merged 46 commits intomainfrom
Conversation
…ollection-render-gl' into DanielZhong/VectorTiles
|
Thank you for the pull request, @danielzhong! ✅ We can confirm we have a CLA on file for you. |
donmccurdy
left a comment
There was a problem hiding this comment.
@danielzhong Great progress here, I like how this is coming together! I left a few initial comments, mainly since we're writing the code and the spec in parallel just trying to make sure things align.
| const values = new Float64Array(indices.length * 3); | ||
| for (let i = 0; i < indices.length; i++) { | ||
| const vertexIndex = indices[i]; | ||
| const srcOffset = vertexIndex * 3; | ||
| values[i * 3] = positions[srcOffset]; | ||
| values[i * 3 + 1] = positions[srcOffset + 1]; | ||
| values[i * 3 + 2] = positions[srcOffset + 2]; | ||
| } | ||
|
|
||
| polylineCollection.add({ positions: values }, polylineView); | ||
| setPrimitiveFeatureId(polylineView, featureIdSource, indices[0]); |
There was a problem hiding this comment.
I'm hoping we'll be able to upload a range of the position array directly to the collection, but I guess the collection needs to allow more than float64array here? Opened:
| if (primitiveType === PrimitiveType.TRIANGLE_STRIP) { | ||
| triangleIndices = | ||
| ModelReader.convertTriangleStripToTriangleIndices(indices); | ||
| } else if (primitiveType === PrimitiveType.TRIANGLE_FAN) { | ||
| triangleIndices = ModelReader.convertTriangleFanToTriangleIndices(indices); | ||
| } |
There was a problem hiding this comment.
I think it's OK just to error out if the primitive uses the vector extension with TRIANGLE_STRIP or TRIANGLE_FAN topology, since we won't be able to identify the original polygon connectivity in those cases anyway.
| polygonCollection.add( | ||
| { | ||
| positions: positions, | ||
| triangles: toUint32Array(triangleIndices), |
There was a problem hiding this comment.
See above, with #13277 it should be OK to use whatever the source array type is.
| polygonView, | ||
| ) { | ||
| const primitiveType = primitive.primitiveType; | ||
| const positions = readTransformedPositions(primitive, nodeTransform); |
There was a problem hiding this comment.
Do we need to transform the vertex positions? I'm hoping that we can apply the node transform to collection.modelMatrix or something like that, but not 100% sure how this all fits with the rest of the loading process!
| if (PrimitiveType.isLines(primitiveType) && defined(polylineCollection)) { | ||
| if (primitiveType === PrimitiveType.LINES) { | ||
| for (let i = 0; i + 1 < indices.length; i += 2) { | ||
| appendPolylinePrimitive( | ||
| polylineCollection, | ||
| polylineView, | ||
| featureIdSource, | ||
| positions, | ||
| [indices[i], indices[i + 1]], | ||
| ); | ||
| } | ||
| } else if (primitiveType === PrimitiveType.LINE_STRIP) { | ||
| appendPolylinePrimitive( | ||
| polylineCollection, | ||
| polylineView, | ||
| featureIdSource, | ||
| positions, | ||
| indices, | ||
| ); | ||
| } else if (primitiveType === PrimitiveType.LINE_LOOP) { | ||
| const loopIndices = new Uint32Array(indices.length + 1); | ||
| for (let i = 0; i < indices.length; i++) { | ||
| loopIndices[i] = indices[i]; | ||
| } | ||
| loopIndices[indices.length] = indices[0]; | ||
| appendPolylinePrimitive( | ||
| polylineCollection, | ||
| polylineView, | ||
| featureIdSource, | ||
| positions, | ||
| loopIndices, | ||
| ); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Here I think it's OK if we throw an error if we see any primitive types not allowed by the vector extension.
packages/engine/Source/Scene/Model/createVectorTileBuffersFromModelComponents.js
Show resolved
Hide resolved
…ollection-typedarray' into DanielZhong/VectorTiles # Conflicts: # packages/engine/Source/Scene/BufferPolygonCollection.js
…to DanielZhong/VectorTiles
…to DanielZhong/VectorTiles
…ollection-render-gl' into DanielZhong/VectorTiles
…ollection-render-gl' into DanielZhong/VectorTiles
donmccurdy
left a comment
There was a problem hiding this comment.
Awesome work, thanks Daniel! Just a few comments, no major concerns on my side.
I'd slightly prefer fewer defensive checks still, but that's easier to do with the // @ts-check suggestion which might be out of scope here, so not a big deal for now. Tests are working well locally.
Heads up that in earlier version of the sample tilesets I generated, the "extensionsUsed" property wasn't declared properly and the tilesets could go down the existing glTF rendering path rather than using the new vector code. I've fixed the tilesets: polylines and polygons are working as-is, there's a small fix needed for points I think (see comments).
Approving now since the comments are mostly optional stuff, the point primitive fix is the only critical thing.
| const url = multipleContents._innerContentResources[index].url; | ||
| const message = defined(error.message) ? error.message : error.toString(); | ||
| if (tileset.tileFailed.numberOfListeners > 0) { | ||
| if (defined(tileset.tileFailed) && tileset.tileFailed.numberOfListeners > 0) { |
There was a problem hiding this comment.
Do you recall why this change was needed? I'm not seeing errors if I remove it currently, maybe it can be removed?
If related to my sample tilesets ... those were just thrown together with a quick script, so mostly I'm wanting to check that we're not adding workarounds for what might have been invalid data on my part. :)
There was a problem hiding this comment.
Entirely optional suggestion, feel very free to skip this — For new files you might find it helpful to include a // @ts-check comment at the top of the file. If you're using an editor with TypeScript Language Services enabled (like VSCode or Zed) you should start to see better autocomplete, and errors where type information is missing or conflicting. Any errors visible in the editor will also be reported by npm run tsc.
For example if #13313 happens to merge before this PR, npm run tsc would start complaining about calls to point.setColor(...) (which has been removed).
We could certainly do this in a future PR too.
| this._decodeModel = this._decodeModel && this._decodeModel.destroy(); | ||
| this._pointCollections = undefined; | ||
| this._polylineCollections = undefined; | ||
| this._polygonCollections = undefined; | ||
| this._vectorBuffers = undefined; |
There was a problem hiding this comment.
It would be best to destroy the collections before we dereference them:
for (const collection of this._pointCollections) {
collection.destroy();
}
this._pointCollections = undefined;| if (points.length === 0 && polylines.length === 0 && polygons.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return { | ||
| points: points.length > 0 ? points : undefined, | ||
| polylines: polylines.length > 0 ? polylines : undefined, | ||
| polygons: polygons.length > 0 ? polygons : undefined, | ||
| }; |
There was a problem hiding this comment.
Optional - I think if we allowed empty arrays here, and made the arrays non-nullable, we'd need fewer if/defined protections elsewhere in the PR. But that sort of change is easier to do safely with the // @ts-check mode on, which might be out of scope for this PR, so feel free to leave this as-is too. :)
| if (points.length === 0 && polylines.length === 0 && polygons.length === 0) { | |
| return undefined; | |
| } | |
| return { | |
| points: points.length > 0 ? points : undefined, | |
| polylines: polylines.length > 0 ? polylines : undefined, | |
| polygons: polygons.length > 0 ? polygons : undefined, | |
| }; | |
| return { points, polylines, polygons }; |
| continue; | ||
| } | ||
|
|
||
| collection._vectorLocalModelMatrix = Matrix4.clone(nodeTransform); |
There was a problem hiding this comment.
Hm, I was hoping it might be possible to avoid putting extra data onto the collection, but I don't see an obvious way right now. Not sure if that should be the responsibility of the Cesium3DTiles classes or if the BufferPrimitiveCollection needs more metadata. Ok with me as-is though, no changes needed for now unless you have more thoughts on it.
packages/engine/Source/Scene/Model/createVectorTileBuffersFromModelComponents.js
Outdated
Show resolved
Hide resolved
| function getFeatureId(featureIdSource, vertexIndex) { | ||
| if (!defined(featureIdSource)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| let featureId; | ||
| if (defined(featureIdSource.values)) { | ||
| if (vertexIndex < 0 || vertexIndex >= featureIdSource.values.length) { | ||
| return undefined; | ||
| } | ||
| featureId = featureIdSource.values[vertexIndex]; | ||
| } else { | ||
| featureId = | ||
| Math.floor(vertexIndex / featureIdSource.repeat) + featureIdSource.offset; | ||
| } | ||
|
|
||
| featureId = Math.trunc(featureId); | ||
| if ( | ||
| defined(featureIdSource.nullFeatureId) && | ||
| featureId === featureIdSource.nullFeatureId | ||
| ) { | ||
| return undefined; | ||
| } | ||
| return featureId; | ||
| } |
There was a problem hiding this comment.
It seems like we're re-implementing a bit from packages/engine/Source/Scene/Model/FeatureIdPipelineStage.js here and in getFeatureIdSource above. That's probably OK, I don't see an easier way to access its results, maybe it's just worth including a TODO for later.
packages/engine/Source/Scene/Model/createVectorTileBuffersFromModelComponents.js
Outdated
Show resolved
Hide resolved
lilleyse
left a comment
There was a problem hiding this comment.
I did a quick pass, not going super deep. The approach here looks good.
Description
This PR adds support for rendering glTF-based vector tile content using the current
CESIUM_mesh_vectorspec.The runtime decodes the glTF, extracts vector data into
BufferPrimitiveCollectiontypes, and renders them through the existing buffer collection renderer path.Supported vector primitive handling in this PR
BufferPointCollectionBufferPolylineCollectionBufferPolygonCollectionCESIUM_mesh_vector.polygonAttributeOffsets,polygonIndicesOffsets, and optional hole metadata.High-level flow
Cesium3DTilesetloads a.gltf/.glbtile.Cesium3DTileContentFactory, glTF content is routed toVectorGltf3DTileContentwhentileset.isGltfExtensionUsed("CESIUM_mesh_vector")istrue.VectorGltf3DTileContent.fromGltf()callsModel.fromGltfAsync()to decode the asset without rendering the model directly.update(), once the decoded model is ready,initializeVectorPrimitives()callscreateVectorTileBuffersFromModelComponents().createVectorTileBuffersFromModelComponents()converts glTF primitives into buffer collections:BufferPointCollection[]BufferPolylineCollection[]BufferPolygonCollection[]collection.modelMatrixduring update.VectorGltf3DTileContent.update()updates each collection and callscollection.update(frameState).renderBufferPointCollectionrenderBufferPolylineCollectionrenderBufferPolygonCollectionIssue number and link
Testing plan
Author checklist
CONTRIBUTORS.mdI have updatedCHANGES.mdwith a short summary of my change