Repository navigation
Conversation
buildScissorGroupsFromDraws grouped consecutive draws by clip only, and a ScissorGroup renders its tiers one after another: SDF shapes, convex paths, stencil paths, images, text. Under one clip every convex fill therefore landed before every stencil fill, whatever order they came in. Painter's-algorithm scenes break: a 3D surface drawn back to front had its non-convex far faces and its grid strokes painted over near faces. Start a new group where the tier changes as well as where the clip does. Runs of one tier stay one group, and draws that add nothing to a group (base layer, empty paths) never split one.
kolkov
left a comment
There was a problem hiding this comment.
The change looks right: groups split only where the tier changes, and runs of one tier keep their grouping. One question: drawTier mirrors the switch in drawsToScissorGroup. Would deriving one from the other, or adding a test that fails when a new tier is added to only one of them, keep the two from drifting apart?
kolkov
left a comment
There was a problem hiding this comment.
The bug is real and the fix is at the right level. A ScissorGroup renders its lists one after another, so grouping by clip alone does reorder draws inside a group — and painter's-algorithm content (back-to-front surfaces, grid strokes) is exactly the case that shows it.
I checked the grouping logic against the awkward inputs:
len(rc.pendingDraws) == 0returns early, sodrawTier(&rc.pendingDraws[0])cannot index past the slice.- A
tierNonedraw inside a run does not split it (convex, none, convexstays one group), and a group that starts withtierNonepicks up the first real tier it meets. Both are right: those draws contribute nothing to the group, so they cannot reorder anything. - The
clipChangedbranch is nowend := iin both arms, which is what the old code did with two branches; simplifying it is fine.
TestScissorGroups_SameTierStaysOneGroup is the test I care most about, because it pins the cost: a flat scene must not grow renders. Good that it is there.
One thing is still open from my earlier note, and this PR is the right place to close it. drawTier mirrors the switch in drawsToScissorGroup by hand. If someone adds a draw kind, or moves a kind between lists, in one of the two and not the other, the grouping silently reorders draws again — the exact bug this PR fixes, with no test failing. Two ways out, either is fine:
- a table test that walks every
drawCommandKindand asserts thatdrawTiersays "none" exactly whendrawsToScissorGroup([]drawCommand{cmd})puts nothing in the group; or - one function that reports both, so the tier and the placement cannot disagree.
I would take the test: it is a few lines, it survives refactoring of both sides, and it turns this class of regression into a red build.
Approving on the code once that guard is in. CI needs a fresh push for the September-era runs regardless.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Problem
Under a single clip, the GPU tier does not paint draws in the order they were submitted.
buildScissorGroupsFromDrawsputs consecutive draws with the same clip into oneScissorGroup.recordGroupDrawsthen renders a group one tier at a time: SDF shapes first, then convex paths, stencil paths, images and text. As a result, every convex fill of a clip lands before every stencil fill of that clip, whatever order the calls came in.This breaks any painter's-algorithm scene. When a 3D surface is drawn back to front, its non-convex far faces and its grid lines end up painted over the near faces.
Fix
Start a new group where the render tier changes, as well as where the clip changes.
RenderFrameGroupedalready renders groups in sequence, so this keeps submission order across tiers:drawTiermirrors the switch indrawsToScissorGroup, which is what decides the list a draw lands in. A table test checks the exact destination list for every draw command kind, including both path tiers and discarded shapes. A command-kind count requires new kinds to have a corresponding test fixture.Minimal example
repro-order/main.go depends only on gg and wgpu. It paints three shapes back to front: a blue square, then an orange concave arrow over it, then a green square over the arrow's tip. The squares take the convex fast path and the arrow takes stencil-then-cover. The program renders the scene once on the GPU and once on the CPU and compares the two:
In the wild: figure
We found this in figure, a grammar-of-graphics plotting library for Go that renders through gg. Its 3D surfaces are sorted back to front and drawn as one quad per cell. Most cells are convex, but some fold and go through the stencil path. On the GPU tier, the folded far cells, the box walls behind the surface and the grid lines were all painted over the near surface.
Here is the same four-camera saddle, rendered with figure's GPU tier (
github.com/timzifer/figure/backend/gg/gpu). Both renders already include the NonZero stroke fix:figure's GPU parity test for this scene drops from 1.2 % to 0.5 % of pixels off the CPU; what remains is anti-aliasing along cell edges. figure builds against our fork (timzifer/gg, tag
v0.52.6-figure.2) until this lands upstream.Tests
internal/gpu/draw_order_test.go:TestScissorGroups_KeepSubmissionOrderAcrossTiers: convex, stencil and convex fills under one clip must become three groups, in order. Without the fix they are one group.TestScissorGroups_SameTierStaysOneGroup: five convex fills under one clip stay one group.TestFlushGPU_DrawsConvertToScissorGroupsnow expects its SDF shape and convex path in two ordered groups instead of one.TestDrawTierMatchesScissorGroup: 17 cases cover all nine command kinds and compare the reported tier against the exact destination list. Shape cases distinguish fill and stroke alpha and cover unsupported shapes.TestScissorGroups_IgnoredDrawsDoNotSplitRun: base layers, empty paths and discarded shapes at the beginning, middle and end preserve a single convex run. Before the shape-tier correction, all four transparent/unsupported shape variants produced five groups instead of one.go test -race ./...passes (Windows amd64, Go 1.25.3).golangci-lint run --timeout=5mpasses.