Screenshot test: drawImage under rotated clip (#3921)#4993
Closed
shai-almog wants to merge 2 commits into
Closed
Conversation
Issue #3921 ("Clipping region not respected with non-90 degree rotations") reports an EncodedImage misclip under a rotated clip when the outer Graphics already has scale + translate applied. The existing ClipUnderRotation screenshot test (added in 95d64ba) covers the polygon-clip rasterisation path with fillRect; this new test exercises the same path through drawImage, which is what ddyer0's original screenshots actually show failing. Uses pushClip / popClip throughout. ddyer0's repro relied on `int[] clip = g.getClip(); ...; g.setClip(clip);` for save/restore, which can't by API contract preserve a non-axis-aligned (rotated-rect) clip shape -- ddyer0's own follow-up comment on the issue identified that round-trip as the source of the artefact, not a port bug. This test goes through the documented save/restore API so the rasterisation half of the bug is isolated. Three visual outcomes, distinguishable against a navy reference outline of the pre-rotation inner clip: - Correct: 30deg-tilted slice of the test image, overhanging the navy outline on two diagonal corners. - Bug A: image slice matches the navy outline exactly (rasteriser collapsed the rotated-rect clip to its bbox). - Bug B: full 60x60 image rendered, swamping the outline (rasteriser saw a polygon clip and disabled clipping; suspected pre-fix iOS Metal behaviour). Registered in Cn1ssDeviceRunner alongside ClipUnderRotation; added to the HTML5 skip list to match the other graphics screenshot tests. Goldens not included -- they need a CI capture per port. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
JavaScript port screenshot updatesCompared 21 screenshots: 20 matched, 1 missing reference.
|
Collaborator
Author
Android screenshot updatesCompared 111 screenshots: 110 matched, 1 missing reference.
Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
iOS Metal screenshot updatesCompared 111 screenshots: 110 matched, 1 missing reference.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
iOS screenshot updatesCompared 111 screenshots: 110 matched, 1 missing reference.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Feedback on the first revision: the 4 grid cells looked different from one another (the outer scale + translate I copied from ddyer0's repro mapped the image to slightly different positions depending on cell bounds), the colours were not vivid enough (the green 4px border disappeared at the rendering scale), and the comment block kept referring to a non-existent "red fill" carried over from the sibling ClipUnderRotation.java. Changes: - Geometry computed from cell centre: `side = 0.7 * min(w, h)`, `imgX = x + (w - side) / 2`. Identical across all 4 cells. - Outer scale + translate dropped. They were ddyer0's repro shape, but the bug is about rotation under clip, not scale, so the scale/translate was noise. - Test image is now solid yellow with a 10-pixel magenta border and a black diagonal X. Yellow / magenta / black all distinct from the gray background and the navy reference outline. - A dim (alpha=64) full-size copy of the image is drawn first as an underlay so the clipped over-paint has a baseline to compare against: if the bright over-paint extends past the dim underlay it's a no-clip bug; if it matches the navy outline exactly it's a bbox-collapse bug; if it's a tilted square overhanging the navy outline at two corners it's correct. - Comment block shrunk to ~15 lines. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Summary
ImageClipUnderRotationscreenshot test that reproduces the exact shape of Clipping region not respected with non-90 degree rotations. #3921 -- outerscale(2.5) + translate, per-tilerotateRadians(30deg) + clipRect(inner) + drawImage-- with the properpushClip/popClipsave/restore.ClipUnderRotation(PR Screenshot test for clip-under-rotation (#3921) #4924) which covers the same polygon-clip rasterisation path withfillRect. ddyer0's original screenshots show a misclippedEncodedImage, not a misclippedfillRect, so thedrawImagepath needs its own coverage.pushClip/popClipthroughout. The original repro usedint[] clip = g.getClip(); ...; g.setClip(clip);which can't preserve a rotated-rect clip shape -- ddyer0's own follow-up comment on the issue identified that round-trip as the source of his visible artefact, not a port bug. Using the documented save/restore API isolates the rasterisation half.Expected outcomes (visually distinguishable against the navy axis-aligned outline of the pre-rotation inner rect)
ClipRect.mpolygon initialiser storedx=y=w=h=-1, Metal execute then calledCN1MetalSetScissor(0, 0, -2, -2)whose width<=0 / height<=0 branch sets the scissor to the full framebuffer).The 2x2 grid from
AbstractGraphicsScreenshotTestseparates the form-graphics path (top cells) from the mutable-image path (bottom cells), so a Metal-only / mutable-only regression localises automatically.Test plan
🤖 Generated with Claude Code