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

Property table array in subtree format #642

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Mar 2, 2022

Instead of inlining a property table in tileMetadata and contentMetadata there is now a dedicated section for property tables. This is consistent with EXT_structural_metadata and is more future proof if we ever need to support property tables referring to other property tables for metadata inheritance (#641).

Current                                                  Proposed                                  
{
  "tileMetadata": {
    "class": "tile",
    "count": 85,
    "properties": {
      "horizonOcclusionPoint": {
        "values": 2
      },
      "countries": {
        "values": 3,
        "arrayOffsets": 4,
        "stringOffsets": 5,
        "arrayOffsetType": "UINT32",
        "stringOffsetType": "UINT32"
      }
    }
  },
  "contentMetadata": [
    {
      "class": "content",
      "count": 60,
      "properties": {
        "attributionIds": {
          "values": 6,
          "arrayOffsets": 7,
          "arrayOffsetType": "UINT16"
        },
        "minimumHeight": {
          "values": 8
        },
        "maximumHeight": {
          "values": 9
        },
        "triangleCount": {
          "values": 10,
          "min": 520,
          "max": 31902
        }
      }
    }
  ]
}
{
  "propertyTables": [
    {
      "class": "tile",
      "count": 85,
      "properties": {
        "horizonOcclusionPoint": {
          "values": 2
        },
        "countries": {
          "values": 3,
          "arrayOffsets": 4,
          "stringOffsets": 5,
          "arrayOffsetType": "UINT32",
          "stringOffsetType": "UINT32"
        }
      }
    },
    {
      "class": "content",
      "count": 60,
      "properties": {
        "attributionIds": {
          "values": 6,
          "arrayOffsets": 7,
          "arrayOffsetType": "UINT16"
        },
        "minimumHeight": {
          "values": 8
        },
        "maximumHeight": {
          "values": 9
        },
        "triangleCount": {
          "values": 10,
          "min": 520,
          "max": 31902
        }
      }
    }
  ],
  "tileMetadata": 0,
  "contentMetadata": [1]
}

@lilleyse lilleyse requested a review from javagl March 2, 2022 16:44
Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I haven't yet started the implementation, this change looks sensible to me.

Copy link
Contributor

@javagl javagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to align the structure more with that of EXT_structual_metadata.
I'm a bit confused about tileMetadata and contentMetadata, though: They are ... wrappers for an integer. If I had to find a reason for that, could only come up with ~"well, maybe, some day, we want to store other things there, beyond the property table index". But that sounds a bit vague. Is there any specific reason to wrap this index into objects?

@lilleyse
Copy link
Contributor Author

lilleyse commented Mar 3, 2022

I'm a bit confused about tileMetadata and contentMetadata, though: They are ... wrappers for an integer. If I had to find a reason for that, could only come up with ~"well, maybe, some day, we want to store other things there, beyond the property table index". But that sounds a bit vague. Is there any specific reason to wrap this index into objects?

Future proofing was the main reason. Maybe there would be other encodings in the future? Metadata inheritance is also in the back of my mind, but now that I think about it those fields would be part of the property table definition. It should be ok to simplify and make them integers instead of wrappers for integers. I'll make the change.

@lilleyse
Copy link
Contributor Author

lilleyse commented Mar 3, 2022

@javagl updated

@javagl
Copy link
Contributor

javagl commented Mar 3, 2022

Future proofing was the main reason.

So now it will be my fault if we hit a road block in the future :-) But I don't see any reason now to not merge it.

@javagl javagl merged commit 5fd4856 into extension-revisions Mar 3, 2022
@lilleyse lilleyse deleted the subtree-property-tables branch March 3, 2022 19:53
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.

3 participants