[Bugfix] Frame(cache_subtree=True) silently freezes its subtree's content: render_dirty is a write-once latch, so markContentDirty never propagates #368

New issue

Closed

opened 2026-07-13 03:05:36 +00:00 by john · 2 comments

john commented 2026-07-13 03:05:36 +00:00

Owner

Copy link

Found while fixing #364. Not grid-specific -- the repro below has no Grid in it at all.

Defect

UIDrawable::markContentDirty() (src/UIDrawable.cpp:~1090) only propagates up the parent chain conditionally:

void UIDrawable::markContentDirty() {
    bool was_dirty = render_dirty;
    render_dirty = true;
    composite_dirty = true;

    auto p = parent.lock();
    if (p && (!was_dirty || !p->render_dirty)) {   // <-- the latch
        p->markContentDirty();
    }
}

The guard is a legitimate optimization if render_dirty is reliably cleared once the drawable renders. It is not. A drawable that is not in render-texture mode never calls clearDirty() -- UIFrame::render only rebuilds-and-clears under if (use_render_texture && render_dirty) (src/UIFrame.cpp:165). So an ordinary nested Frame/Caption/Sprite sets render_dirty = true on its first mutation and it stays true for the object's whole life.

From then on, every subsequent mutation of that drawable has was_dirty == true, and if its parent is also permanently-dirty (same reason), (!was_dirty || !p->render_dirty) is false and the invalidation is never propagated to any ancestor. A caching ancestor above it (Frame(cache_subtree=True)) is never told, so it re-blits its stale composite.

Repro (no grid involved)

scene = mcrfpy.Scene("p"); scene.activate()
cache = mcrfpy.Frame(pos=(10,10), size=(300,300), cache_subtree=True)   # caching ancestor
scene.children.append(cache)
inner = mcrfpy.Frame(pos=(0,0), size=(280,280))                         # plain frame
cache.children.append(inner)
kid = mcrfpy.Frame(pos=(32,32), size=(48,48), fill_color=mcrfpy.Color(255,0,0))
inner.children.append(kid)

a = shot()                       # screenshot bytes
kid.x = 128.0
b = shot()   # b != a  -> updates   (first mutation propagates: was_dirty was False)
kid.fill_color = mcrfpy.Color(0,255,0)
c = shot()   # c == b  -> STALE     (second mutation is swallowed by the latch)

The first mutation gets through (the drawable was clean, so !was_dirty holds). Every mutation after it is silently dropped. That ordering is the tell: it is not "recolor doesn't invalidate," it is "nothing invalidates after the first time."

Why it has stayed hidden

It only manifests under a caching ancestor -- Frame(cache_subtree=True), or a GridView (which is why #364's test caught it). Without one, every drawable is re-drawn from scratch each frame and the missed invalidation costs nothing visible.

Fix directions

1. Clear the flag honestly: have every drawable clearDirty() at the end of its render(), not just render-texture frames. Then was_dirty means what the guard assumes. This is what #364 did for UIGridView (added clearDirty() after rasterization), which is why grid children now propagate correctly -- but it fixed only that one arm.
2. Drop the was_dirty half of the guard and keep only !p->render_dirty (still terminates: propagation stops at the first already-dirty ancestor). Cheaper to reason about, slightly more walking.

Prefer (1) -- it makes the flag's meaning true rather than working around it being false. But it needs a check that nothing else depends on render_dirty staying latched across frames.

Test

tests/regression/issue_364_grid_children_on_view_test.py case 5b covers the grid arm. This issue needs the Frame-only repro above as its own regression test, asserting screenshot bytes change on the second and third consecutive mutation of a nested child, not just the first.

Found while fixing #364. **Not grid-specific** -- the repro below has no Grid in it at all. ## Defect `UIDrawable::markContentDirty()` (`src/UIDrawable.cpp:~1090`) only propagates up the parent chain conditionally: ```cpp void UIDrawable::markContentDirty() { bool was_dirty = render_dirty; render_dirty = true; composite_dirty = true; auto p = parent.lock(); if (p && (!was_dirty || !p->render_dirty)) { // <-- the latch p->markContentDirty(); } } ``` The guard is a legitimate optimization *if* `render_dirty` is reliably cleared once the drawable renders. It is not. **A drawable that is not in render-texture mode never calls `clearDirty()`** -- `UIFrame::render` only rebuilds-and-clears under `if (use_render_texture && render_dirty)` (`src/UIFrame.cpp:165`). So an ordinary nested Frame/Caption/Sprite sets `render_dirty = true` on its first mutation and it stays true for the object's whole life. From then on, every subsequent mutation of that drawable has `was_dirty == true`, and if its parent is also permanently-dirty (same reason), `(!was_dirty || !p->render_dirty)` is `false` and **the invalidation is never propagated to any ancestor**. A caching ancestor above it (`Frame(cache_subtree=True)`) is never told, so it re-blits its stale composite. ## Repro (no grid involved) ```python scene = mcrfpy.Scene("p"); scene.activate() cache = mcrfpy.Frame(pos=(10,10), size=(300,300), cache_subtree=True) # caching ancestor scene.children.append(cache) inner = mcrfpy.Frame(pos=(0,0), size=(280,280)) # plain frame cache.children.append(inner) kid = mcrfpy.Frame(pos=(32,32), size=(48,48), fill_color=mcrfpy.Color(255,0,0)) inner.children.append(kid) a = shot() # screenshot bytes kid.x = 128.0 b = shot() # b != a -> updates (first mutation propagates: was_dirty was False) kid.fill_color = mcrfpy.Color(0,255,0) c = shot() # c == b -> STALE (second mutation is swallowed by the latch) ``` The first mutation gets through (the drawable was clean, so `!was_dirty` holds). Every mutation after it is silently dropped. That ordering is the tell: it is not "recolor doesn't invalidate," it is "*nothing* invalidates after the first time." ## Why it has stayed hidden It only manifests under a caching ancestor -- `Frame(cache_subtree=True)`, or a `GridView` (which is why #364's test caught it). Without one, every drawable is re-drawn from scratch each frame and the missed invalidation costs nothing visible. ## Fix directions 1. **Clear the flag honestly**: have every drawable `clearDirty()` at the end of its `render()`, not just render-texture frames. Then `was_dirty` means what the guard assumes. This is what #364 did for `UIGridView` (added `clearDirty()` after rasterization), which is why grid children now propagate correctly -- but it fixed only that one arm. 2. **Drop the `was_dirty` half of the guard** and keep only `!p->render_dirty` (still terminates: propagation stops at the first already-dirty ancestor). Cheaper to reason about, slightly more walking. Prefer (1) -- it makes the flag's meaning true rather than working around it being false. But it needs a check that nothing else depends on `render_dirty` staying latched across frames. ## Test `tests/regression/issue_364_grid_children_on_view_test.py` case 5b covers the grid arm. This issue needs the Frame-only repro above as its own regression test, asserting screenshot bytes change on the **second and third** consecutive mutation of a nested child, not just the first.

john added the

system:ui-hierarchy

Bugfix

priority:tier1-active

system:rendering

labels 2026-07-13 03:05:36 +00:00

john referenced this issue 2026-07-13 03:06:38 +00:00

[Refactoring] Let perspective-enabled grids take the render early-out (track FOV/perspective_map changes in content-generation) #352

john commented 2026-07-13 11:28:19 +00:00

Author

Owner

Copy link

Correction: the original writeup mis-stated the shape of this bug

Re-probed against feat/355-grid-input-revival (which does not touch markContentDirty — confirmed pre-existing on master). Two corrections:

1. It is NOT "nothing invalidates after the first time." Position changes still propagate, because x = ... routes through markCompositeDirty(), a different function with an unconditional push. Only markContentDirty() latches. My repro's third step was a move, which is why it appeared to recover:

mutation 1 (move)     propagated: True
mutation 2 (recolor)  propagated: False
mutation 3 (move)     propagated: True   <-- I read this as "recovers". It doesn't; moves were never broken.

2. It is worse than "the second mutation is dropped." Under a caching ancestor, content mutations NEVER propagate — not even the first.

recolor 1 -> (0,255,0)   : propagated=False
recolor 2 -> (0,0,255)   : propagated=False
recolor 3 -> (255,255,0) : propagated=False
recolor 4 -> (255,0,255) : propagated=False
caption text 1 -> 'two'   : propagated=False
caption text 2 -> 'three' : propagated=False
caption text 3 -> 'four'  : propagated=False

(By the time the first recolor lands, render_dirty has already latched true from construction/append, so !was_dirty is already false.)

So the accurate statement of the defect is: Frame(cache_subtree=True) silently freezes the content of its entire subtree. Text never updates, colors never change, sprite indices never change. Only movement works. That is a considerably bigger deal than the original title suggests.

Root cause, now proven rather than inferred

clearDirty() is called from exactly one place in the entire codebase:

$ grep -rn "clearDirty()" src/ | grep -v "void UIDrawable"
src/UIDrawable.h:344:    void clearDirty() { render_dirty = false; composite_dirty = false; }
src/UIGridView.cpp:423:    clearDirty();

That one call site is the one #364 added. UIFrame, UICaption, UISprite, UILine, UICircle, UIArc never clear their dirty flags at all — so render_dirty is a write-once latch, and the was_dirty guard in markContentDirty is reasoning off a flag that is permanently true.

This also means the grid branch has left the codebase asymmetric: UIGridView is now the only drawable that honours the dirty contract. Fix direction (1) from the original issue (clear the flag honestly in every render()) is now clearly the right one — it makes the flag's meaning true everywhere instead of true in one class.

## Correction: the original writeup mis-stated the shape of this bug Re-probed against `feat/355-grid-input-revival` (which does **not** touch `markContentDirty` — confirmed pre-existing on master). Two corrections: **1. It is NOT "nothing invalidates after the first time."** Position changes still propagate, because `x = ...` routes through `markCompositeDirty()`, a *different* function with an unconditional push. Only `markContentDirty()` latches. My repro's third step was a move, which is why it appeared to recover: ``` mutation 1 (move) propagated: True mutation 2 (recolor) propagated: False mutation 3 (move) propagated: True <-- I read this as "recovers". It doesn't; moves were never broken. ``` **2. It is worse than "the second mutation is dropped." Under a caching ancestor, content mutations NEVER propagate — not even the first.** ``` recolor 1 -> (0,255,0) : propagated=False recolor 2 -> (0,0,255) : propagated=False recolor 3 -> (255,255,0) : propagated=False recolor 4 -> (255,0,255) : propagated=False caption text 1 -> 'two' : propagated=False caption text 2 -> 'three' : propagated=False caption text 3 -> 'four' : propagated=False ``` (By the time the first recolor lands, `render_dirty` has already latched true from construction/append, so `!was_dirty` is already false.) So the accurate statement of the defect is: **`Frame(cache_subtree=True)` silently freezes the *content* of its entire subtree.** Text never updates, colors never change, sprite indices never change. Only movement works. That is a considerably bigger deal than the original title suggests. ## Root cause, now proven rather than inferred `clearDirty()` is called from **exactly one place in the entire codebase**: ``` $ grep -rn "clearDirty()" src/ | grep -v "void UIDrawable" src/UIDrawable.h:344: void clearDirty() { render_dirty = false; composite_dirty = false; } src/UIGridView.cpp:423: clearDirty(); ``` That one call site is the one #364 added. `UIFrame`, `UICaption`, `UISprite`, `UILine`, `UICircle`, `UIArc` never clear their dirty flags at all — so `render_dirty` is a **write-once latch**, and the `was_dirty` guard in `markContentDirty` is reasoning off a flag that is permanently true. This also means the grid branch has left the codebase **asymmetric**: `UIGridView` is now the only drawable that honours the dirty contract. Fix direction (1) from the original issue (clear the flag honestly in every `render()`) is now clearly the right one — it makes the flag's meaning true everywhere instead of true in one class.

john changed title from [Bugfix] markContentDirty's was_dirty short-circuit latches: a drawable that never clears render_dirty stops propagating invalidation forever to [Bugfix] Frame(cache_subtree=True) silently freezes its subtree's content: render_dirty is a write-once latch, so markContentDirty never propagates 2026-07-13 11:28:28 +00:00

john commented 2026-07-13 11:40:44 +00:00

Author

Owner

Copy link

Fixed on feat/355-grid-input-revival (9d67e33)

markContentDirty()'s walk up the parent chain is now unconditional, matching markCompositeDirty().

Why not fix direction (1) or (2) from the original issue

Both suggestions in the issue body turn out to be wrong, and it's worth recording why:

Direction (2) — "drop the was_dirty half, keep !p->render_dirty" — does not fix the bug at all. The parent in the failing case is an ordinary Frame whose render_dirty has itself latched true. So !p->render_dirty is false and the walk still stops dead. I'd have shipped a no-op.

Direction (1) — "clear the flag honestly in every render()" — works, but buys a shorter walk at the price of an invariant that breaks silently the day someone adds a drawable type and forgets to call clearDirty(). That is precisely the failure that produced this bug in the first place: UIGridView remembered (via #364) and six other classes did not.

Unconditional propagation cannot be broken that way. The correctness of the render cache no longer depends on every drawable's render path remembering to do bookkeeping.

The cost is real but lands where it doesn't matter

200k content mutations, best of 3:

depth	1	4	8	16
before	82ns	86ns	82ns	87ns
after	76ns	114ns	175ns	295ns

The old build's flatness with depth is the defect itself — it was flat because it wasn't walking. (That same build fails 10 of the new test's assertions.)

Entity movement — the actual hot path — is completely unchanged: 164ns/move either way. Position changes already went through markCompositeDirty, which has always walked unconditionally. So the added cost applies only to content mutations (colour, text, sprite index), which are orders of magnitude rarer than movement in a real frame. Crucible confirms no regression (step_swarm 12.9 → 10.7ms, entity_churn 36.3 → 31.5ms, within noise).

Test

tests/regression/issue_368_cache_subtree_content_dirty_test.py — 13 assertions. Every consecutive recolour and caption edit must repaint through the cache, at depth 1 and at depth 5.

The load-bearing one is case 4a: idle frames under a cache must stay byte-identical. Without it, "mark everything dirty every frame" would pass every other assertion in the file while making cache_subtree worthless.

Suite 329/329.

## Fixed on `feat/355-grid-input-revival` (9d67e33) `markContentDirty()`'s walk up the parent chain is now **unconditional**, matching `markCompositeDirty()`. ### Why not fix direction (1) or (2) from the original issue Both suggestions in the issue body turn out to be wrong, and it's worth recording why: **Direction (2) — "drop the `was_dirty` half, keep `!p->render_dirty`" — does not fix the bug at all.** The parent in the failing case is an ordinary Frame whose `render_dirty` has *itself* latched true. So `!p->render_dirty` is false and the walk still stops dead. I'd have shipped a no-op. **Direction (1) — "clear the flag honestly in every `render()`" — works, but buys a shorter walk at the price of an invariant that breaks silently the day someone adds a drawable type and forgets to call `clearDirty()`.** That is precisely the failure that produced this bug in the first place: `UIGridView` remembered (via #364) and six other classes did not. Unconditional propagation cannot be broken that way. The correctness of the render cache no longer depends on every drawable's render path remembering to do bookkeeping. ### The cost is real but lands where it doesn't matter 200k content mutations, best of 3: | depth | 1 | 4 | 8 | 16 | |---|---|---|---|---| | before | 82ns | 86ns | 82ns | 87ns | | after | 76ns | 114ns | 175ns | 295ns | The old build's **flatness with depth is the defect itself** — it was flat because it wasn't walking. (That same build fails 10 of the new test's assertions.) **Entity movement — the actual hot path — is completely unchanged: 164ns/move either way.** Position changes already went through `markCompositeDirty`, which has always walked unconditionally. So the added cost applies only to *content* mutations (colour, text, sprite index), which are orders of magnitude rarer than movement in a real frame. Crucible confirms no regression (`step_swarm` 12.9 → 10.7ms, `entity_churn` 36.3 → 31.5ms, within noise). ### Test `tests/regression/issue_368_cache_subtree_content_dirty_test.py` — 13 assertions. Every consecutive recolour and caption edit must repaint through the cache, at depth 1 and at depth 5. The load-bearing one is case 4a: **idle frames under a cache must stay byte-identical.** Without it, "mark everything dirty every frame" would pass every other assertion in the file while making `cache_subtree` worthless. Suite 329/329.

john referenced this issue from a commit 2026-07-13 11:44:13 +00:00

fix(grid): overlay children belong to the GridView, not the shared GridData

john referenced this issue from a commit 2026-07-13 11:44:13 +00:00

fix(render): content invalidation propagates again; clear hover on focus loss

john closed this issue 2026-07-13 11:44:13 +00:00

john referenced this issue from a commit 2026-07-13 11:44:13 +00:00

Merge branch 'feat/355-grid-input-revival': revive Grid input, split GridData from GridView

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

master

feat/381-snippet-shots

feature/issues-317-319-verify-bugs

feature/issue-316-sparse-perspective

feature/issue-314-doc-followthrough

feature/api-freeze-313-314

threedee

emscripten-mcrogueface

rogueliketutorial25

alpha_presentable

alpha_streamline_2

alpha_streamline_1

interpreter_mode

grid_entity_integration

reprs_and_member_names

break_up_ui_h

standardize_font_handling

standardize_vector_handling

standardize_color_handling

standardize_texture_handling

raii_pyobjects

rebase-for-7DRL_Feb-2024

engjam_uicollection

engjam_ui_overhaul

0.2.3-prerelease-7drl2026-emscripten

Labels

Clear labels   

Alpha Release Requirement

This issue is a blocker to 0.1 Alpha release of McRogueFace.

  

Bugfix

  

Demo Target

Functionality to demonstrate; Depends on new features, but this issue isn't for writing them.

  

Documentation

  

Major Feature

Significant time and effort required.

  

Minor Feature

Some effort required to create or overhaul functionality.

  

priority:tier1-active

Current development focus - critical path to v1.0

  

priority:tier2-foundation

Important foundation work - not blocking v1.0

  

priority:tier3-future

Future features - deferred until after v1.0

  

priority:tier4-deferred

  

Refactoring & Cleanup

no new functionality, just improving the codebase.

  

system:animation

Animation and property interpolation

  

system:documentation

Documentation infrastructure

  

system:grid

Grid system and spatial containers

  

system:input

Input handling and events

  

system:performance

Performance optimization and profiling

  

system:procgen

Procedural Generation (Noise, BSP, Heightmap)

  

system:python-binding

Python/C++ binding layer

  

system:rendering

Rendering pipeline and visuals

  

system:ui-hierarchy

UI component hierarchy and composition

  

Tiny Feature

quick and easy - a few lines or with little interconnection around the codebase

  

workflow:blocked

Blocked by other work - waiting on dependencies

  

workflow:needs-benchmark

Needs performance testing and benchmarks

  

workflow:needs-documentation

Needs documentation before or after implementation

No labels

Alpha Release Requirement

Bugfix

Demo Target

Documentation

Major Feature

Minor Feature

priority:tier1-active

priority:tier2-foundation

priority:tier3-future

priority:tier4-deferred

Refactoring & Cleanup

system:animation

system:documentation

system:grid

system:input

system:performance

system:procgen

system:python-binding

system:rendering

system:ui-hierarchy

Tiny Feature

workflow:blocked

workflow:needs-benchmark

workflow:needs-documentation

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

john/McRogueFace#368

No description provided.