CesiumGS / 3d-tiles

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

Consider avoiding ambiguities for property names in extensions #496

Closed javagl closed 2 years ago

javagl commented 3 years ago

There are some cases where extensions define a new property that has the same name as a property of the extended object. For example, tile.3DTILES_multiple_contents.schema.json defines a property that is called content, and that is an array, and tile.schema.json defines a property that is called content, but this is a single object.

In order to avoid ambiguities, one should consider to avoid overloading names like that.

ptrgags commented 3 years ago

@javagl Hm... thinking about multiple contents, we probably should call it contents plural since it's an array.

I think the thinking here was that what we conceptually wanted to do was change the 3D Tiles 1.0 content property to accept arrays. However, we didn't want to modify the core specification, so we just made a property in the extension that is supposed to override the main one.

Did you find any other cases with naming ambiguities like this?

javagl commented 3 years ago

The content was the only specific example that I've seen so far. (And it could (and IMHO should) indeed be named contents in the extension)

The purpose of this issue was also to find an agreement of whether something like this should be avoided in general, and (if this is the case), resolve this issue by reviewing the existing extensions for further cases like this, and fix them accordingly.

donmccurdy commented 3 years ago

Both content and contents seem equally fine for an array in my opinion, the term "content" is more uncountable than singular. But agree that consistency in usage and type is important.

lilleyse commented 2 years ago

It might make sense to rename it to contents in the extension now.

To support multiple contents in 3D Tiles 1.1 without introducing a breaking change we added a contents property: https://github.com/CesiumGS/3d-tiles/pull/634#discussion_r812108700. 3D Tiles 1.1 and 3DTILES_multiple_contents should have consistent naming.

lilleyse commented 2 years ago

Fixed in https://github.com/CesiumGS/3d-tiles/pull/649