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 for the 1.1 specification #726

Merged
merged 10 commits into from
Jan 3, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jan 2, 2023

Addressing #711 : I'll replicate the bullet point list here, and mark the parts that have already been solved with checkboxes.

  • 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)
      • NOTE: It's actually given in radians, deviating from the EPSG spec. Emphasized this in the text.
    • 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 Based on internal discussion, it should remain optional
    • 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 Note') 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`](https://github.com/CesiumGS/3d-tiles/blob/aafebe993986aa9c668d35a63a42cc72ccdcc327/specification/schema/Subtree/subtree.schema.json#L16)
  • 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 3, 2023

A summary of the points that are not ticked:


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.

For class.property: 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.

  • I think that this could make sense. But I'd tend to tackle this as its own issue/PR, if desired. Taking the offset and scale into account makes this not entirely trivial. One would have to write it in a form like

    The max may not be larger than maxDataTypeValue*scale+offset ...

    with proper specification of what maxDataTypeValue is...


For templateUri ... One could consider to add the requirement that the variables {level}, {x}, {y}, {z} MUST be present for the respective tree types.

  • I'm hesitating to constrain this further for now, in view of the possible alternatives for template URIs that may be required in the context of Limiting the number of files in a directory for implicit tiling #598 . For example, iff we, say, define an extension that allows a variable like {mortonIndex}, then the other variables would be unused...

For subtree ... I thought that one could add the constraint that buffer views may not overlap

  • There doesn't seem to be a strong reason to add that requirement

For availability ... One could add a constraint that availableCount may not be larger than bufferView.byteLength/8 (?)

  • That doesn't seem to be necessary, in hindsight.

For tile ... (see checklist)

  • I think that adding constraints for the transform could make sense. If this should be addressed, it should be tracked in a dedicated issue.
  • 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?

Structural: ...

  • Somehow related to the second point of the tile considerations (above): It's sometimes hard to pin down how semantics affect the validity of an asset. This might be a broader question, once on the spec level, and once on the level of how the validator treats semantics.

Regarding extensionsUsed: ...

@javagl javagl marked this pull request as ready for review January 3, 2023 18:52
@lilleyse
Copy link
Contributor

lilleyse commented Jan 3, 2023

Looks good. Thanks @javagl.

@lilleyse lilleyse merged commit ffc9cf2 into CesiumGS:draft-1.1 Jan 3, 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

Successfully merging this pull request may close these issues.

None yet

2 participants