Skip to content
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

Follow-up actions for GPM support and metadata picking #12225

Open
javagl opened this issue Sep 30, 2024 · 0 comments
Open

Follow-up actions for GPM support and metadata picking #12225

javagl opened this issue Sep 30, 2024 · 0 comments

Comments

@javagl
Copy link
Contributor

javagl commented Sep 30, 2024

Basic experimental support for the NGA_gpm_local extension that has been defined in GPM 1.2 has been added in PR 12204. Strongly related to this was the (also experimental) functionality of picking metadata values from property textures, which was implemented in PR 12075. The latter implements the metadata picking functionality generically, based on the existing, built-in handling of metadata that was originally implemented as a support for EXT_structural_metadata.

There are a few follow-up actions for these PRs, and for further support of GPM. Some of them have been brought up as review comments, but could not be addressed before the initial release.

Most of them refer to the metadata picking:

  • Some performance issues have been reported in the main review comment for the metadata picking. Although I could not immediately reproduce them, they might be related to a late change in the PR that might affect the shader caching. A first step for investigating this would be to verify that it does not re-build the shaders each time when one of the picking functions is called
    • Based an a first test/investigation, it looks like this is unrelated to the pickMetadata function itself, but rather caused by the Chrome DevTools - some details in this comment
  • The approach for the metadata picking, summarized in one sentence: It render the objects with a shader that mimics a 'custom shader' that renders the metadata values, and reads the resulting metadata values from the frame buffer. This does have quite a few drawbacks and limitations (some of them summarized in a PR comment - it mainly boils down to the limitation that result from the frame buffer being limited to 4 bytes). However, the way that it is currently implemented also raised another concern in a review comment - namely, that skipping certain shader stages could cause unexpected results when certain parts of the models are transparent. A shallow summary of the question could be: When something is rendered transparently, should it still be possible to pick metadata values from that object? There might not be a one-fits-all solution for that, and the options for solving this have to be thought through carefully.
  • The picking functions need information about the property that is supposed to be picked. The property is only uniquely identified by the schemaId, the className, and the propertyName. However, in many cases, users won't care about (and maybe not even know) the schema.id. Therefore, the schemaId can be undefined in these functions. This was brought up in a review comment and raises some questions, because it seemed to violate the "optionals last" convention in CesiumJS. However, the schemaId parameter is not an optional parameter in this case. It is still required, with undefined serving as a "wildcard" for ignoring the schema ID. If this is a problem, different solutions could be considered. Buf every potential solution here should be carefully examined for compatbility with potential solutions for the issue of Qualified names for metadata access in custom shaders #10085
  • Some of the functionality that was required for the metadata picking can be summarized as "translating data from a raw binary buffer into properly typed metadata values". As pointed out in a review comment, much of this functionality is pretty generic, and thus, may not have to reside in the MetadataPicking class, but could be moved into a more generic class. (In fact, much of the functionality overlaps with what is currently done in MetadataTableProperty - namely, translating binary data into metadata values. The main difference is that the values come from the binary metadata in one case, and from a frame buffer in the other case). Some of the functionality, as it is implemented in MetadataTableProperty, could not immediately be re-used, because it was implemented as instance functions, and because it had to handle certain forms of "legacy" data. But in a follow-up pass, we should examine both classes and see whether common code parts can be generalized and moved into a place where they can be used for both purposes.
  • Right now, the metadata picking focussed on property textures. The basic approach should in theory also be applicable to other metadata sources (like property attributes). There, some furter limitations and considerations come into play: Property textures already are limited (e.g. to UINT8 types). Property attributes could be anything, and only a tiny part of that can sensibly be funneled through the 4 bytes of a frame buffer. But it could be worthwhile to just try it out, and see (and document) where the limitations are.
    • Update: The question of which types can be transported though the frame buffer (and how) has to be analyzed more holistically. Until now, picking property attributes is disabled by only looking for property texture properties (done in 1a7dbb6 ).

Points that refer to metadata picking and its connection to GPM:

  • The NGA_gpm_local extension defines "Per-Point-Error Textures" (PPE textures). These are structurally very similar to property textures in EXT_structural_metadata. So right now, these parts of the NGA_gpm_local extension are in fact translated into a StructuralMetadata instance internally. (This way, the whole existing metadata functionality, like picking and custom shaders, can be applied to it - otherwise, the whole, low-level, internal metadata handling of CesiumJS would have to be made aware of the NGA_gpm_local extension and its PPE textures, which would be 99% equal to property textures - leading to lots of code duplication and lots of potential for suble errors...). The resulting limitation of that (as mentioned in a review comment) is that when a model contains EXT_structural_metadata and NGA_gpm_local PPE textures, then the latter wil overwrite the EXT_structural_metdata. Options for handling this have been considered (like "merging the schemas"), but all "simple" solutions would quickly lead to cases with pretty unexpected and ambiguous behavior.

Points that are specific to the GPM support:

  • The GPM specification defines two extensions: NGA_gpm_local, which is a glTF extension, and NGA_gpm, which is a 3D Tiles extension. Currently, only NGA_gpm_local is supported. This already raises some questions about Generalized support for external- and vendor extensions #12223 .
  • An implementation of the NGA_gpm (3D Tiles) extension should be added, once the general approach for supporting external/vendor extensions has been decided upon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant