fix(textureatlas): Allocate texture space more incrementally on resize

[donmccurdy]

Description

Texture coordinates are currently compressed to 12-bit precision, effectively on a 4096x4096 grid. That could be revisited (see #13050), but regardless, billboard-related code must be careful with texture coordinate precision, especially in large atlases. This PR makes two changes along those lines:

1. Atlases are now constrained to power-of-two dimensions on resize, aligning the atlas pixel grid cleanly with the 4096x4096 texture coordinate quantization grid, and ensuring we get the same visual results on WebGL1- and WebGL2-compatible devices.
2. When resizing atlases, TextureAtlas.js now considers more candidate sizes, and in most cases should not allocate space in the atlas with a packing density <<0.5.

Previously as the current candidate atlas size became larger, the iterative resize algorithm would use a larger multiplier to select the next candidate size, (2x, 4x, 8x, 16x, ...). In the particular case of #5154 this created a 2K-by-8K atlas, which required 16x more memory than required and (more visibly) exceeded the 4096px limit at which the 12-bit texcoord quantization could precisely address pixels in the atlas.

Preview:

before	after

This improves image alignment with each billboard, but isn't a complete fix for all such issues. More changes in upcoming PRs, but this change seemed worthwhile on its own.

Issue number and link

• Partial fix for Billboard clipped on right side #5154

Testing plan

I'm looking through existing examples with one or more billboards. For each relevant example, compare computed texture atlas size before vs. after. Check that atlas size is "reasonable", i.e. not >> 2x the total area of the input images.

• iTwin Feature Service
  • Before: 768 x 3072
  • After: 1024 x 1024
• Billboards
  • Before: 236 x 120
  • After: 128 x 32
• Clustering
  • Before: 264 x 1040, 752 x 220, 768 x 192, 256 x 256
  • After: 512 x 512, 256 x 128, 256 x 256
• Map Pins
  • Before: 192 x 192
  • After: 128 x 128
• GeoJSON simplestyle
  • Before: 768 x 3072
  • After: 1024 x 512
• Particle System Tails
  • Before: 320 x 1280
  • After: 256 x 256
• LocalToFixedFrame
  • Before: 264 x 912
  • After: 512 x 256
• Moon
  • Before: 848 x 232
  • After: 256 x 256
• Mars
  • Before: 1072 x 268, 768 x 192
  • After: 512 x 512, 256 x 256
• Particle System Fireworks
  • Before: 10240 x 1280
  • After: 1024 x 1024

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

PR Dependency Tree

• PR fix(textureatlas): Allocate texture space more incrementally on resize #13042 👈
  • PR fix(BillboardCollection): Fix precision loss in billboard image texcoords #13050

This tree was auto-generated by Charcoal

[github-actions]

Thank you for the pull request, @donmccurdy!

✅ We can confirm we have a CLA on file for you.

[donmccurdy]

In practice this loop can't spend more than 28 iterations for the largest supportable atlas size, because we'll have ... ahem, other challenges ... if the texture atlas size exceeds 16K x 16K. No need to use an increasing scaling factor (which may cause us to skip smaller qualifying atlas sizes) to reduce the number of iterations.

[donmccurdy]

Aside: Constraining to power-of-two dimensions is not strictly required to solve the precision/alignment issue, as the fix proposed in #13050 would address that issue independently.

I still personally feel that it's worthwhile. Fewer codepaths to think about, both manually and in unit tests, makes it easier to avoid precision bugs like those addressed by this PR. And we do at least need the power-of-two dimensions on WebGL 1 devices — so that code path can't go away at this time. See #13053 for background on why fewer code branches might be helpful for billboards. Even with power-of-two atlas dimensions, this PR still provides a consistent reduction in texture atlas sizes, reducing memory compared to earlier releases.

[jjhembd]

Thanks @donmccurdy, the code change makes sense to me, and the new spec is useful.

In your testing plan, how are you checking what size textures were allocated?

[jjhembd]

I think the link is wrong.
Normally we link to the issue we're fixing. In this case, since it's only a partial fix, maybe a link to the PR does make sense?

[javagl]

GitHub cleverly resolves any /issues/<number>/ and /pull/<number> to the respective /pull/ or /issues/ version, just based on the number. (I.e. this link, even though it (literally) has /issues/, is redirected to this PR).

[donmccurdy]

Ahh I'd copy/pasted the PR number without noticing that we linked to the issue not the PR, thanks! I did the same on my last two PRs as well, now fixed in this PR. 😅

Agreed, I might prefer to link to the PR instead of the issue in this particular case, since there's another bug remaining to fix for the original issue.

[donmccurdy]

Thanks @jjhembd!

In your testing plan, how are you checking what size textures were allocated?

My approach was to put a console.log statement here...

cesium/packages/engine/Source/Renderer/TextureAtlas.js

Line 438 in 3d78834

this._texture = this._copyFromTexture(context, width, height, newRectangles);

... and click through the demos in sandcastle manually. I could see an argument for logging a console warning (developer-facing only?) if the texture atlas exceeds 4K on any side, since that will (1) require more precision than current texture coordinate compression, and (2) likely fail on on some devices, particularly mobile. That might have made it easier to spot the issue in this case, but I haven't totally made up my mind about added logging to the PR - happy to add it, and would be curious if unconditional console.warn or //>>includeStart('debug', pragmas.debug); would be preferred.

[jjhembd]

Thanks @donmccurdy for the update! The tests work as expected for me.

[jjhembd]

@donmccurdy sorry I missed this earlier: this changelog entry should have been under a new heading for the January release (1.137).
I can fix it in the next PR.