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

Cleanup pass for 1.1 - JSON schema #677

Merged
merged 10 commits into from
Mar 28, 2022

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Mar 22, 2022

Addressing some further points from #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: #654 (comment)

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

javagl commented Mar 23, 2022

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 KhronosGroup/glTF#2062 (comment) (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:

  • "Downwards": It validates an example input.
  • "Upwards": It passes validation against the 2020-12 meta-schema (!)

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:

  • It complains when a group is missing something that is required as of metadataEntity (namely, the class)
  • It complains when something does not match the definitions from the ref'd schemas (e.g. when there is an extension that is, say, a string instead of an object)

(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
Copy link
Contributor Author

javagl commented Mar 26, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

javagl commented Mar 28, 2022

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
Copy link
Contributor

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
Copy link
Contributor

Thanks @javagl.

@lilleyse lilleyse merged commit 83c237c into CesiumGS:draft-1.1 Mar 28, 2022
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