CesiumGS / 3d-tiles

Specification for streaming massive heterogeneous 3D geospatial datasets :earth_americas:
2.04k stars 459 forks source link

Cleanup pass for 1.1 - JSON schema #677

Closed javagl closed 2 years ago

javagl commented 2 years ago

Addressing some further points from https://github.com/CesiumGS/3d-tiles/issues/651 .

This mainly refers to the JSON schema, so it could make sense to have this as one dedicated PR.


Use .gltf or .glb everywhere in examples

The 1.1 documentation does not seem to refer to the legacy tile formats any more. The only mentions are in "Next" (i.e. the 1.0 extensions)

Flesh out 3D Tiles Next schema descriptions: https://github.com/CesiumGS/3d-tiles/pull/654#discussion_r822163023

I've done a minor wording pass and elaborated some descriptions a bit. If this is not considered to be "done", it should be considered to be "ongoing", and/or tracked in a dedicated issue.

Add deprecated where applicable - batch table, feature table, properties

Done.


Unrelated:

We talked about replacing allOf [ { $ref: } ] with a simple $ref. I did this on one pass, in all placed except for the "root property". I think that it should be possible to replace

    "type": "object",
    "description": "Metadata about the tile's content and a link to the content.",
    "allOf": [
        {
            "$ref": "rootProperty.schema.json"
        }
    ],

with

    "$ref": "rootProperty.schema.json"
    "description": "Metadata about the tile's content and a link to the content.",

but this has to be verified (omitting the type at the top level looks odd, even though I'm somewhat sure that this is valid...)

lilleyse commented 2 years ago

We talked about replacing allOf [ { $ref: } ] with a simple $ref. I did this on one pass, in all placed except for the "root property". I think that it should be possible to replace .... with ... but this has to be verified (omitting the type at the top level looks odd, even though I'm somewhat sure that this is valid...)

Let us know what you find. Hopefully it's valid and we can remove all occurrences of allOf.

lilleyse commented 2 years ago

Could you also verify whether we can remove the empty property declarations in "derived" schemas? We've talked about removing them before. For example...

    "properties": {
        "id": {
            "type": "string",
            "pattern": "^[a-zA-Z_][a-zA-Z0-9_]*$",
            "description": "Unique identifier for the group within the tileset. This must be an alphanumeric identifier matching the regular expression `^[a-zA-Z_][a-zA-Z0-9_]*$`."
        },
        "class": {},
        "properties": {},
        "extensions": {},
        "extras": {}
    }
javagl commented 2 years ago

Let us know what you find. Hopefully it's valid and we can remove all occurrences of allOf. Could you also verify whether we can remove the empty property declarations in "derived" schemas?

It's tremendously hard to find that out by reading the specification. I had tried to find the answer, and eventually asked it at https://github.com/KhronosGroup/glTF/issues/2062#issuecomment-1075606401 (in the hope that I could stand on the shoulders of giants here). Questions like these quickly tend to lead to looong GitHub issue discussions in the json-schema-org GitHub organization, and even when there is seemingly reliable information (by the main spec editors) buried in one of these discussions, I always glimpse at the date, and when it's <=2018, I have to shrug and say: "Yeah, well, maybe this has changed in the meantime...". Particularly the handling of $ref seems to be a contentious topic, and underwent some semantic changes...

However, what I found out so far:

1. Replacing the type+allOf(ref) with a plain ref seems to be valid. I tried this out with one of our schema files, and this covers both aspects:

2. Omitting the properties that have already been part of the ref'd schema seems to be valid. I tried this out with our group.schema.json and the ref'd metadataEntity.schema.json, again, covering both aspects:

(I actually verified all this with https://github.com/jimblackler/jsonschemafriend . Kudos to the developer there. There are a bunch of JSON validation libraries out there that have ... ... lots of room for improvements. But this one seems to do what it is supposed to do).


tl;dr: I'll update the schemas accordingly. If this turned out to be "wrong", I'd be curious to see why...

javagl commented 2 years ago

The schema updates have been done, BUT I have not yet checked in how far that affects the code generator. (I noticed some fundamental flaws in my code generator, and am currently trying to find a solution for that). I'll try to check whether it works with the cesium-native generator as soon as possible.

lilleyse commented 2 years ago

Omitting the properties that have already been part of the ref'd schema seems to be valid.

Is it ok then to remove the empty {} everywhere?

javagl commented 2 years ago

Removing the empty {}s should be fine.

But replacing the allOf with ref throws out the code generator,

Cesium Schema Code Generator

I've already spent more time than I should with the intricacies of the code generator for this particular task, and trying to fix that does not seem to be possible (for me) within a reasonable time frame.

One viable path could be to create a PR based on this branch, just before the Replaced single-element allOf with ref commit, just to get it "done" and see what else is left on the checklist, but I'm open for other suggestions here as well.

lilleyse commented 2 years ago

Removing the empty {}s should be fine.

Ok let's get that change in and then merge. The code generator can be fixed post-merge.

lilleyse commented 2 years ago

Thanks @javagl.