fix(webgl): respect clipping rect in reuseRenderOp's default-shader path#32
Merged
Conversation
The "both shaders are 'default'" fast path in `reuseRenderOp` returned true without checking the clipping rect (or RTT state). Two clipping containers with different clipping rects but the same default shader were getting batched into one render op, so every quad in the batch was drawn under the FIRST node's scissor rect — meaning containers past the first in a row lost their background (drawn outside the batched scissor) and per-container guides like reference lines were silently culled. The text-vertical-center example surfaced this: with the optical-mode default and a re-ordered tree, all but the first box per row lost their color, and one row's reference line disappeared on top of the sub-pixel rasterization issue described below. Move the RTT and clipping-rect checks above the default-shader fast path so correctness can't be bypassed. Performance impact is negligible — compareRect is 4 number compares. Also: bump the center-guide line in `text-vertical-center` from h=1 to h=2. At the default examples config (resolution=720, appHeight=1080) the logical→physical scale is 0.667, so a 1-logical-px line at certain Y values covers no pixel center and disappears — separate, well-known sub-pixel rasterization behavior, not a renderer bug. 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
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
WebGlRenderer.reuseRenderOphad a fast path for "both nodes use the default shader" that returned true without checking the clipping rect or RTT state. Two clipping containers with different scissor rects but the same default shader were getting batched into one render op, so every quad in the batch was drawn under the first node's scissor.In practice this meant: in a row of side-by-side
clipping: trueboxes, only the first box rendered its background — every subsequent box's quad was clipped out by the first box's scissor rect. Per-box children (reference lines, small markers) were silently culled too.How it surfaced
The
text-vertical-centerexample renders a row of colored clipping boxes per fontSize, each with a horizontal red center-guide line. After the'optical'baseline default landed, re-ordering the children of each box (text before line) created a render op layout that triggered the bug — boxes past the first lost their color, and one row's red line vanished.The bug existed all along; the chain-break from SDF text ops (which sets
curRenderOp = null) was masking it. Any pattern that puts twoclipping: truecolored siblings adjacent in render order would hit it.Fix
Move the RTT and clipping-rect checks above the default-shader fast path in
reuseRenderOp:Performance impact is negligible —
compareRectis four number compares.Bonus: example test sub-pixel fix
While diagnosing this I also found that
text-vertical-center's fontSize 60 row was missing its red center-guide line. Separate issue, but worth shipping together for review context:At the default examples config (
resolution=720, appHeight=1080), the logical→physical scale is720/1080 = 0.667. A 1-logical-px line at certain Y positions covers no pixel center after rasterization and disappears. fontSize 60's row at logical y=430 lands at physical Y range[286.67, 287.33]— neither pixel 286 (center 286.5) nor pixel 287 (center 287.5) is inside. Bumping the line toh: 2(1.33 physical px) guarantees rasterization. Added a comment explaining it.Test plan
pnpm buildclean.vitest run— 182/182 pass.pnpm start: with the fix,text-vertical-centershows all boxes with their background colors and all rows' red center-guide lines.Files
reuseRenderOpcorrectness fix.🤖 Generated with Claude Code