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

Avoid duplication in schema with metadataEntity #608

Merged
merged 5 commits into from
Dec 20, 2021

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Dec 12, 2021

A draft that may eventually fix #607 - further details to be discussed there.

@lilleyse
Copy link
Contributor

This change looks good to me. It's essentially equivalent to what glTF does for material.normalTextureInfo.schema.json and material.occlusionTextureInfo.schema.json which "inherit" from textureInfo.schema.json and can add their own additional properties. The code generators in cesium-native handle these types.

One difference I noticed is that material.normalTextureInfo.schema.json and material.occlusionTextureInfo.schema.json declare the properties of the base schema.

        "index": { },
        "texCoord": { },

Should that be happening here as well?

{
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "$id": "group.schema.json",
    "title": "Group Metadata",
    "type": "object",
    "description": "An object containing metadata about a group.",
    "allOf": [
        {
            "$ref": "metadataEntity.schema.json"
        }
    ],
    "properties": {
        "class": {},
        "properties": {},
        "extensions": {},
        "extras": {}
    }
}

@javagl
Copy link
Contributor Author

javagl commented Dec 13, 2021

Should that be happening here as well?

This could be done here, and maybe it should be done, but only for consistency. Long(er)-term, these properties may be omitted when they are already mentioned in the allOf base schema. (This might eventually also be done in glTF, as of KhronosGroup/glTF#2062 (comment) , but probably only after a few other updates).

@javagl
Copy link
Contributor Author

javagl commented Dec 13, 2021

I could mark this as "ready for review", but before merging, we should probably check whether the spec text should be updated - for example, places that talk about a group might have to be reworded, from

This-and-that is a group, see the [group.schema.json]

to

A group is a metadata entity, see [metadataEntity.schema.json]

@lilleyse
Copy link
Contributor

Yes, do a pass over the spec text and then bump me for review.

@javagl
Copy link
Contributor Author

javagl commented Dec 18, 2021

I've added the links to the schema where appropriate, and did a minor wording update.

I stumbled over the fact that the word 'entity' was apparently used with a meaning that is different from the definition in the https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#concepts section:

Entities are instantiations of a class, populated with property values conforming to the class definition.

The wording here said

... properties do not take on particular values until a metadata is assigned (i.e. the class is "instatiated") to a particular entity within the 3D Tiles hierarchy

Specifically:

  • "Entity" once means "that thing that is created from the metadata values" (i.e. the instance of the metadata class)
  • "entity" can also be the object that this instance is associated with (e.g. the group or tile)

Maybe the wording would be less confusing if it was

... properties do not take on particular values until a metadata is assigned (i.e. the class is "instatiated") to a particular entity within element of the 3D Tiles hierarchy

but maybe some of these ambiguities are inevietable, due to the genericity of the terms. I hope that the updated wording is OK, though...

@javagl javagl marked this pull request as ready for review December 18, 2021 20:32
@lilleyse
Copy link
Contributor

Thanks @javagl. The edits looks good to me.

@lilleyse lilleyse merged commit 3dd9c6e into CesiumGS:main Dec 20, 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.

Avoid duplication in schema - consider a metadataEntity
2 participants