Skip to content

Optimization of offsets for MAXAR_content_geojson string properties#1415

Open
timoore wants to merge 3 commits into
mainfrom
maxar-offset-opt
Open

Optimization of offsets for MAXAR_content_geojson string properties#1415
timoore wants to merge 3 commits into
mainfrom
maxar-offset-opt

Conversation

@timoore

@timoore timoore commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

Offsets to string property values were always stored as 64 bit integers. This PR chooses the appropriate size, which can be UINT8, UINT16, UINT32, or UINT64

Issue number or link

#1414

Author checklist

  • I have submitted a Contributor License Agreement (only needed once).
  • I have done a full self-review of my code.
  • I have updated CHANGES.md with a short summary of my change (for user-facing changes).
  • I have added or updated unit tests to ensure consistent code coverage as necessary.
  • I have updated the documentation as necessary.

Remaining Tasks

Testing plan

Run a client with a MAXAR_content_geojson tileset and verify that string metadata is still coherent.

Reviewer checklist

Thank you for taking the time to review this PR. By approving a PR you are taking as much responsibility for these changes as the author.

As you review, please go through the checklist below:

  • Review and run all parts of the test plan on this branch and verify it matches expectations.
    • If the issue is a bug please make sure you can reproduce the bug in the main branch and then checkout this branch to make sure it actually solved the issue.
  • Review the code and make sure you do not have any remaining questions or concerns. You should understand the code change and the chosen approach. If you are not confident or have doubts about the code, please do not hesitate to ask questions.
  • Review the unit tests and make sure there are no missing tests or edge cases.
  • Review documentation changes and updates to CHANGES.md to make sure they accurately cover the work in this PR.
  • Verify that the Contributor License Agreement has been submitted, if needed.

@j9liu j9liu self-requested a review July 13, 2026 17:53

@j9liu j9liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @timoore! I tried this out in Unreal and confirmed that the string data still returns correctly.

Just a few minor comments before merge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Update CHANGES.md for this with a note under "Fixes"?

Comment on lines +137 to +140
// Shouldn't happen
if (parameterIt == &offsetParameters[4]) {
return offsetParameters[3].gltfConstant;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this shouldn't happen, perhaps indicate so with a debug / CESIUM_ASSERT statement instead?

Comment on lines +318 to +320
// Offsets refer to arrays, so their maximum possible size is the platform's
// size_t.
using StringOffset = size_t;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: I was confused by what "refer to" meant on the first read, but maybe this would be clearer?

Suggested change
// Offsets refer to arrays, so their maximum possible size is the platform's
// size_t.
using StringOffset = size_t;
// Since offsets refer to the offset of array elements, their maximum
// possible size is the platform's size_t.
using StringOffset = size_t;

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.

2 participants