fix(textureatlas): Apply internal padding between images

[donmccurdy]

Description

The texture atlas used for billboard rendering has 1px padding enabled by default (borderWidthInPixels), but the padding is only applied around the edges of the atlas — not between images. I believe this is the cause, or at least a cause, of #4525. This PR introduces internal padding, so that the existing option affects spacing between images in the atlas. Image subregions have no padding and are unaffected.

Screenshots:

before	after

Issue number and link

• Fixes Rouge outline and boxes around label's characters and billboards #4525
• Fixes half of Billboard clipped on right side #5154

Testing plan

See unit tests and #13037, and:

http://localhost:8080/Apps/Sandcastle2/index.html?id=itwin-feature-service

With 'points' (pins) enabled and all other features hidden, this is a good example showing the top of one pin bleeding into the bottom of the pin above it in the texture atlas.

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): Apply internal padding between images #13038 👈

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.

[jjspace]

I see that adding padding to the atlas works but that's just filling the last row of pixels with transparency isn't it? Would it be better to say that the billboards are displaying 1 px too much from the atlas?

[donmccurdy]

I see that adding padding to the atlas works but that's just filling the last row of pixels with transparency isn't it?

@jjspace previously the atlas put an N-pixel border around the perimeter of the entire atlas, then packed billboards tightly inside of that. With this PR the primary goal is to have interior N-pixel borders between billboards as well.

Implementation-wise, there's more than one way to get there. TexturePacker.js implements binary tree bin packing, where each time a billboard is added, we find an empty rectangle (the large outer square below), insert the image (yellow tile below), and create two new empty rectangles in the remaining space for future use:

Image Credit: Jake Gordon

The solution in this PR is that each time we do an insertion and create those two empty rectangles in the remaining space, we offset them from the inserted billboard by N pixels so that space isn't considered available:

Entirely possible there are other better ways to do this! With the visual tests I'm hoping it will be easier to see the tradeoffs if we decide to change how the padding is handled.

---

Would it be better to say that the billboards are displaying 1 px too much from the atlas?

I think it's a combination of linear filtering in the texture sampler and perhaps some numerical precision limits. I also tested switching to 'nearest' filtering and while that definitely reduces the visual artifacts here, it doesn't solve them entirely (which this PR does, as far as I can tell). I may do a separate PR for the texture filtering, but that has other tradeoffs and wouldn't be the right fix for this specific issue probably.

In #13020 there was also some discussion of mipmapping billboards, and padding between billboards would be a hard prerequisite to that.

Finally, I suspect this may have been the original intention of the TexturePacker.js borderWidthInPixels option? Or at least I can't think of a reason that padding on the atlas perimeter would have been useful without also including internal padding.

[jjspace]

Thanks for the explanations @donmccurdy, I think this makes sense given that. Just one more small question then I think this is good to go

[donmccurdy]

Added the new test case, good call on that, thanks @jjspace!

[jjspace]

LGTM, thanks for the updates