Extract Geometry from 3D Models#13330
Conversation
|
Thank you for the pull request, @alarkbentley! ✅ We can confirm we have a CLA on file for you. |
|
I know this is just a draft PR but - before you spend too much (more) time on this - is there an github issue this stems from? I haven't evaluated the usefulness of this feature, but it's good to discuss new features before so we don't waste your time if it's not a good fit for Cesium. |
|
Until alarkbentley chimes in: This looks like it is supposed to become sort of a building block of #13244 In general, the functionality to "access geometry data" is required at many, many places. And it is implemented at many, many places, which is not ideal. Some places that come to my mind are extracting the vertex positions for picking, handling "outline rendering", or extracting the vertex positions for draping. However, the specific context here appears to aim at the possiblity to extract "something like a clipping polygon" from a tileset. To be confirmed. |
|
@mzschwartz5 As @javagl mention this is a building block for #13244. I decided to split this up into two separate PRs to keep the complexity down. Also this capability itself can have some value outside of generating footprints in the case the user want's to do calculations based on the tile geometry. If we decide to not expose this in the public API in the end that is fine, we could keep it as an internal utility. But I do think it could provide end user value. I will try to complete this by the end of week and add a proper PR description for you to review. |
Yes!!! I already mentioned #13052 , outlining some of the use cases that are currently prevented by the bufffer-vs-typedArray handling, and which boil down to that pretty fundamental task of "accessing geometry data". Given that this is a draft, and it's not yet clear where exactly this is going, it may be too early (and maybe not even my "job") to provide first feedback here. But I've already been pondering with ~"some of this", so maybe it's OK: I had a short look at There are There is However... even if it's once more a shameless self-promotion: Iff anything that is done in the (One thing that the |
|
@javagl I'm currently working on merging the capabilities of ModelMeshUtility and ModelReader (see my commit a few minutes ago). My initial mistake was to base my implementation on the pickModel (which I assume was written before we had the ModelReader). Would be nice to port pickModel to use the ModelReader... but I'm not sure I want to open that bucket right now. Current state is very much a draft still, but I appreciate the early feedback. |
It's not necessary to call this a "mistake". I certainly took ... inspiration... from (If there is a "mistake", then it is that
I certainly don't want to "urge" you to do that in any way (if it causes too much effort or difficulties). But the goal of the
One important point for "aligning" things and avoiding redundancies: I've seen that some of your implementations checked for 'WebGL2'. This was related to reading data from the GPU to the CPU, and that whole buffer-vs-typedArray topic. As far as I had seen in earlier PRs, you had some ~ Even if this check for "WebGL2" is necessary: The methods received the I think that the name of a class should reflect the purpose and usage of that class. So Yup, that's 64 unrelated, complex, largely undocumented properties, and only one of them is the The point is: The methods did not need a (And as I mentioned above: I hope that they don't even need that. If they need "something like that", we could try to find a better solution...) |
|
I'm not familiar enough to have an opinion on most of the above, but — especially in a performance-oriented API, it might be ideal to return |
|
Most of the And again: It certainly has to be improved and extended, to be that universal tool that I would have liked it to be some years ago, and imagined it to be in a (hopefully not so distant) future. About avoiding allocations: Some functions already accept an optional
Right now, there's a bit of a mix of types and requirements. At some places, "things" (tuples) are stored or expected as The different There already are conversion functions, back and forth, with the respective One thing - of which I'm aware that it can be 'dangerous'! - is ... what you might call a "flyweight pattern". For example, when something just needs a bunch of All these things are design considerations that one could think about, deeply and thoroughly (even if it started on a 'green field'). Retrofitting generalizations or abstractions to existing structures can be even more challenging. |
javagl
left a comment
javagl
left a comment
There was a problem hiding this comment.
There are some inlined comments. Some of them may become obsolete, depending on whether/how the 'High-level' comments are addressed.
High-level:
Despite the fact that the current implementation is synchronous the introduced API endpoint is exposed with an async handle to enable future non-breaking improvements such as WebWorkers or async GPU readback.
Big +1 for that. The only way to make a future-proof API is to make everything async. This is obscure and inconvenient, but the harsh truth for JavaScript.
I think that the current API is very constrained. That's not necessarily a bad thing. The API should do exactly what it is supposed to do, and not be over-generalized to accidentally offer half-baked functionalities. But in its current form, the API can hardly evolve. Adding new extractions is hardly possible without considerable changes of the structures.
Extracted properties:
The GeometryResult currently has the positions and colors. It is very reasonable to assume that people will want to extract more - mainly, the normals. (A very recent case: https://community.cesium.com/t/how-to-align-glb-models-to-a-gltf-tileset-surface-picking-height-and-surface-normals/45632 - someone wanted to pick something, and align a model to the picked point, based on its surface normal).
There are some options that one could consider. One could add a normals property to that output. And one could consider adding a texCoords property to that output. Eventually, it could be very desirable to have the possibility to access arbitrary EXT_structural_metadata property attributes.
At this point, one could consider a return type like this
class GeometryResult {
attributeNames: string[]; // contains strings like `POSITION` and `NORMAL` or `_MY_PROPERTY`
attributeValues: Map<string, Array>; // Contains the values - see notes below!
attributeTypes: Map<string, ...>; // See notes below
}
Of course, the attribute values would be untyped. They could contain Cartesian3[] or Color[] or Matrix2[]. In the spirit of the API design mantra that "simple things should be easy, and complex things should be possible", one could consider typed convenience functions for the common attributes:
class GeometryResult {
...
getPositions() : Cartesian3[] | undefined {
const a = this.attributeValues("POSITION");
if (defined(a)) { return undefined; }
return a as Cartesian3[];
}
}
For other attributes, the relevant information could be stored in the attributeTypes, which would include the type (like VEC3) and componentType (like UINT16) in one form or the other.
The limitation of the return type is pretty much tied to the options and the extractPositions and extractColors flags. A pragmatic generalization would be to offer some
options.extractedProperties = ["POSITION", "_MY_PROPERTY" ];
to handle that. (The caller can not know beforehand which properties are there, but that's a different topic - the caller doesn't know about the presence of the colors either...)
Extracted structure:
I think that I do understand the intention behind grouping the extracted geometry by the feature ID. But I don't understand the intention behind doing that by default. I'll have to check some details, but think that it should be relatively easy to make this an optional, second step. Very roughly (somewhat brainstorming):
The extractAttributesFromPrimitive receives the primitive, the feature ID label, and the Map to store the grouped results. It should be fairly easy to split this up into two functions:
extractAttributesFromPrimitive(which, I think, could actually return aGeometryResult!)partitionGeometryResult
The latter could receive a GeometryResult and the feature ID data, and return a Map<number, GeometryResult> that contains the clustered/grouped geometry. This would allow differentiating between the case where the user wants the geometry of one feature, and the case where the user just wants the whole geometry of the picked object. The latter would be useful for the cases where the input data is not even clustered by a feature ID (e.g. point clouds without batch tables).
Tests
I tested the new Sandcastle, and it works nicely, as intended.
Loading a non-batched point cloud
const tileset = await Cesium.Cesium3DTileset.fromUrl(
"../../SampleData/Cesium3DTiles/PointCloud/PointCloudRGB/tileset.json",
);
omitting the feature ID handling
//if (!(pickedFeature instanceof Cesium.Cesium3DTileFeature)) {
// return;
//}
// Highlight the selected feature
//selectedFeature = pickedFeature;
//selectedFeature.color = highlightColor;
throws
Error
at new DeveloperError (http://localhost:8081/packages/engine/Build/Unminified/index.js:7460:11)
at Check.defined (http://localhost:8081/packages/engine/Build/Unminified/index.js:7496:11)
at AttributeCompression.dequantize (http://localhost:8081/packages/engine/Build/Unminified/index.js:29414:17)
at _ModelReader.readAttributeAsTypedArray (http://localhost:8081/packages/engine/Build/Unminified/index.js:105275:59)
at extractAttributesFromPrimitive (http://localhost:8081/packages/engine/Build/Unminified/index.js:125155:39)
at http://localhost:8081/packages/engine/Build/Unminified/index.js:125084:7
at _ModelReader.forEachPrimitive (http://localhost:8081/packages/engine/Build/Unminified/index.js:105947:9)
at ModelGeometryExtractor.getGeometryForModel (http://localhost:8081/packages/engine/Build/Unminified/index.js:125080:23)
at Model3DTileContent.getGeometry (http://localhost:8081/packages/engine/Build/Unminified/index.js:125352:41)
at <anonymous>:73:37
This may not be an issue of this PR. It may be caused by the fact that the PntsLoader does not assign the count property to attributes, except for the "POSITION" attribute, as of
I think that it could and should assign this property, iff it can be verified that this count is always present and correct.
After pragmatically inserting the proper count values:
For a point cloud, there are no indices, and that will crash
ModelReader.js:684 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'typedArray')
at _ModelReader.readIndicesAsTypedArray (ModelReader.js:684:46)
at extractAttributesFromPrimitive (ModelGeometryExtractor.js:217:33)
at ModelGeometryExtractor.js:76:7
at _ModelReader.forEachPrimitive (ModelReader.js:898:9)
at ModelGeometryExtractor.getGeometryForModel (ModelGeometryExtractor.js:72:15)
at Model3DTileContent.getGeometry (Model3DTileContent.js:233:33)
at <anonymous>:86:37
at cancelMouseEvent (ScreenSpaceEventHandler.js:290:9)
at handleMouseUp (ScreenSpaceEventHandler.js:312:5)
at handlePointerUp (ScreenSpaceEventHandler.js:863:5)
When loading composite data...
const tileset = await Cesium.Cesium3DTileset.fromUrl(
"../../SampleData/Cesium3DTiles/Composite/Composite/tileset.json",
);
then vertices will be shown only when clicking the lower left box (not for other boxes). I haven't looked at how to resolve this. Maybe what is currently done with the "feature ID" will have to be done with some "instance ID/index" instead. This may become obsolete, depending on possible changes of the feature ID handling.
When loading vector data...
const viewer = new Cesium.Viewer("cesiumContainer", {
terrain: Cesium.Terrain.fromWorldTerrain(),
});
const geocoder = viewer.geocoder.viewModel;
geocoder.searchText = "Vienna, Austria";
geocoder.flightDuration = 0.0;
geocoder.search();
const tileset = await Cesium.Cesium3DTileset.fromIonAssetId(5737);
the clicking will cause
Uncaught (in promise) TypeError: content.getGeometry is not a function
at <anonymous>:86:37
at cancelMouseEvent (ScreenSpaceEventHandler.js:290:9)
at handleMouseUp (ScreenSpaceEventHandler.js:312:5)
at handlePointerUp (ScreenSpaceEventHandler.js:863:5)
at HTMLCanvasElement.listener (ScreenSpaceEventHandler.js:53:5)
This could be avoided with some instanceof Model3DTileContent check at the client side, but that's probably not intended or desired.
I think whatever was supposed to be accomplished with the DeveloperError.throwInstantiationError() simply does not work, and that all ""implementations"" of the Cesium3DTileContent ""interface"" will have to explicitly implement this function (making the whole ""interface"" obsolete after all...)
| /** | ||
| * Groups unique vertex indices by feature ID from indices or vertex count. | ||
| * This is shared across all attribute extraction to avoid duplicate work. | ||
| * @private |
There was a problem hiding this comment.
Note: Some specific aspects of this may become obsolete, depending on whether or how the API is changed for non-featureId-cases.
This should include proper documentation of the cases that the input parameters are undefined (as JDoc text, or part of @param JSDocs).
...
If the indices are undefined, then nothing will be done, so you wouldn't have to call this method in the first place.
If the featureIdData is undefined, then the map will map the value `0` to the set of all indices, in a very inefficient way.
@param {TypedArray|undefined} indices The indices
@param {TypedArray|undefined} featureIdData The feature ID data
The "very inefficient" part refers to the fact that it is checking for defined(featureIdData) for each index. This does not change. There could be some
if (!defined(featureIdData) {
map.set(0, [...indices]);
return map;
}
watchdog at the beginning. (Again: depending on how relevant 'featureIdData' will be in the future!)
| const extractColors = options.extractColors ?? false; | ||
| const result = new Map(); | ||
|
|
||
| if (!model._ready) { |
There was a problem hiding this comment.
This is "fail silent". Users will receive an empty result and have no chance of figuring out why. It should at least be VERY boldly documented, or print a warning. Things like Model.boundingSphere even go so far to throw
if (!this._ready) {
throw new DeveloperError(
"The model is not loaded. Use Model.readyEvent or wait for Model.ready to be true.",
);
}
But I would hesitate to recommend that. It's quirky, and the concept of "ready" as a whole doesn't make sense. (Some more RAII-like approach would be nice, but ... here we are, related to #13052 ...)
| // Positions need per-instance-transform duplication | ||
| if (defined(posData)) { | ||
| for (let t = 0; t < instanceTransforms.length; t++) { | ||
| readPosition(posData, vertexIndex, numPosComponents, scratchPosition); |
There was a problem hiding this comment.
I think that this readPosition can be pulled out of the for loop, if the multiplyByPoint below is changed to receive a dedicated result, as in
... originalScratchPosition, transformedScratchPosition
so that the original position only has to be read once from the position data (and the same position is then transformed with all transforms)
| * | ||
| * @private | ||
| */ | ||
| function computeNodeTransforms(runtimeNode, sceneGraph, model, result) { |
There was a problem hiding this comment.
Just a cross-link: This is basically the same of what I did in https://github.com/CesiumGS/cesium/pull/11830/changes#diff-c25ab54b614865d4ba17f0fbc27717f5cbaa07931f69deb0b83b7d0daa1d3505R235
(Unfortunately, that PR had to be abandoned, and the "revival" at #12658 probably counts as "abandoned" as well in the meantime. The model picking is still highly inefficient. Maybe we can "revive the revival" after this PR is merged, given that it now does contain building blocks like this computeNodeTransforms, which should have existed right from the beginning...)
So a 👍 for that.
| const instances = node.instances; | ||
|
|
||
| if (defined(instances)) { | ||
| const transformsCount = instances.attributes[0].count; |
There was a problem hiding this comment.
The ModelReader was only a stub when I created it, and will require further iterations to be "complete". Specifically: It currently still carries a comment
NOTE: All this does not properly handle MATn types.
And I think that these 'transforms' that are extracted here could/should eventually be handled with the readAttributeAsTypedArray as well, when (not 'if', but 'when') it supports MAT4 attributes.
| offset, | ||
| elementStride, | ||
| quantization, | ||
| readPosition(vertices, i0, numPosComponents, scratchV0); |
There was a problem hiding this comment.
Note: I have not (yet) thoroughly reviewed or explicitly tested the changed in pickModel.js. The diff-view here on GitHub is scattered. I looked at the resulting code, ad it looks much simpler and cleaner than before. So it should be possible to review (only) the new state of the code with reasonable effort.
But there are some aspects of pickModel that are the reason why I'd like to spend a bit more time for a review here. These aspects are related to #12658 . It looks like this refactoring was essentially done here as well, but I'd like to check the details. Specifically: The handling of the 'vertical exaggeration' is a bit tricky, and I'll have to explicitly and dedicatedly review and test this.
In the meantime and for now:
I think that the three readPosition calls can be pulled out of the loop, storing the results in some scratchVX_input, and then change the calls below to
`multiplyByPoint(..., scratchVX_input, scratchVX_output)
so that the positions only have to be read once, and are then transformed with all matrices.
|
It was brought to my attention that I'm working on something somewhat similar, here. You can read in that PR's description what the overall goal is there, but that specific link is to a general interface I'm introducing to access geometry data. The idea would then be to implement version of that interface for various geometry-containing classes ( Note: my PR is still in draft. That interface I'm building will have more methods added to it beyond just getting/setting vertices. E.g. I could see it containing general methods for accessing generic vertex attributes. |
This reverts commit 81e6398.
alarkbentley
changed the title
|
@javagl Could you take another round at this when you get some time to spare? I did some major refactoring based on your previous comments which all made sense. Summary of the updates: ModelReader
pickModel
ModelGeometryExtractor
GeometryResult
Cesium3DTileContent
|
@mzschwartz5 Thanks for bringing this to my attention. This work certainly has some potential overlap. |
4 participants
Add
getGeometryAPI for extracting vertex data from ModelsSummary
Adds a new public
Model.prototype.getGeometry()method that extracts per-primitive vertex attributes (positions, normals, colors, feature IDs, etc.) from a loaded Model. This is backed by a newModelGeometryExtractorutility, a generalizedGeometryResultclass, and significant refactoring ofModelReaderto consolidate scattered vertex-reading logic into a single, reusable module.Despite the fact that the current implementation is synchronous the introduced API endpoint is exposed with an async handle to enable future non-breaking improvements such as WebWorkers or async GPU readback.
Motivation
There was no public API to access the underlying vertex information of 3D Models in Cesium. Use cases such as geometric analysis, custom visualizations, and spatial queries on picked features required workarounds or access to private internals.
Key driver for this was the ongoing PoC to extract 2D footprints based on model geometry in runtime: #13244
Usage example
Screenshot
Changes
New API
Model.prototype.getGeometry(options)— Public async method on theModelclass. Returns aPromise<GeometryResult[]>with one entry per primitive.options.featureIdLabel— Label of the feature ID set to match (default"featureId_0").options.attributes— Array of semantic strings to extract (e.g."POSITION","NORMAL","COLOR_0","_FEATURE_ID","TEXCOORD_1"). Defaults to["POSITION"].options.extractIndices— Whether to extract vertex indices (defaultfalse).Cesium.GeometryResult— Generalized result class holding extracted per-primitive data. Properties:attributeNames,attributeValues(Map),attributeTypes(Map),indices,primitiveType,count,instances. Convenience methods:getPositions(),getNormals(),getColors(),getFeatureIds(),getAttributeValues(name),getAttributeType(name).New modules
ModelGeometryExtractor— Extracts requested vertex attributes from a loadedModel. Performs a single scene-graph traversal with per-instance transform support, and reusesModelReaderfor typed-array access and GPU readback.Bug fixes
ModelReader—octDecodebug fix — Fixed incorrect argument order inAttributeCompression.octDecodeInRangecall (was passing aCartesian3, now correctly passesc.x, c.y) and fixed swapped arguments in the subsequentCartesian3.packcall. (previously in pickModel) LinkgetInstanceTransformsbug fix — Fixed incorrect matrix multiplication order for meshes with EXT_mesh_gpu_instancing. (previously in pickModel) LinkRefactoring
ModelReader—readAttributeAsTypedArraynow supports in-memorytypedArrayfast path — returns the attribute's existing typed array directly when available, skipping GPU readback entirely. Previously only handled GPU buffer reads.forEachPrimitive(model, options, callback)— New static method that traverses all runtime nodes and primitives in a model's scene graph. Accepts an optionaloptionsobject withmapProjection(for 2D projection) andperInstanceFeatureIds(boolean, defaultfalse, to control whether per-instance feature IDs are fetched). Computes per-node transforms, then invokes the callback with(runtimePrimitive, primitive, instances, computedModelMatrix).pickModel— Significantly simplified (~295 lines removed, ~86 added) by delegating node traversal and vertex unpacking toModelReaderinstead of inlining its own scene-graph walk and vertex-reading logic.Sandcastle demo
3d-tiles-geometry-extraction-devdemo showing how to pick a feature, extract its geometry, and visualize extracted vertex positions as point entities.Tests
GeometryResultSpec— 205 lines, 19 tests covering initialization defaults, all convenience getters (getPositions,getNormals,getColors,getFeatureIds), fallback key resolution (e.g.COLORvsCOLOR_0), genericgetAttributeValues/getAttributeType, and property storage.ModelGeometryExtractorSpec— ~991 lines, 24 tests covering positions extraction, feature ID extraction (by label, positionalLabel), attribute filtering, index extraction, node transform application, instance transform duplication (positions, colors, feature IDs), per-vertex vs per-instance feature ID precedence, multi-set color extraction (COLOR_0/COLOR_1), normal extraction, primitiveType propagation, and edge cases.ModelReaderSpec(forEachPrimitive block) — ~512 lines, 19+ tests covering scene-graph traversal, transform computation (MAT4 typed array, MAT4 buffer, VEC3 attributes, interleaved buffers, translation+rotation+scale), 2D projection, instance feature ID fetching with opt-inperInstanceFeatureIds, and edge cases.ModelSpec— 4 new tests forgetGeometrycovering default behavior, options passthrough, and undefined options handling.Testing plan
3d-tiles-geometry-extraction-dev) against a batched 3D Tiles tileset.3d-tiles-picking-devthat is utilizing the refactoredpickModelstill is working.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my changeAI acknowledgment
If yes, I used the following Tools(s) and/or Service(s):
GitHub Copilot
If yes, I used the following Model(s):
Claude 4.6 Opus