Skip to content

EXT_feature_metadata JSON schema cleanup#6

Closed
kring wants to merge 30 commits intoCesiumGS:3d-tiles-nextfrom
kring:metadata-schema-cleanup
Closed

EXT_feature_metadata JSON schema cleanup#6
kring wants to merge 30 commits intoCesiumGS:3d-tiles-nextfrom
kring:metadata-schema-cleanup

Conversation

@kring
Copy link
Member

@kring kring commented May 7, 2021

Fixes some small things I noticed while adding this extension to cesium-native:

  • Specify that when the min/max/etc properties are arrays they must be arrays of numbers, not arrays of just anything.
  • Remove possibly invalid (and redundant anyway) not.required property.

@kring
Copy link
Member Author

kring commented May 7, 2021

By the way, are we sure we want specify min/max/etc to be "type": ["number", "array"]? glTF accessors have min/max properties that are very similar, and in that case they're always arrays. If the accessor is a scalar, it's simply an array of one element. This is much easier to deal with in statically typed languages.

Comment on lines 39 to 43
"not": {
"required": [
"schema",
"schemaUri"
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to ensure that schema and schemaUri are mutually exclusive and was copied from camera.schema.json in the glTF spec:

https://github.com/KhronosGroup/glTF/blob/c6a5521ebd1469aba076949a97f150bf3591b63f/specification/2.0/schema/camera.schema.json#L36-L38

I wonder if there's a better way to do this.

Comment on lines 82 to 88
"type": [
"number",
"array"
]
],
"items": {
"type": "number"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type should "integer" in both places here. I just pushed a fix related to that b3f48c2

@lilleyse
Copy link
Collaborator

By the way, are we sure we want specify min/max/etc to be "type": ["number", "array"]? glTF accessors have min/max properties that are very similar, and in that case they're always arrays. If the accessor is a scalar, it's simply an array of one element. This is much easier to deal with in statically typed languages.

I've debated this too. I'm not a huge fan of single element arrays since scalar properties tend to be way more common in metadata and the array form adds clutter/confusion IMO. I'd rather see the min/max value match the type exactly. Also there is better symmetry with the "default" property which doesn't use array form for single elements either.

I think glTF gets away with it because scalar accessors are not all that common. I understand why it's done though.

@lilleyse
Copy link
Collaborator

@kring thanks for the fixes and sorry for such a late review on this, I missed have must it when you posted it. Looks good I just had a couple comments.

@lilleyse
Copy link
Collaborator

Merged these changes in #10

@lilleyse lilleyse closed this Aug 30, 2021
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.

3 participants

Comments