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

Clarifications and additions for the specification #711

Closed
javagl opened this issue Aug 24, 2022 · 3 comments
Closed

Clarifications and additions for the specification #711

javagl opened this issue Aug 24, 2022 · 3 comments

Comments

@javagl
Copy link
Contributor

javagl commented Aug 24, 2022

Edit 2023-01-11: Most of the points mentioned here have been addressed in #726 (comment) - see the next comment for an updated state summarizing the points that are not addressed yet.

Possible constraints and clarifications to add to the schema and specification:

For boundingVolume:

  • The latitude and longitude for the region bounding volume should be constrained to the ranges that are mentioned in https://spatialreference.org/ref/epsg/4979/ (-180.0000, -90.0000, 180.0000, 90.0000)
  • The minimum height should not be larger than the maximum height
  • The south may not be greater than the north
  • The radius of the bounding sphere should not be negative

For properties (although deprecated?)

  • The maximum should not be smaller than the minimum

For statistics:

  • The classes should be required
  • The desciption says "A dictionary, where each key corresponds to a class ID in the classes dictionary and..." - this is confusing. It should say "... in the classes dictionary of the metadata schema of the containing tileset, and ..."
  • Similarly the statistics.class.properties description should refer to the class of the metadata schema

For statistics.class.property:

  • One could add constraints (like !(min>max)) here. But one could go pretty far, e.g. !(mean>min), and even make mathematically based constraints about the possible values of the standard deviation and such. Maybe this is a step too far for now (there are more important constraints/checks to be added)

For class.property:

  • The enumType should be disallowed for non-"ENUM" types
  • I think the componentType should be required for the SCALAR/VECn/MATn types.
  • One could go full pedantic mode, and require min/max to be integer values when the component type is integral, and require the bounds of the values to be obeyed. For a UINT8 (without offset or scale), a max=0.8 does not make sense, and neither does a min=-10000.
  • The min/max/offset/scale do not make sense for variable length arrays. This is mentioned at https://github.com/CesiumGS/3d-tiles/tree/draft-1.1/specification/Metadata#minimum-and-maximum-values but could also be mentioned in the schema

For templateUri

  • One could consider to add the requirement that the variables {level}, {x}, {y}, {z} MUST be present for the respective tree types. For now, I assume that any {unknown} expression is an error, and "missing" ones (e.g. a missing {z} for an OCTREE) should cause a warning.

For subtree

  • I thought that one could add the constraint that buffer views may not overlap, so that
    { byteOffset: 0, byteLength: 10 }
    { byteOffset: 5, byteLength: 5 }
    
    was disallowed. But I don't see a strong reason to disallow that (and glTF apparently does allow this as well...)

For availability

  • One could add a constraint that availableCount may not be larger than bufferView.byteLength/8 (?)
  • The Content Availability section could be a bit clearer about multiple contents (it only refers to tile.content now, and does not mention tile.contents)
  • We could add a rule to say that trailing (unused) bits in an availability buffer should always be 0

For tile :

  • One could consider to add constraints to the transform. From "weak" to strict: It should probably not be zero. It should probably be invertible. It should probably have a positive determinant. It should probably not contain skew/shear components. It should probably be decomposable into translation, rotation, scale components. See https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#transformations for inspiration.
  • The conditions for when content/contents MUST be present are difficult. One could say that one of them MUST be present when implicitTiling is present, but ... strictly speaking, only when there is any contentAvailability[i]=true.

For schema

  • The schema.id is marked as 'required' and must match the usual "ID regex". The ID may eventually be used (e.g. by CesiumJS) to uniquely identify properties in Custom Shaders, or to identify that the schema objects of two loaded tilesets are the same schema. But the spec should (at least as an 'Implementation Node') describe the purpose, and give usage hints - for example, whether it should only be a semi-random "unique ID", or a human-readable string, or maybe something that includes a concept of "versioning".

For tileset:

  • One could mention that each element of extensionsRequired must also appear in extensionsUsed

Structural:

  • It should be clarified under which conditions certain constraints are condidered to be satisfied. For example, when a tile defines a geometricError that is larger than the geometricError of the parent, but overrides this geometric error with metdata so that it is smaller than that of the parent, should this cause a validation issue?

For subtree

Regarding extensionsUsed:

  • The spec says at https://github.com/CesiumGS/3d-tiles/tree/draft-1.1/specification#extensions-1

    All extensions used in a tileset or any descendant external tilesets shall be listed in the entry tileset JSON

    This should be reviewed and possibly clarified. If one tileset refers to an external tileset that does not use any extensions, but is later modified to use a certain extension, then the containing tileset may become "invalid", because it does not declare this extension as extensionsUsed. While this makes sense from the perspective of knowing which extensions will be used as a whole, it somewhat contradicts the idea of external tilesets being "building blocks" that can be combined arbitrarily.

@javagl
Copy link
Contributor Author

javagl commented Jan 11, 2023

Most of the points mentioned above have been addressed in #726 (comment). Points that have been moved into dedicated issues or remain open for discussion are listed here:

Issues that rather affect the validator, and not so much the specification:

Open issues from the list above:

  • For statistics.class.property: One could add constraints (like !(min>max)). There are some constraints that could easily be added and checked. But it's hard to say which constraints could be justified mathematically. (This is related to Consider validating limits of property values 3d-tiles-validator#260, and may be revisited when this has been examined further)

  • For tile: About the presence of content or contents: Right now, the implicit tiling section says that tile.content and tile.contents shall be omitted when there is no contentAvailability[x]===1. It seems natural to require it to be present when there is a 1-bit in the bitstream. Should this be added in the spec text and the tile.content schema? More importantly: Are there other (less tangible or obvious) conditions under which tile content must be present?

A new question that could be considered for the specification:

  • Should empty dictionaries {} or arrays [] be disallowed in general? In many places, they already are disallowed via the minProperties/minItems constraints in the schema. But there are some places that explicitly require empty elements, or at least do not explicitly disallow them.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 18, 2023

@javagl
Copy link
Contributor Author

javagl commented Jun 12, 2023

The points here should all have been addressed with the finalization of 3D Tiles 1.1. If there's anything left, it can be tracked in a dedicated issue.

@javagl javagl closed this as completed Jun 12, 2023
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

No branches or pull requests

2 participants