fix(desktop): clear the dirty boundary set before painting, not after - #198
Conversation
A boundary re-dirtied while it was being recorded re-registers itself for the next frame (layer_tree.go, "if boundary re-dirtied, register it for next frame"). draw() then called ClearDirtyBoundaries() at the end of the frame, which threw that registration away. The widget's own sceneDirty stayed true, so every later InvalidateScene took the already-dirty O(1) guard and returned without notifying the window — the boundary was never painted again. The set is only the O(1) frame-skip gate: painting walks the tree on each boundary's sceneDirty and never reads it. So it can be cleared as soon as the gate has consumed it, which leaves anything registered during painting intact. Any widget written from a goroutine other than the UI thread hits this on the first frame that overlaps a write, and animated widgets that re-dirty during Draw are the same shape. A terminal emulator under continuous output froze within one frame and stayed frozen after the output stopped; the render loop kept being woken 60 times a second and skipped every frame. The test covers the app-level contract the fix depends on — a boundary that re-dirties during recording is still registered after a paint pass that began with a cleared set. draw() itself has no test: it needs a live gogpu.Context.
kolkov
left a comment
kolkov
left a comment
There was a problem hiding this comment.
Validated against code and enterprise references (Flutter _nodesNeedingPaint pattern). Re-registration path confirmed (layer_tree.go:585-591), already-dirty guard confirmed (boundary.go:87-95), painting does NOT read dirty set. Fix is safe and minimal.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Merged, thank you. We had 15 local commits on a feature branch when this landed — rebased cleanly, no conflicts. Significant parallel work has been happening in gg as well (text rendering quality, vector icon pipeline, stroke hinting). A cascade release is coming soon — your wgpu#291 is already merged on that side. Good catch on the re-dirty-during-paint race. The terminal emulator use case is exactly the kind of stress test that finds these edge cases. |
2 participants
The bug
A boundary re-dirtied while it is being recorded re-registers itself for the next
frame (
layer_tree.go, "if boundary re-dirtied, register it for next frame").draw()then calledClearDirtyBoundaries()at the end of the frame, which threwthat registration away.
That is unrecoverable rather than a dropped frame: the widget's own
sceneDirtyisstill true, so every later
InvalidateScenetakes the already-dirty O(1) guard andreturns without notifying the window. Nothing re-registers the boundary and it is
never painted again.
Who hits it
Any widget written from a goroutine other than the UI thread, on the first frame that
overlaps a write — and animated widgets that re-dirty during
Draware the same shape(the spinner case that comment in
recordBoundaryis about).Found in a terminal emulator: under continuous output it froze on screen within one
frame and stayed frozen after the output stopped, while the render loop kept being
woken 60 times a second and skipping every frame.
The fix
Clear the set before painting instead of after. The set is only the O(1) frame-skip
gate (
needsAnyWork) — painting walks the tree on each boundary's ownsceneDirtyand never reads it, but it writes to it. Clearing once the gate has been consumed
leaves anything registered during painting intact.
Test
TestDirtyBoundaryRegisteredDuringPaintSurvivesTheFramecovers the app-level contractthe fix depends on: a boundary that re-dirties during recording is still registered
after a paint pass that began with a cleared set.
To be straight about its limits — it pins the
appcontract, notdraw()'s callorder, so it would not by itself catch someone moving the clear back.
draw()has notest harness: it needs a live
gogpu.Context.go build ./...,go test ./...andgolangci-lint runare clean.