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

Add NGA_gpm validation #319

Merged
merged 26 commits into from
Nov 21, 2024
Merged

Add NGA_gpm validation #319

merged 26 commits into from
Nov 21, 2024

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Nov 9, 2024

This builds upon the state from #316 - that PR also provides some general context.

This adds support for the validation of the NGA_gpm extension, which is the 3D Tiles extension for GPM (corresponding to NGA_gpm_local, which is the glTF extension).

I'm opening this as a draft for now:

  • The other PR should be merged first
  • There are some open questions about the GPM specification/schema that fell out of the implementation here, and that are still being discussed internally
  • There are no specs here yet

The last point is strongly related to #226 : I locally do have the necessary specs. But creating them was an odd mix of semi-automation and lots of (lots of) manual work, tweaking the files to be "broken" in one way or another, carefully matching the different ways of being "broken" with the spec and the schema. It still has to be decided whether another approach should be taken here.

A draft PR that shows the specs and the corresponding files is in #320


(Unrelated: I snuck in the (EDIT: specs for the) validation of the 3DTILES_bounding_volume_S2 extension here. The files for that already did exist. But the actual ...Specs.ts had been missing. This was only a minor fix that went along with the minor restructuring of the specs directory that I did for this PR)

@javagl
Copy link
Contributor Author

javagl commented Nov 16, 2024

Merged in the specs that had been shown as a preview in #320

Added validation of aspects that are not covered by the JSON schema itself:

  • Validate that unitVector has unit length (epsilon=0.00001)
  • Validate that lsrAxisUnitVectors form an orthonormal basis (dot product epsilon=0.0005)
  • Validate that referenceDateTime is a valid ISO8601 string
  • Validate that the lengths of "upper triangular matrices" are in fact triangular numbers
  • Validate that the ppeManifest[i].source values are unique

Added validation of aspects that are currently not covered by the JSON schema. This refers to aspects that are not covered by the current ("preview") version of the JSON schema, but - as confirmed via internal discussion - should become part of the schema, and therefore also be validated:

  • The referenceSystem may not contain an epoch when it has a definition
  • The modelCoordSystem should not contain properties that it does not need, depending on its mcsType
  • The anchorPointMetadata.contentIndex should not be negative
  • In NGA_gpm_local:
    • The covarianceDirectUpperTriangle should have at least a length of 1
    • The anchorPointsIndirect and anchorPointsDirect should have a length of at least 1
    • The covarUpperTriangle should have a length of at most 6 (i.e. it should have a length of exactly 6)

@javagl javagl marked this pull request as ready for review November 21, 2024 14:45
Base automatically changed from gpm-support to main November 21, 2024 14:47
@javagl
Copy link
Contributor Author

javagl commented Nov 21, 2024

As mentioned above, one aspect of the validation is

Validate that lsrAxisUnitVectors form an orthonormal basis (dot product epsilon=0.0005)

We now encountered data sets that are likely supposed to be valid, and where these dot products had been in the range of 0.0088.... So we preliminarily changed that epsilon to 0.01. This sounds like a lot. It may have to be adjusted in the future.

@lilleyse lilleyse merged commit c0d4f5a into main Nov 21, 2024
2 checks passed
@lilleyse lilleyse deleted the gpm-support-3d-tiles branch November 21, 2024 19:00
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

Successfully merging this pull request may close these issues.

2 participants