Repository navigation
Keep live atlas builder from packing over existing sprites - #72
Merged
Merged
Conversation
markRegion replaced the skyline over the marked columns, so marking a shorter entry after a taller one lowered it and new images were packed on top of existing sprites. It now only raises the skyline and clamps the region to the atlas width. Fixes treeform#70. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #70.
Problem
newAtlasBuilderFromAtlasfills its skyline by callingmarkRegionfor every existing entry, in table order.markRegionusedaddToSkyline, which replaces the skyline over the rect's columns withy + height. When a shorter entry was marked after a taller one in the same columns, the skyline went down. The nextaddImage/sk.addAtlasImagethen landed on top of an existing sprite, so every quad drawing that sprite showed the new image.Fix
markRegionnow only raises the skyline. Across[x, x + width), each node keepsmax(node.y, y + height). Parts of nodes outside the region stay as they are, and neighbors with the same height are merged. So the order entries are marked in no longer matters.The region is also clamped to
[0, atlasSize).markOccupiedEntrypads by the margin, so a sprite on the right edge used to create a skyline node that went past the end of the atlas.allocatestill usesaddToSkyline. That's correct there:findSkylinePositionplaces a new rect at or above every node it covers, so replacing those nodes can't lower the skyline.Tests
Added to
tests/test_atlas_png.nim(already run in CI):newAtlasBuilderFromAtlasplaces a new image without overlapping any existing entry. This is the builder repro from the issue.Without the fix, the new tests fail with
(success: true, x: 0, y: 8, ...). With the fix, all tests pass. The issue's repro now outputs:The other unit tests in CI (
test_textboxes,test_inputs,test_layout,test_layout_regressions,test_layout_clips,test_cpu_backend) still pass on macOS.🤖 Generated with Claude Code