GPM 1.2 support #12204
GPM 1.2 support #12204
Conversation
|
Thank you for the pull request, @javagl! ✅ We can confirm we have a CLA on file for you. |
|
The last commits consisted of
What is still missing: Mentioning this in CHANGES.md. We should emphasize that this is a "preview feature", offered for early feedback, and some details may still change in future releases. On the side of the user, the access to the data that is parsed from the root-level GPM extension should now be simple. Given
The returned object will be a For the GPM data that is stored in the mesh primitives (similar to property textures), there currently is no way of obtaining that data explicitly in its original form: In order to be usable in CesiumJS (e.g. for custom shaders), the data is internally converted into a Some details of this conversion are (intentionally) not really "specified". There are some caveats with that. For example, when a glTF contains both the The following is a Sandcastle that is extracted from an existing demo, and focusses on the aspects of the GPM support that are implemented here:
(The sandcastle refers to |
There was a problem hiding this comment.
Thanks @javagl! We talked with @lilleyse offline– He mentioned that he took a look at the overall approach here and that it looks good 👍
My comments are much more about the API and how we can communicate the (experimental) change to users.
Please merge in main when you can. Please see @jjspace's instructions to streamline merging the recent prettier updates.
The following is a Sandcastle that is extracted from an existing demo...
Can we commit this to the repo for reference? I think it can live under the "Development" Sandcastle examples so that it does not get published to the cesium.com deployment of Sandcastle.
What is still missing: Mentioning this in CHANGES.md. We should emphasize that this is a "preview feature", offered for early feedback, and some details may still change in future releases.
I see that most of the new code here is not exposed in the public API. So, if I was adding to CHANGES.md, mention the added support in Model, and then add the caveat "This feature is not final and is subject to change without Cesium's standard deprecation policy."
Additionally, we have a comment in Model.js that says "Cesium supports glTF assets with the following extensions:" Can we add this extension to the list, with the same caveat?
We can talk about the naming. There's a class like Spdcf. It could be called StrictlyPositiveDefiniteCorrelationFunction. If someone thinks that it should be renamed: Please suggest sensible names for its properties A and T here as well...
I know it's verbose, but I don't think "SPDCF" is common enough that the abbreviation should be used, as per the coding guide. I'm open to a compromise as of one or two descriptive words in there.
This is definitely from the perspective of someone who is trying to understand via the code first instead of being familiar with the extension– However I expect this will be most people's experience while debugging. At the very minimum, can we link to spec where a developer can find more info?
And I think that it could make sense to have an infrastructure (or at least some ideas) about how to handle ~"highly use-case specific (vendor) glTF extensions" in general. This could go as far as establishing some "plugin-concept" that allows plugging in loaders for specific extensions into the GltfLoader, and transporting the parsed and loaded data back to the client.
I think this a good idea. Given the timeline on this PR and the desire to constrain the scope, I would say we should move forwards with the approach in this PR for now. But I think "loader plugin" concept would be great for extensibility in the future. Does it make sense to open an issue and discuss further?
| * @private | ||
| * @experimental This feature is subject to change without Cesium's standard deprecation policy. |
There was a problem hiding this comment.
While there is an argument to be made for this being a private function, I'm curious as to why this is marked as experimental. It does not seem to be coupled to the GPM extension at all.
There was a problem hiding this comment.
Right now, model.getExtension(...) can only refer to an extension object from the root of the glTF. The GPM extension also contains extension objects in the mesh primitives. And it is by no means clear how ~"elements of a glTF" should/could be made accessible (even less their extensions).
But I can remove that part of the comment. @private might be enough.
| * this will return the model representation of the extension. | ||
| * | ||
| * @param {string} extensionName The name of the extension | ||
| * @returns The object, or `undefined` |
There was a problem hiding this comment.
| * @returns The object, or `undefined` | |
| * @returns {object|undefined} The object, or `undefined` |
There was a problem hiding this comment.
Also the object here should always be an instance of a ResourceLoader, correct?
There was a problem hiding this comment.
The object that is returned there is of the type that the object has after parsing it from the extension JSON. In this case, it will have the type GltfGpmLocal.
| * @private | ||
| * @experimental This feature is not final and is subject to change without Cesium's standard deprecation policy. | ||
| */ | ||
| function PpeTexture(options) { |
There was a problem hiding this comment.
Please document the options object.
|
|
||
| /** | ||
| * Create the JSON description of a metadata class that treats | ||
| * the given PPE texture as a property texture property(!). |
There was a problem hiding this comment.
| * the given PPE texture as a property texture property(!). | |
| * the given PPE texture as a property texture property. |
Love the enthusiasm though 😄
packages/engine/Source/Scene/Model/Extensions/Gpm/GltfMeshPrimitiveGpmLoader.js
Show resolved
Hide resolved
| }, | ||
| }); | ||
|
|
||
| GltfMeshPrimitiveGpmLoader.prototype.loadResources = async function () { |
There was a problem hiding this comment.
For clarity, I might suggest marking functions used internally only with an underscore, ie. GltfMeshPrimitiveGpmLoader.prototype._loadResources.
There was a problem hiding this comment.
I now added some underscores there. On a certain level, this should not be necessary, because the whole class is tagged as @private, meaning that everything within this class already is private. And given that things like
private and do not have an undescore, I assume that "internally" basically means "in this file" here.
(This can make sense, as a concept, but ... consistency is difficult. And I know that many of these difficulties can be attributed to the underlying "programming "language""...)
The last commit before Prettier v3 was used to reformat the code
This commit reformats all code using prettier v3 relative to the pre-prettier-v3 tag
|
I addressed some (most) of the inlined comments. There are a few open ones, related to
The task to create demos is already on my TODO list. Depending on the intended structure, this could be separate demos for this PR and the "Metadata Picking", or it could be one that replaces https://demos.cesium.com/owt-gpm/ , which combines both features. (Is it criticial that this sandcastle becomes part of this PR?)
I added the comment Regarding
Right now, most of the classes are tagged with both. This does not make much sense. But considering that clients should be able to obtain these structures, via that Unless you disagree, I'd remove the (I'll do this together with the remaining JSDoc fixes) About the renaming: As mentioned in the first post: I can rename |
Nope. But I do think having one or both committed to the repo eventually (rather than deployed separately to demos.cesium.com) will be useful. I'm thinking that we want to have it in place to ensure that other changes do not break this feature, beyond just unit tests.
Overall, I was actually thinking the other way around: It does make sense that most of the new code is private API, since users of CesiumJS will only ever use it by loading a model that has the new extension. Therefore, no need to note the But yes, anywhere a client can access a class via the API, it should not be marked as
I see why consistency between the spec and the implementation may be preferred. Perhaps if we provide a link to the spec for context in the inline documentation that would be sufficient. |
|
Just a short note about the
I'll try to classify that sensibly in the next update pass (tomorrow). But the structures/classes in the will return an |
|
The previous commits addressed most of the remaining inlined comments. Two broader questions (and how I addressed them): The
|
# Conflicts: # packages/engine/Source/Scene/DerivedCommand.js
| @@ -44,23 +44,23 @@ | |||
| }); | |||
|
|
|||
| const fragmentShaderSource = ` | |||
| uniform sampler2D colorTexture; | |||
There was a problem hiding this comment.
We updated some options in the prettier merge instructions to prevent unwanted trailing spaces like this from showing up.
There was a problem hiding this comment.
It's not immediately clear how this could be fixed "after the fact". I'd think that running npm run prettier on the current state and committing all the changed should fix "everything" (and that the current CI shouldn't even pass when there was still something wrong). But I can have a closer look (and maybe try to better understand what's actually done in the merge instructions - for now, I just followed them "blindly"...)
There was a problem hiding this comment.
OK. To clean up this particular PR for now, I would recommend checking out the previous unchanged files (Apps/Sandcastle/*, packages/engine/Source/Scene/ArcGisMapServerImageryProvider.js, etc) from main and committing to this branch.
CC @jjspace
There was a problem hiding this comment.
I think that it didn't work when pulling them from 'main', but with
git checkout post-prettier-v3 -- "Apps/Sandcastle/gallery/Custom Post Process.html"
(for all relevant files), it seemed to work - at least, the files do no longer show up as 'modified' here.
|
Thanks @javagl! I think this should be good to go. Please see my comment about documenting a potential follow-up: https://github.com/CesiumGS/cesium/pull/12204/files#r1781500043 |
|
I added the comment that you linked to, and the other open points (from the metadata picking PR) in a follow-up issue at #12225 - I'll scan the reviews again to see whether I overlooked something, and extend that issue as necessary. |
Summary
The document "The Generic Point-cloud Model (GPM): Implementation and Exploitation, Version 1.2, 2024-02-29" from the National Geospatial-Intelligence Agency (NGA) is published at https://nsgreg.nga.mil/csmwg.jsp .
And I'll now try to summarize a 264 page document in one paragraph 😎 :
The "GPM" is the "Generic Point-cloud Model", and describes methods for modeling error propagation (or "uncertainty") within point clouds that have been acquired e.g. with LIDAR scanners, for the use in a geospatial context. In GPM Version 1.2, these concepts have been generalized to be applicable to meshes. The idea is to store error metadata inside a glTF, using a glTF extension. The main elements of the data can very roughly be summarized to be "anchor points", which are a sort of reference points with a known location and known error/uncertainty, and "per-point errors" (PPE), which, in the case of mesh data, are stored in a texture. The anchor points are stored at the top-level of the glTF. The per-point error textures are associated with mesh primitives, similar to Property Textures in
EXT_structural_metadata.Description
The current (DRAFT) state of this PR:
Source/Scene/Model/Extensions/Gpmsubfolder.Cartesian3instead of anumber[3]array for somepositionand such). We can argue about the 1:1 mapping of the JSON schema structures. The result may be "inconvenient" in some way. But trying to find a good "convenience layer" requires a deep understanding of the goals and usage patterns.Spdcf. It could be calledStrictlyPositiveDefiniteCorrelationFunction. If someone thinks that it should be renamed: Please suggest sensible names for its propertiesAandThere as well...GltfLoaderGltfGpmLoaderandGltfMeshPrimitiveGpmLoader, that are hooked into theGltfLoaderGltfGpmLoaderloads the top-level GPM information, and offers it as the classes described aboveGltfMeshPrimitiveGpmLoaderloads the GPM information for each mesh primitive. This involves loading the PPE (per-point error) textures. Right now, this data is translated into instances for "structural metadata", likeEXT_structural_metdata, but there are many details to be sorted out here...Some high-level considerations:
The extension is probably not something that could be considered as "the most widely used core functionality of CesiumJS". And I think that it could make sense to have an infrastructure (or at least some ideas) about how to handle ~"highly use-case specific (vendor) glTF extensions" in general. This could go as far as establishing some "plugin-concept" that allows plugging in loaders for specific extensions into the
GltfLoader, and transporting the parsed and loaded data back to the client.Right now, for the GPM data, this is done ... very pragmatically. The
Model/Extensions/Gpmsubdirectory already suggests that there might be other directories inModel/Extensions/...that contain the respective extension classes. The loaders are currently hard-wired into theGltfLoader, but it might be relatively easy to find some sort of abstraction there. The question about how to transport the extension information to the client is still open.The apprach here right now is:
ModelComponents.extensionsdictionary that can store "anything"GltfGpmLoaderloads the GPM JSON, translates it into an object, and this is eventually just stored asloader._components.extensions["NGA_gpm_local"] = gpmLoader._gltfGpmLocal;Model3DTileContent.getExtensionthat just forwards toModel.getExtension, and allows clients to obtain that parsed data likeconst extensionObject = modelContent.getExtension("NGA_gpm_local")(the point is: There should probably not be some
Model.getNgaGpmLocalfunction in theModelclass...)Details about the point that the 'mesh primitive GPM data' is currently translated into something that resembles
EXT_structural_metadatawill be added here soon. But there are too many details to be sorted out (and that are likely to change) to explain that right now.Testing plan
Beyond the unit tests, I have added a sandcastle for testing this in #12204 (comment)
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change