Fix nested canvas clipping and clipped clears - #1872
bkaradzic-microsoft wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR fixes correctness issues in the Canvas2D NanoVG-backed implementation, specifically around nested clipping behavior and clearRect behavior under clipping/alpha/filters, and adds GPU readback regression tests to prevent regressions.
Changes:
- Intersect Canvas rectangle clipping with the active NanoVG scissor to preserve parent clip bounds for nested clips.
- Implement
clearRectviaDESTINATION_OUTcoverage while forcingglobalAlpha=1and clearing filters, restoring prior state afterward. - Add GPU readback regression tests for nested clips and clipped clears, with skip handling wired from the native test runner.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Polyfills/Canvas/Source/nanovg/nanovg_filterstack.h | Initializes filter stack count to avoid undefined state when creating “empty” stacks. |
| Polyfills/Canvas/Source/Context.cpp | Updates clip handling to use scissor intersection and adjusts clearRect compositing to clear only clipped regions independent of alpha/filters. |
| Apps/UnitTests/Source/Tests.JavaScript.cpp | Exposes a global flag to skip GPU canvas tests when render/external texture tests are disabled. |
| Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts | Adds GPU readback regression tests for nested clips and clipped clearRect. |
| Apps/UnitTests/JavaScript/dist/tests.javaScript.all.js | Bundled output updated to include the new JS test cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
|
The Windows/Hermes failure occurred after a successful build: snippet requests returned HTTP 403 on that runner, so all 314 Playground cases failed during scene loading (three failed fetch attempts per case), before rendering. The same commit passed all 314 cases in the Windows D3D11 and V8 jobs, and the failed snippet URLs now return HTTP 200 from a separate probe. No code or test changes are warranted by this evidence. The failed-only retry (attempt 2, job 103128300425) has now passed both Validation Tests and Unit Tests. All 34 checks are green on the same commit; no code or test changes were needed. |
8589386 to
f1a87e5
Compare
Intersect rectangular clips with the existing NanoVG scissor so nested clips preserve parent bounds and save/restore state. Clear clipped regions with destination-out coverage while ignoring global alpha and filters. Initialize empty filter stacks and add GPU-readback regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Zero-initialize the filter element array because NanoVG copies the full filter stack into state and draw calls. Clarify the existing asymmetric anti-alias clip extension without changing its bounds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
The snippet returns its scene before ImportMeshAsync and onMeshLoad finish, so readiness can capture an empty scene. Preserve the import promise and return the scene only after its completion, using the existing per-test source replacement support. A delayed import reproduced every pixel of the blank CI image on both the PR and its upstream base. The scene now passes with the same delay, without changing the reference image, render count, or tolerance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Normalize negative rectangle extents before NanoVG scissor intersection, preserving parent bounds, transforms, and the existing AA adjustment. Add GPU readback coverage for both signed axes and rotated nested clips, comparing all pixels with equivalent positive rectangles. The regression fails against the previous PR head and passes with the normalization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
055b39a to
763b288
Compare
Summary
clearRectwith opaque destination-out coverage, independent ofglobalAlphaand filters, while restoring the prior drawing state.Base
Rebased onto
masterat4af724a6. Includes signed-rectangle normalization before scissor intersection. This includes the already-merged Canvas work from #1855 and does not depend on the open branding PR #1844.Validation
RelWithDebInfo: all 6 selected native cases passed, including all 94 JavaScript cases.Signed-rectangle update