john/McRogueFace
1
0
Fork 0
Code Issues 35 Pull requests Projects Releases Wiki Activity

[Bugfix] Grid children's dirty propagation dead-ends at the internal UIGrid, never reaching the GridView or its caching ancestors #364

New issue
Closed
opened 2026-07-12 22:05:04 +00:00 by john · 2 comments
john commented 2026-07-12 22:05:04 +00:00
Owner
Copy link

Found by adversarial review of the #355/#359 branch (feat/355-grid-input-revival, 85e4bbe). Reported as minor; not fixed there.

Defect

UIGrid::get_children (src/UIGridPyProperties.cpp ~:350) parents a grid's child drawables to the internal _GridData/UIGrid object:

o->owner = self->data;   // the UIGrid, which is NOT in the scene graph

UIDrawable::markCompositeDirty() walks up the owner chain to invalidate caching ancestors. For a grid child that walk reaches the internal UIGrid — which no scene ever contains — and stops. It never reaches the UIGridView that actually renders the child, nor any Frame(use_render_texture=True) above it.

#359's GridData::views registry fixed exactly this push path for GridData mutations (entity moves, layer edits, via GridData::markDirty), but grid children are UIDrawables that dirty themselves through UIDrawable::markCompositeDirty(), which knows nothing about the registry.

Consequence

Mutating a grid child (moving a speech bubble, changing a marker's color) may not invalidate the render caches above it, so the change can fail to appear until something else forces a redraw. This is latent rather than always-visible because #351's early-out polls content_generation — which child mutations do not bump.

Fix

Either (a) parent grid children to the UIGridView rather than the internal UIGrid — which is the architecturally correct answer and may fall out of #361, or (b) have the UIGrid's markCompositeDirty forward into GridData::views the way GridData::markCompositeDirty already does.

Prefer (a). Check whether #361 (_GridData sheds UIDrawable) subsumes this entirely — if children can no longer be parented to a non-drawable, the dead-end cannot exist.

Test

Append a Frame to grid.children inside a Frame(use_render_texture=True), render, mutate the child, render again, and assert the composite is updated.

Found by adversarial review of the #355/#359 branch (`feat/355-grid-input-revival`, 85e4bbe). Reported as minor; not fixed there. ## Defect `UIGrid::get_children` (`src/UIGridPyProperties.cpp` ~:350) parents a grid's child drawables to the **internal `_GridData`/UIGrid object**: ```cpp o->owner = self->data; // the UIGrid, which is NOT in the scene graph ``` `UIDrawable::markCompositeDirty()` walks *up* the `owner` chain to invalidate caching ancestors. For a grid child that walk reaches the internal UIGrid — which no scene ever contains — and stops. It never reaches the `UIGridView` that actually renders the child, nor any `Frame(use_render_texture=True)` above it. #359's `GridData::views` registry fixed exactly this push path for **GridData mutations** (entity moves, layer edits, via `GridData::markDirty`), but grid children are `UIDrawable`s that dirty themselves through `UIDrawable::markCompositeDirty()`, which knows nothing about the registry. ## Consequence Mutating a grid child (moving a speech bubble, changing a marker's color) may not invalidate the render caches above it, so the change can fail to appear until something else forces a redraw. This is latent rather than always-visible because #351's early-out polls `content_generation` — which child mutations do **not** bump. ## Fix Either (a) parent grid children to the `UIGridView` rather than the internal UIGrid — which is the architecturally correct answer and may fall out of #361, or (b) have the UIGrid's `markCompositeDirty` forward into `GridData::views` the way `GridData::markCompositeDirty` already does. Prefer (a). Check whether #361 (`_GridData` sheds `UIDrawable`) subsumes this entirely — if children can no longer be parented to a non-drawable, the dead-end cannot exist. ## Test Append a Frame to `grid.children` inside a `Frame(use_render_texture=True)`, render, mutate the child, render again, and assert the composite is updated.
john commented 2026-07-13 00:10:00 +00:00
Author
Owner
Copy link

Correction: fix (a) is not available, and this is a #361 prerequisite (2026-07-12)

Analysis session on feat/355-grid-input-revival. Three corrections to the body above.

1. The mechanism is parent, not owner. The body says markCompositeDirty() "walks up the owner chain." It doesn't — it walks parent (UIDrawable.cpp:1082), and it is not virtual (UIDrawable.h:336), so GridData::markCompositeDirty (which does fan out to views) can never be reached from a parent-chain walk. The dead-end is real; the description of it was imprecise. The chain is: UIGrid::get_children sets the collection's owner to the internal UIGrid (UIGridPyProperties.cpp:284) → UICollection::append calls setParent(owner) (UICollection.cpp:657) → the child's parent is the internal UIGrid, whose own parent is null.

2. Fix (a) — "parent grid children to the GridView" — is impossible, so this issue is NOT subsumed by #361. children lives on GridData (GridData.h:130), beside entities: shared world content in grid-world coordinates (#360). A UIDrawable has exactly one parent weak_ptr, but a GridData may have N views. Whichever view the child was appended through would become its parent arbitrarily, and if that view died the parent would dangle while the child still rendered in the remaining views. Fix (b) is the answer — children stay owned by GridData, and the upward push fans out through the views registry, exactly as GridData::markCompositeDirty already does for entity moves.

This inverts the dependency: #361 does not subsume this issue, it is blocked on it. Once UIGrid sheds UIDrawable, setParent(internal UIGrid) will not typecheck, so #361 cannot land without answering this first. Full design note (unified ParentRef variant replacing the ad-hoc parent + parent_scene fields) is in the #361 comment thread.

3. Scope is narrower than filed, but there's a hidden cost. The #351 early-out already disables itself whenever a grid has any children (UIGridView.cpp:189: && (!grid_data->children || grid_data->children->empty())), so the child's own pixels are never stale — a grid with children fully re-renders every frame regardless. The dead-end only bites a caching ancestor above the view (a Frame(use_render_texture=True) wrapping the grid), which re-blits its stale composite. That matches the test recipe in the body exactly.

The flip side: any grid with even one child currently pays a permanent full re-render. That's the #352 carve-out. Precise child dirty-tracking (this issue's fix (b), plus bumping content_generation on child mutation) would let the carve-out be removed and restore the early-out for grids with children — which is a real performance win, not just a correctness fix.

Blocking #361.

## Correction: fix (a) is not available, and this is a #361 prerequisite (2026-07-12) Analysis session on `feat/355-grid-input-revival`. Three corrections to the body above. **1. The mechanism is `parent`, not `owner`.** The body says `markCompositeDirty()` "walks up the `owner` chain." It doesn't — it walks `parent` (`UIDrawable.cpp:1082`), and it is **not virtual** (`UIDrawable.h:336`), so `GridData::markCompositeDirty` (which *does* fan out to `views`) can never be reached from a parent-chain walk. The dead-end is real; the description of it was imprecise. The chain is: `UIGrid::get_children` sets the collection's `owner` to the internal UIGrid (`UIGridPyProperties.cpp:284`) → `UICollection::append` calls `setParent(owner)` (`UICollection.cpp:657`) → the child's `parent` is the internal UIGrid, whose own `parent` is null. **2. Fix (a) — "parent grid children to the GridView" — is impossible, so this issue is NOT subsumed by #361.** `children` lives on `GridData` (`GridData.h:130`), beside `entities`: shared world content in grid-world coordinates (#360). A `UIDrawable` has exactly one `parent` weak_ptr, but a `GridData` may have N views. Whichever view the child was appended through would become its parent arbitrarily, and if that view died the parent would dangle while the child still rendered in the remaining views. **Fix (b) is the answer** — children stay owned by GridData, and the upward push fans out through the `views` registry, exactly as `GridData::markCompositeDirty` already does for entity moves. This inverts the dependency: #361 does not subsume this issue, it is **blocked on** it. Once `UIGrid` sheds `UIDrawable`, `setParent(internal UIGrid)` will not typecheck, so #361 cannot land without answering this first. Full design note (unified `ParentRef` variant replacing the ad-hoc `parent` + `parent_scene` fields) is in the #361 comment thread. **3. Scope is narrower than filed, but there's a hidden cost.** The #351 early-out already disables itself whenever a grid has any children (`UIGridView.cpp:189`: `&& (!grid_data->children || grid_data->children->empty())`), so the child's own pixels are never stale — a grid with children fully re-renders every frame regardless. The dead-end only bites a **caching ancestor above the view** (a `Frame(use_render_texture=True)` wrapping the grid), which re-blits its stale composite. That matches the test recipe in the body exactly. The flip side: **any grid with even one child currently pays a permanent full re-render.** That's the #352 carve-out. Precise child dirty-tracking (this issue's fix (b), plus bumping `content_generation` on child mutation) would let the carve-out be removed and restore the early-out for grids with children — which is a real performance win, not just a correctness fix. Blocking #361.
john commented 2026-07-13 03:05:12 +00:00
Author
Owner
Copy link

Resolved: children move to the GridView. Fix (a) is back on, by changing what "children" means (2026-07-13)

Decision (repo owner, 2026-07-13): children live on the GridView; entities stay on GridData.

My earlier comment ruled out fix (a) ("parent grid children to the GridView") on the grounds that children lives on GridData, which may have N views, so no single view can be the parent. That reasoning was sound but it took children-on-GridData as given. It isn't. Move the collection to the view and the objection evaporates: each child belongs to exactly one view, so its parent is a real scene-graph drawable and the dead-end cannot be expressed.

Why this is the right semantics, not just a convenient one

A drawable child is an overlay painted over the map through one camera -- a speech bubble, a marker, a range indicator. It occupies no cell, nothing collides with it, the turn manager cannot see it, and it is already drawn on top of layers and entities and already unaffected by FOV. Entities are the world contents: they live in cells, they hit walkable/transparent, pathfinding and grid.step() act on them, and every camera must show the same ones. So entities are shared and children are per-view. The engine has been treating children as overlays all along; only the ownership disagreed.

This also kills the duplicate-annotation worry in reverse: a minimap at zoom=0.25 should not inherit the main view's speech bubbles (they'd render as illegible smears). Per-view children is the behavior you want anyway.

What it resolves

  • This issue, structurally. A child's parent is the UIGridView, so markCompositeDirty()/markContentDirty() walk up through it to any Frame(cache_subtree=True) exactly as a Frame's child does. No views fan-out, no grid_owner field.
  • #361's Q1 (global_position ambiguous under N views). One child, one view, one camera, one screen pixel. The ambiguity was purely a consequence of children living on shared data. (The transform itself is still not applied -- see the follow-up issue below -- but the question of what the answer should be is now settled.)
  • The ParentRef variant becomes optional cleanup rather than a prerequisite for #361. GridData holds no UIDrawable at all now, so once it sheds UIDrawable there is no non-drawable-parent case to represent. #361 is unblocked.

Migration cost: near zero

mcrfpy.Grid is the UIGridView (same PyTypeObject), so grid.children.append(bubble) is unchanged for every single-view script in the repo. The API-surface snapshot confirms it: children gains on Grid/GridView, loses on the internal _GridData, delegation drops 62 -> 61. The public surface of mcrfpy.Grid is identical.

What changes: entity.grid.children will not exist once #361 lands (entity.grid -> GridData) -- same accepted break, same reason, as center_camera. And child.parent = <_GridData> now raises TypeError instead of silently working.

The one thing given up: a decorative drawable conceptually baked into the map (a zone-name Caption that should survive save/load) is now view-scoped. Such a thing wants to be a layer or an entity. Flagged as a decision, not discovered later.

Fix (committed)

  • GridData::children / children_need_sort -> UIGridView (GridData.h:128, UIGridView.h)
  • UIGrid::get_children -> UIGridView::get_children; the collection's owner is the view, so UICollection::append already parents correctly and already calls owner->markContentDirty()
  • UIDrawable::removeFromParent drops its UIGRID arm entirely (down to Frame / GridView / scene)
  • UIDrawable::set_parent no longer accepts a _GridData
  • UIGrid::render's child-drawing block deleted (the ghost path has no children); UIGrid::resize's aligned-child loop moved to UIGridView::resize -- it was never ported when the view was split out, so a resized grid never re-anchored aligned children
  • PyScene hover walk, mcrfpy.find()/findAll(), and the ImGui scene explorer read the view's children. find no longer dedups children by GridData (two views hold different children); entities still dedup via visited_grids.

A second, real bug this exposed

UIGridView::render's #351 early-out never consulted its own render_dirty. It got away with it because every content mutation bumps content_generation -- and the children carve-out (!children->empty() -> always full render) papered over the rest. But removing the last child lifts the carve-out, at which point every other input is unchanged and the view re-blits a cached raster with the removed child still baked into it. Fixed by adding !render_dirty to can_skip and calling clearDirty() after a full rasterization.

That also means the view now has a precise per-child dirty signal, so the !children->empty() carve-out can be dropped -- restoring the early-out for grids that have children, which is the performance half of my earlier comment. Left in place here (conservative = always correct) and handed to #352.

Test

tests/regression/issue_364_grid_children_on_view_test.py -- 17 assertions, all failing beforehand in the ways predicted:

  • child's parent is the Grid view, not _GridData
  • children are per-view; entities are shared across views
  • _GridData rejects being a parent and exposes no children
  • the load-bearing case: a grid inside Frame(cache_subtree=True); move / recolor / remove a grid child and assert the screenshot bytes change. All three were byte-identical (stale composite) before the fix. Plus an idle-stable pair, so the skip path is not merely disabled.

Suite 326/326.

## Resolved: children move to the GridView. Fix (a) is back on, by changing what "children" means (2026-07-13) Decision (repo owner, 2026-07-13): **children live on the `GridView`; entities stay on `GridData`.** My earlier comment ruled out fix (a) ("parent grid children to the GridView") on the grounds that `children` lives on `GridData`, which may have N views, so no single view can be the parent. That reasoning was sound but it took `children`-on-`GridData` as given. It isn't. Move the collection to the view and the objection evaporates: each child belongs to exactly one view, so its parent is a real scene-graph drawable and the dead-end cannot be expressed. ### Why this is the right semantics, not just a convenient one A drawable child is an **overlay** painted over the map through one camera -- a speech bubble, a marker, a range indicator. It occupies no cell, nothing collides with it, the turn manager cannot see it, and it is already drawn *on top of* layers and entities and already unaffected by FOV. **Entities** are the world contents: they live in cells, they hit `walkable`/`transparent`, pathfinding and `grid.step()` act on them, and every camera must show the same ones. So entities are shared and children are per-view. The engine has been treating children as overlays all along; only the ownership disagreed. This also kills the duplicate-annotation worry in reverse: a minimap at `zoom=0.25` should *not* inherit the main view's speech bubbles (they'd render as illegible smears). Per-view children is the behavior you want anyway. ### What it resolves - **This issue**, structurally. A child's `parent` is the `UIGridView`, so `markCompositeDirty()`/`markContentDirty()` walk up through it to any `Frame(cache_subtree=True)` exactly as a Frame's child does. No `views` fan-out, no `grid_owner` field. - **#361's Q1** (`global_position` ambiguous under N views). One child, one view, one camera, one screen pixel. The ambiguity was purely a consequence of children living on shared data. (The transform itself is still not applied -- see the follow-up issue below -- but the question of *what the answer should be* is now settled.) - **The `ParentRef` variant becomes optional cleanup rather than a prerequisite for #361.** `GridData` holds no `UIDrawable` at all now, so once it sheds `UIDrawable` there is no non-drawable-parent case to represent. #361 is unblocked. ### Migration cost: near zero `mcrfpy.Grid` **is** the `UIGridView` (same `PyTypeObject`), so `grid.children.append(bubble)` is unchanged for every single-view script in the repo. The API-surface snapshot confirms it: `children` gains on `Grid`/`GridView`, loses on the internal `_GridData`, delegation drops 62 -> 61. The public surface of `mcrfpy.Grid` is identical. What changes: `entity.grid.children` will not exist once #361 lands (`entity.grid` -> `GridData`) -- same accepted break, same reason, as `center_camera`. And `child.parent = <_GridData>` now raises `TypeError` instead of silently working. The one thing given up: a decorative drawable conceptually baked into the *map* (a zone-name Caption that should survive save/load) is now view-scoped. Such a thing wants to be a layer or an entity. Flagged as a decision, not discovered later. ### Fix (committed) - `GridData::children` / `children_need_sort` -> `UIGridView` (`GridData.h:128`, `UIGridView.h`) - `UIGrid::get_children` -> `UIGridView::get_children`; the collection's `owner` is the view, so `UICollection::append` already parents correctly and already calls `owner->markContentDirty()` - `UIDrawable::removeFromParent` drops its `UIGRID` arm entirely (down to Frame / GridView / scene) - `UIDrawable::set_parent` no longer accepts a `_GridData` - `UIGrid::render`'s child-drawing block deleted (the ghost path has no children); `UIGrid::resize`'s aligned-child loop moved to `UIGridView::resize` -- it was never ported when the view was split out, so a resized grid never re-anchored aligned children - `PyScene` hover walk, `mcrfpy.find()/findAll()`, and the ImGui scene explorer read the view's children. `find` no longer dedups children by GridData (two views hold different children); entities still dedup via `visited_grids`. ### A second, real bug this exposed `UIGridView::render`'s #351 early-out never consulted its own `render_dirty`. It got away with it because every content mutation bumps `content_generation` -- and the children carve-out (`!children->empty()` -> always full render) papered over the rest. But **removing the last child lifts the carve-out**, at which point every other input is unchanged and the view re-blits a cached raster with the removed child still baked into it. Fixed by adding `!render_dirty` to `can_skip` and calling `clearDirty()` after a full rasterization. That also means the view now has a *precise* per-child dirty signal, so the `!children->empty()` carve-out can be dropped -- restoring the early-out for grids that have children, which is the performance half of my earlier comment. Left in place here (conservative = always correct) and handed to **#352**. ### Test `tests/regression/issue_364_grid_children_on_view_test.py` -- 17 assertions, all failing beforehand in the ways predicted: - child's parent is the Grid view, not `_GridData` - children are per-view; entities are shared across views - `_GridData` rejects being a parent and exposes no `children` - **the load-bearing case**: a grid inside `Frame(cache_subtree=True)`; move / recolor / remove a grid child and assert the screenshot bytes change. All three were byte-identical (stale composite) before the fix. Plus an idle-stable pair, so the skip path is not merely disabled. Suite 326/326.
john closed this issue 2026-07-13 11:44:13 +00:00
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
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#361 [Refactor] Complete #252: GridData becomes a public data type, GridView is the only drawable, Grid becomes an alias
john/McRogueFace
Reference
john/McRogueFace#364
No description provided.