[Refactor] Complete #252: GridData becomes a public data type, GridView is the only drawable, Grid becomes an alias #361

Closed
opened 2026-07-12 19:20:28 +00:00 by john · 4 comments
Owner

The gap

#252's design (docs/GRID_ENTITY_OVERHAUL_ROADMAP.md, Phase 4) states plainly:

Grid is NOT a UIDrawable. It has no position, size, or rendering.
GridView IS a UIDrawable. It references a Grid and renders it.

What actually shipped is the compat-shim half. The Python name moved to the view (PyUIGridViewType.tp_name == "mcrfpy.Grid"; the old type became internal mcrfpy._GridData), but the C++ class never gave up its drawable half:

// src/UIGrid.h:36
class UIGrid: public UIDrawable, public GridData

UIGrid still owns render(), click_at(), get_bounds(), box, ptex, its own center_x/center_y/zoom/camera_rotation, its own perspective_entity, and its own render textures — a complete second, stale copy of the camera that no longer corresponds to anything the user sees.

The bare-_GridData render path is real, and it is maintained

This is not merely vestigial. UIGrid::render() is alive (src/UIGrid.cpp:97), and scene.children.append(view.grid) draws a second, complete copy of the grid through UIGrid's own camera, frozen at construction values. Pan or zoom the real view and the ghost does not move.

Worse, the data layer actively keeps that ghost alive:

// src/GridData.cpp:12
void GridData::markDirty() {
    content_generation++;
    static_cast<UIGrid*>(this)->UIDrawable::markDirty();   // <-- feeding the ghost
    if (auto view = owning_view.lock()) view->markDirty();
}

and GridData.h:149 says so outright: "covering both render paths (a bare _GridData rendered directly, and the normal GridView)." A bare _GridData rendered directly is a supported path today. That is the legacy behavior we must not carry into 1.0.

Why this is not cosmetic

The seam is actively producing bugs. #355, #357, #358 are all the same shape: code inspects a scene child for derived_type() == UIGRID, and it is dead in every case because scene children are always UIGRIDVIEW.

Worse, UIGridView::init_with_data (src/UIGridView.cpp:566-613) constructs a full _GridData Python object, steals its GridData sub-object via an aliasing shared_ptr, copies the rendering state out, and Py_DECREFs the wrapper. So every GridData in the engine is physically the base sub-object of a hidden UIGrid whose UIDrawable half — serial_number, is_python_subclass, click_callable — belongs to a discarded wrapper. That's why UIGrid::fireCellClick's PythonObjectCache::lookup(this->serial_number) can never find a user's Grid subclass: it looks up an object that no longer exists.

Resolution (decided 2026-07-12)

Semantically, GridData is a map, not a widget — closer to a HeightMap or a DiscreteMap than to anything on screen. Only GridView is a widget. Grid is a legacy holdover meaning "a GridData and a GridView together."

1. UIGrid sheds UIDrawable; the data type goes public

GridData becomes a first-class, standalone-constructible public type (mcrfpy.GridData), not the internal mcrfpy._GridData. It has no position, no size, no camera, no render().

This ordering is forced, not preferred. Today GridData is never independently heap-allocated (always a UIGrid base sub-object — the comment at GridData.cpp:10 relies on it), so static_cast<UIGrid*>(this) in markDirty is currently valid. A public GridData() constructor makes that downcast undefined behavior the first time a user allocates one standalone. Shedding UIDrawable is therefore a prerequisite for exposing GridData, not cleanup that follows it.

2. Entity.grid returns the GridData, not the view

UIEntity::get_grid (src/UIEntity.cpp:816-860) currently returns the owning GridView and falls back to a bare _GridData wrapper when there isn't one. It must return the GridData.

This is a user-visible break, and it is accepted pre-1.0. Rationale (repo owner): "Entities can be in grids that aren't visible at all, so we have to accept that change." There may be zero views over an entity's grid, or several — so "the grid an entity is in" simply is not a camera. Code that wants the camera reaches the GridView from the scene, or holds the view it created; code that wants the data can also reach it through the view (view.grid).

Migration: player.grid.center_camera(player.pos)view.center_camera(player.pos) on the view actually being rendered. This was always ill-defined through the entity once N views were possible.

3. owning_view is deleted (resolves #359 by removal)

GridData::owning_view is a single weak_ptr, which is structurally wrong the moment two views share one GridData — the entire premise of #252. It has exactly two consumers:

  • UIEntity::get_grid — removed by (2) above.
  • GridData::markDirty/markCompositeDirty notifying the view's render cache — already redundant with content_generation, which views poll via last_content_gen (#351).

With both gone, the back-pointer has no readers and is deleted. #359 is resolved by removal, not by wiring it up.

4. Grid becomes an alias, and is NOT deprecated

mcrfpy.Grid is already PyUIGridViewType — the same PyTypeObject, just with tp_name = "mcrfpy.Grid". The legacy hazard is the C++ dualism, not the spelling.

  • Rename the canonical tp_name to mcrfpy.GridView.
  • Export mcrfpy.Grid as an alias bound to the same type object (not a subclass — so isinstance works both directions and there is no MRO to explain).
  • No DeprecationWarning. Once UIGrid is pure data, Grid(grid_size=...) is exactly "a GridView that creates its own GridData" — a constructor overload, not legacy behavior. Warning on it would fire in every script, demo, and game in the repo to buy nothing.
  • GridView holds shared_ptr<GridData>, so the data outlives any Python referent to it. No lifetime work needed.

Resulting surface:

data = mcrfpy.GridData(grid_size=(80, 40))          # pure data; no position, never rendered
view = mcrfpy.GridView(grid=data, pos=(0,0), size=(800,400))
mini = mcrfpy.GridView(grid=data, pos=(820,0), size=(160,80))   # second camera, same map
g    = mcrfpy.Grid(grid_size=(80, 40))              # sugar: makes its own GridData
g.grid                                              # -> GridData
entity.grid                                         # -> GridData (NOT a view)
scene.children.append(data)                         # -> TypeError

Scope

  • src/UIDrawable.cpp — ~14 case PyObjectsEnum::UIGRID: arms in the getset/parent/shader/click switches (lines 31, 56, 262, 317, 1349, 1394, 1595, 1640, 1686, 1731, 1786, 1831, plus 1010 and 2198)
  • src/Animation.cpp:577 — UIGRID animation target arm
  • src/UICollection.cpp:72, 166, 1134 — conversion, append (must now reject with TypeError), stats
  • src/ImGuiSceneExplorer.cpp:128, 288 — see #358
  • src/McRFPy_API.cpp:1634 — wrapper construction; plus type registration (move GridData from internal_types[] to exported_types[], add the Grid alias binding)
  • src/GridData.cpp:12,21 — drop the static_cast<UIGrid*>(this) downcasts and the owning_view notifications
  • src/UIEntity.cpp:816-860get_grid returns GridData; delete the owning-view branch and the grid_as_uigrid fallback
  • src/UIGridView.cpp:682, 855-881 + src/UIGridView.h:208-218 — delete owning_view assignment/reset logic
  • Remove PyObjectsEnum::UIGRID entirely if nothing needs it (src/UIDrawable.h:27-38)
  • Delete UIGrid::render, UIGrid::click_at, UIGrid::get_bounds, UIGrid::move/resize, and the whole duplicate camera/render-texture block
  • Docs: GridData needs a real class docstring (it is a public type now); Grid documented as "a GridView that creates its own GridData"

Sequencing

Do #355 first. #355 moves the cell-callback family and child/entity hit-testing off UIGrid and onto UIGridView (where the camera lives, and where the live Python object actually is). After it lands, UIGrid has no input responsibilities left and its drawable half is only render + bounds — a much smaller surgery than doing both at once.

Do not fold this into #355: mixing a user-visible bugfix with a mechanical type refactor makes the diff unreviewable. Conversely, do keep the API break in this one issue rather than smearing it across #355/#359's follow-ups.

Supersedes #359 (which is resolved here by deleting owning_view rather than fixing it).

Validation

  • Full suite green (cd tests && python3 run_tests.py)
  • scene.children.append(data) raises TypeError rather than rendering a stale-camera duplicate
  • Multiple GridViews over one GridData render independently, with independent cameras
  • A standalone mcrfpy.GridData(...) with no view at all can be mutated, stepped, and queried without UB (this is what the old static_cast made impossible)
  • entity.grid returns GridData; isinstance(mcrfpy.Grid(...), mcrfpy.GridView) is True
## The gap #252's design (docs/GRID_ENTITY_OVERHAUL_ROADMAP.md, Phase 4) states plainly: > **Grid is NOT a UIDrawable.** It has no position, size, or rendering. > **GridView IS a UIDrawable.** It references a Grid and renders it. What actually shipped is the compat-shim half. The *Python name* moved to the view (`PyUIGridViewType.tp_name == "mcrfpy.Grid"`; the old type became internal `mcrfpy._GridData`), but the *C++ class* never gave up its drawable half: ```cpp // src/UIGrid.h:36 class UIGrid: public UIDrawable, public GridData ``` `UIGrid` still owns `render()`, `click_at()`, `get_bounds()`, `box`, `ptex`, its own `center_x/center_y/zoom/camera_rotation`, its own `perspective_entity`, and its own render textures — a complete second, stale copy of the camera that no longer corresponds to anything the user sees. ## The bare-`_GridData` render path is real, and it is maintained This is not merely vestigial. `UIGrid::render()` is alive (src/UIGrid.cpp:97), and `scene.children.append(view.grid)` **draws a second, complete copy of the grid through UIGrid's own camera, frozen at construction values**. Pan or zoom the real view and the ghost does not move. Worse, the data layer actively keeps that ghost alive: ```cpp // src/GridData.cpp:12 void GridData::markDirty() { content_generation++; static_cast<UIGrid*>(this)->UIDrawable::markDirty(); // <-- feeding the ghost if (auto view = owning_view.lock()) view->markDirty(); } ``` and GridData.h:149 says so outright: *"covering both render paths (a bare `_GridData` rendered directly, and the normal GridView)."* A bare `_GridData` rendered directly is a **supported path today**. That is the legacy behavior we must not carry into 1.0. ## Why this is not cosmetic The seam is actively producing bugs. #355, #357, #358 are all the same shape: code inspects a scene child for `derived_type() == UIGRID`, and it is dead in every case because scene children are always `UIGRIDVIEW`. Worse, `UIGridView::init_with_data` (src/UIGridView.cpp:566-613) constructs a full `_GridData` Python object, steals its `GridData` sub-object via an aliasing `shared_ptr`, copies the rendering state out, and `Py_DECREF`s the wrapper. So every `GridData` in the engine is physically the base sub-object of a hidden `UIGrid` whose `UIDrawable` half — `serial_number`, `is_python_subclass`, `click_callable` — belongs to a **discarded wrapper**. That's why `UIGrid::fireCellClick`'s `PythonObjectCache::lookup(this->serial_number)` can never find a user's `Grid` subclass: it looks up an object that no longer exists. ## Resolution (decided 2026-07-12) Semantically, `GridData` is a **map**, not a widget — closer to a `HeightMap` or a `DiscreteMap` than to anything on screen. Only `GridView` is a widget. `Grid` is a legacy holdover meaning "a GridData and a GridView together." ### 1. `UIGrid` sheds `UIDrawable`; the data type goes public `GridData` becomes a first-class, standalone-constructible public type (`mcrfpy.GridData`), not the internal `mcrfpy._GridData`. It has no position, no size, no camera, no `render()`. **This ordering is forced, not preferred.** Today `GridData` is *never* independently heap-allocated (always a `UIGrid` base sub-object — the comment at GridData.cpp:10 relies on it), so `static_cast<UIGrid*>(this)` in `markDirty` is currently valid. A public `GridData()` constructor makes that downcast **undefined behavior** the first time a user allocates one standalone. Shedding `UIDrawable` is therefore a *prerequisite* for exposing `GridData`, not cleanup that follows it. ### 2. `Entity.grid` returns the `GridData`, not the view `UIEntity::get_grid` (src/UIEntity.cpp:816-860) currently returns the owning `GridView` and falls back to a bare `_GridData` wrapper when there isn't one. It must return the `GridData`. **This is a user-visible break, and it is accepted pre-1.0.** Rationale (repo owner): *"Entities can be in grids that aren't visible at all, so we have to accept that change."* There may be zero views over an entity's grid, or several — so "the grid an entity is in" simply is not a camera. Code that wants the camera reaches the `GridView` from the scene, or holds the view it created; code that wants the data can also reach it *through* the view (`view.grid`). Migration: `player.grid.center_camera(player.pos)` → `view.center_camera(player.pos)` on the view actually being rendered. This was always ill-defined through the entity once N views were possible. ### 3. `owning_view` is deleted (resolves #359 by removal) `GridData::owning_view` is a single `weak_ptr`, which is structurally wrong the moment two views share one `GridData` — the entire premise of #252. It has exactly two consumers: - `UIEntity::get_grid` — removed by (2) above. - `GridData::markDirty`/`markCompositeDirty` notifying the view's render cache — already redundant with `content_generation`, which views poll via `last_content_gen` (#351). With both gone, the back-pointer has no readers and is deleted. #359 is resolved by **removal, not by wiring it up**. ### 4. `Grid` becomes an alias, and is NOT deprecated `mcrfpy.Grid` is already `PyUIGridViewType` — the same `PyTypeObject`, just with `tp_name = "mcrfpy.Grid"`. The legacy hazard is the C++ dualism, not the spelling. - Rename the canonical `tp_name` to `mcrfpy.GridView`. - Export `mcrfpy.Grid` as an alias bound to the **same type object** (not a subclass — so `isinstance` works both directions and there is no MRO to explain). - **No `DeprecationWarning`.** Once `UIGrid` is pure data, `Grid(grid_size=...)` is exactly "a GridView that creates its own GridData" — a constructor overload, not legacy behavior. Warning on it would fire in every script, demo, and game in the repo to buy nothing. - `GridView` holds `shared_ptr<GridData>`, so the data outlives any Python referent to it. No lifetime work needed. Resulting surface: ```python data = mcrfpy.GridData(grid_size=(80, 40)) # pure data; no position, never rendered view = mcrfpy.GridView(grid=data, pos=(0,0), size=(800,400)) mini = mcrfpy.GridView(grid=data, pos=(820,0), size=(160,80)) # second camera, same map g = mcrfpy.Grid(grid_size=(80, 40)) # sugar: makes its own GridData g.grid # -> GridData entity.grid # -> GridData (NOT a view) scene.children.append(data) # -> TypeError ``` ## Scope - `src/UIDrawable.cpp` — ~14 `case PyObjectsEnum::UIGRID:` arms in the getset/parent/shader/click switches (lines 31, 56, 262, 317, 1349, 1394, 1595, 1640, 1686, 1731, 1786, 1831, plus 1010 and 2198) - `src/Animation.cpp:577` — UIGRID animation target arm - `src/UICollection.cpp:72, 166, 1134` — conversion, append (must now **reject** with `TypeError`), stats - `src/ImGuiSceneExplorer.cpp:128, 288` — see #358 - `src/McRFPy_API.cpp:1634` — wrapper construction; plus type registration (move `GridData` from `internal_types[]` to `exported_types[]`, add the `Grid` alias binding) - `src/GridData.cpp:12,21` — drop the `static_cast<UIGrid*>(this)` downcasts and the `owning_view` notifications - `src/UIEntity.cpp:816-860` — `get_grid` returns `GridData`; delete the owning-view branch and the `grid_as_uigrid` fallback - `src/UIGridView.cpp:682, 855-881` + `src/UIGridView.h:208-218` — delete `owning_view` assignment/reset logic - Remove `PyObjectsEnum::UIGRID` entirely if nothing needs it (src/UIDrawable.h:27-38) - Delete `UIGrid::render`, `UIGrid::click_at`, `UIGrid::get_bounds`, `UIGrid::move/resize`, and the whole duplicate camera/render-texture block - Docs: `GridData` needs a real class docstring (it is a public type now); `Grid` documented as "a `GridView` that creates its own `GridData`" ## Sequencing **Do #355 first.** #355 moves the cell-callback family and child/entity hit-testing off `UIGrid` and onto `UIGridView` (where the camera lives, and where the live Python object actually is). After it lands, `UIGrid` has no input responsibilities left and its drawable half is only render + bounds — a much smaller surgery than doing both at once. Do not fold this into #355: mixing a user-visible bugfix with a mechanical type refactor makes the diff unreviewable. Conversely, **do keep the API break in this one issue** rather than smearing it across #355/#359's follow-ups. Supersedes #359 (which is resolved here by deleting `owning_view` rather than fixing it). ## Validation - Full suite green (`cd tests && python3 run_tests.py`) - `scene.children.append(data)` raises `TypeError` rather than rendering a stale-camera duplicate - Multiple `GridView`s over one `GridData` render independently, with independent cameras - A standalone `mcrfpy.GridData(...)` with no view at all can be mutated, stepped, and queried without UB (this is what the old `static_cast` made impossible) - `entity.grid` returns `GridData`; `isinstance(mcrfpy.Grid(...), mcrfpy.GridView)` is `True`
john changed title from [Refactor] Complete #252: _GridData (UIGrid) sheds UIDrawable — data is data, GridView is the only drawable to [Refactor] Complete #252: GridData becomes a public data type, GridView is the only drawable, Grid becomes an alias 2026-07-12 20:59:14 +00:00
Author
Owner

Design note: the parent problem, and why the split is right (2026-07-12)

Working session on feat/355-grid-input-revival. This records the analysis so the next person doesn't have to re-derive it. Two decisions are still open at the bottom — this is not yet ready to implement.

1. Do N GridViews duplicate caches for one GridData? No.

Raised as a concern that the #252 split forces every view to hold a redundant copy of render state, and that render state should therefore migrate onto GridData (lazily allocated), making GridData a "headless Entity/Layer/child container that feeds one or more GridViews."

Inventory of what UIGridView actually caches, and what each is a function of:

Cached Function of
renderTexture / rotationTexture (content, center, zoom, rotation, box size, fill_color, perspective)
last_center_x/y, last_zoom, last_camera_rotation, last_box_size, last_content_gen the same inputs (the #351 early-out's record of them)
ptex the tileset (per-view retexturing is a legitimate feature: ASCII view + graphical view of one map)
perspective_entity, perspective_enabled which entity's fog to overlay

Every one is camera- or presentation-local. Two views over one GridData legitimately produce two different images — a minimap at zoom=0.25 beside a main view at 1.0. These are not duplicate caches of one thing; they are distinct outputs.

The case that would have been genuinely alarming — FOV — is already clean. Visibility memory is stored per-entity (UIEntity::perspective_map, 3-state 0/1/2), and a view merely selects whose map to overlay (UIGridView.cpp:317-330). Two views can render two different entities' fog with zero conflict. GridData's TCOD FOV state is a scratchpad keyed by (x, y, radius, algo) with dedup (#292), not a per-view cache.

Moving the raster onto GridData therefore forces one of two bad outcomes: a single shared raster (kills split-screen/minimap outright), or GridData holding a map<view_identity, cache> — which is what the views already are, relocated, plus a new keying problem. The GridView is the lazily-allocated render cache. Zero views already means zero rasters and zero render work: content_generation is a monotone counter that stays valid with no views, and the views fan-out is a no-op over an empty vector.

Conclusion: the split is correct as designed, and it is only half-finished. The duplicate render cache that motivated the concern is real, but it is not view↔view — it is data↔UIGrid: UIGrid's drawable half carries a second camera, its own box, its own ptex, and its own render textures, frozen at construction. That is a phantom view living inside the data layer, and it is exactly the ghost render path this issue already describes. Nothing else needs to move.

2. Grid children cannot be parented to a view (closes out #364's proposed fix (a))

#364 recommends "parent grid children to the UIGridView rather than the internal UIGrid — the architecturally correct answer, may fall out of #361." That option is not available.

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 a child happened to be appended through would become its parent arbitrarily, and if that view died the child's parent would dangle while the child still rendered in the remaining views.

So children stay owned by GridData, and their upward dirty push must fan out through the views registry — structurally identical to what GridData::markCompositeDirty already does for entity moves. #364 is a prerequisite decision for this issue, not a consequence of it: once UIGrid sheds UIDrawable, setParent(internal UIGrid) will not even typecheck.

A back-reference of some kind is unavoidable, and it is worth being explicit about why: when a user writes bubble.x = 5, the child's markCompositeDirty() must reach the views, and the child is the only object in the call. It cannot ask its collection — it does not know it is in one.

3. Proposed: a unified ParentRef (replaces the ad-hoc parent fields)

UIDrawable does not have one parent field today. It has two, mutually exclusive and hand-maintained:

std::weak_ptr<UIDrawable> parent;   // #122
std::string parent_scene;           // #183 - name of scene if top-level

setParent() calls parent_scene.clear(); setParentScene() calls parent.reset(). So a non-drawable parent (Scene) is already a supported concept — the existing approach is just "bolt on a field per parent kind." Adding grid_owner as a third field would mean three mutually-exclusive fields, three setters each clearing the other two, and a three-way branch at every consuming site. Rejected.

Instead, collapse to one tagged parent:

using ParentRef = std::variant<std::monostate,             // unparented
                               std::weak_ptr<UIDrawable>,  // Frame, GridView
                               SceneRef,                   // scene name (#183)
                               std::weak_ptr<GridData>>;   // grid content (#361)
ParentRef parent_ref;

One setter, so the "clear the other field" bug class becomes unrepresentable. Each parent-kind-dependent behavior becomes a single std::visit:

  • markCompositeDirty() — walk up (drawable) / stop (scene: root, nothing above caches) / fan out through views (GridData). Note Scene already answers the "what does a non-drawable parent do here" question: it terminates. GridData's answer is: fan out. Same visit, different arms.
  • removeFromParent() — erase from the right container. This replaces the hand-rolled derived_type() chain at UIDrawable.cpp:973, which contains a dead case UIGRID arm — a third live instance of the #355/#366 bug class, in a third file. See #366.
  • getGlobalPosition() — parent offset / none / grid-world→screen transform.
  • Python parent getter — returns Frame / Scene / GridData. Once GridData is public (this issue), that is a real named object, not a hole.

Open questions (blocking implementation)

Q1. global_position of a grid child is genuinely ambiguous under N views. The child occupies one grid-world point, but each view maps it to a different screen pixel (different camera, zoom, widget position). child.global_position has no single correct answer once a second view exists — it has one per view. Today it accidentally returns something because the parent chain dead-ends at the internal UIGrid, which is wrong rather than ambiguous. Options:

  • (a) resolve under the GridData's primary view — arbitrary, and this issue is deleting primaryView() precisely because arbitrary-view choices are wrong;
  • (b) return the grid-world point, and document that screen-space resolution requires naming a view (e.g. view.to_screen(child));
  • (c) raise.

Q2. Does the ParentRef refactor land inside this issue or before it? It touches every parent site (setParent, setParentScene, getParent, removeFromParent, getGlobalPosition, get_parent/set_parent, alignment). It pairs naturally with this issue, which is already rewriting all the derived_type() switches — but it is separable, and doing it first would make this issue's diff considerably smaller.

Corrections to the issue body above

  • §3 says owning_view's markDirty/markCompositeDirty notification is "already redundant with content_generation, which views poll." That is wrong. markCompositeDirty is a bottom-up push up each view's ancestor chain — a Frame(use_render_texture=True) wrapping a view needs its cache invalidated, which a top-down render-time poll cannot do. The registry is load-bearing and is kept (it shipped in 85e4bbe as std::vector<std::weak_ptr<UIGridView>> views). What this issue deletes is only primaryView() (the arbitrary-view fallback) and UIEntity::get_grid returning a view.
  • Scope list should add: src/UIDrawable.cpp:973 (removeFromParent dead UIGRID arm) and src/UICollection.cpp:~1134 (#366).
## Design note: the parent problem, and why the split is right (2026-07-12) Working session on `feat/355-grid-input-revival`. This records the analysis so the next person doesn't have to re-derive it. **Two decisions are still open at the bottom — this is not yet ready to implement.** ### 1. Do N GridViews duplicate caches for one GridData? No. Raised as a concern that the #252 split forces every view to hold a redundant copy of render state, and that render state should therefore migrate onto `GridData` (lazily allocated), making GridData a "headless Entity/Layer/child container that feeds one or more GridViews." Inventory of what `UIGridView` actually caches, and what each is a function of: | Cached | Function of | |---|---| | `renderTexture` / `rotationTexture` | *(content, center, zoom, rotation, box size, fill_color, perspective)* | | `last_center_x/y`, `last_zoom`, `last_camera_rotation`, `last_box_size`, `last_content_gen` | the same inputs (the #351 early-out's record of them) | | `ptex` | the tileset (per-view retexturing is a legitimate feature: ASCII view + graphical view of one map) | | `perspective_entity`, `perspective_enabled` | *which entity's* fog to overlay | Every one is camera- or presentation-local. Two views over one GridData legitimately produce **two different images** — a minimap at `zoom=0.25` beside a main view at `1.0`. These are not duplicate caches of one thing; they are distinct outputs. The case that would have been genuinely alarming — FOV — is already clean. Visibility memory is stored **per-entity** (`UIEntity::perspective_map`, 3-state 0/1/2), and a view merely selects whose map to overlay (`UIGridView.cpp:317-330`). Two views can render two different entities' fog with zero conflict. `GridData`'s TCOD FOV state is a *scratchpad* keyed by (x, y, radius, algo) with dedup (#292), not a per-view cache. Moving the raster onto `GridData` therefore forces one of two bad outcomes: a single shared raster (kills split-screen/minimap outright), or `GridData` holding a `map<view_identity, cache>` — which is what the views already are, relocated, plus a new keying problem. **The GridView *is* the lazily-allocated render cache.** Zero views already means zero rasters and zero render work: `content_generation` is a monotone counter that stays valid with no views, and the `views` fan-out is a no-op over an empty vector. **Conclusion: the split is correct as designed, and it is only half-finished.** The duplicate render cache that motivated the concern is real, but it is not view↔view — it is **data↔UIGrid**: `UIGrid`'s drawable half carries a second camera, its own `box`, its own `ptex`, and its own render textures, frozen at construction. That is a phantom view living *inside* the data layer, and it is exactly the ghost render path this issue already describes. Nothing else needs to move. ### 2. Grid children cannot be parented to a view (closes out #364's proposed fix (a)) #364 recommends "parent grid children to the `UIGridView` rather than the internal UIGrid — the architecturally correct answer, may fall out of #361." **That option is not available.** `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 a child happened to be appended through would become its parent arbitrarily, and if that view died the child's parent would dangle while the child still rendered in the remaining views. So children stay owned by `GridData`, and their upward dirty push must fan out through the `views` registry — structurally identical to what `GridData::markCompositeDirty` already does for entity moves. **#364 is a prerequisite decision for this issue, not a consequence of it:** once `UIGrid` sheds `UIDrawable`, `setParent(internal UIGrid)` will not even typecheck. A back-reference of some kind is unavoidable, and it is worth being explicit about why: when a user writes `bubble.x = 5`, the child's `markCompositeDirty()` must reach the views, and the child is the only object in the call. It cannot ask its collection — it does not know it is in one. ### 3. Proposed: a unified `ParentRef` (replaces the ad-hoc parent fields) `UIDrawable` does not have one parent field today. It has **two**, mutually exclusive and hand-maintained: ```cpp std::weak_ptr<UIDrawable> parent; // #122 std::string parent_scene; // #183 - name of scene if top-level ``` `setParent()` calls `parent_scene.clear()`; `setParentScene()` calls `parent.reset()`. So a non-drawable parent (Scene) is **already a supported concept** — the existing approach is just "bolt on a field per parent kind." Adding `grid_owner` as a third field would mean three mutually-exclusive fields, three setters each clearing the other two, and a three-way branch at every consuming site. Rejected. Instead, collapse to one tagged parent: ```cpp using ParentRef = std::variant<std::monostate, // unparented std::weak_ptr<UIDrawable>, // Frame, GridView SceneRef, // scene name (#183) std::weak_ptr<GridData>>; // grid content (#361) ParentRef parent_ref; ``` One setter, so the "clear the other field" bug class becomes unrepresentable. Each parent-kind-dependent behavior becomes a single `std::visit`: - **`markCompositeDirty()`** — walk up (drawable) / **stop** (scene: root, nothing above caches) / fan out through `views` (GridData). Note Scene already answers the "what does a non-drawable parent do here" question: it terminates. GridData's answer is: fan out. Same visit, different arms. - **`removeFromParent()`** — erase from the right container. This replaces the hand-rolled `derived_type()` chain at `UIDrawable.cpp:973`, **which contains a dead `case UIGRID` arm** — a third live instance of the #355/#366 bug class, in a third file. See #366. - **`getGlobalPosition()`** — parent offset / none / grid-world→screen transform. - Python `parent` getter — returns `Frame` / `Scene` / `GridData`. Once `GridData` is public (this issue), that is a real named object, not a hole. ### Open questions (blocking implementation) **Q1. `global_position` of a grid child is genuinely ambiguous under N views.** The child occupies one grid-world point, but each view maps it to a different screen pixel (different camera, zoom, widget position). `child.global_position` has no single correct answer once a second view exists — it has one per view. Today it accidentally returns *something* because the parent chain dead-ends at the internal UIGrid, which is wrong rather than ambiguous. Options: - (a) resolve under the GridData's *primary* view — arbitrary, and this issue is deleting `primaryView()` precisely because arbitrary-view choices are wrong; - (b) return the grid-world point, and document that screen-space resolution requires naming a view (e.g. `view.to_screen(child)`); - (c) raise. **Q2. Does the `ParentRef` refactor land inside this issue or before it?** It touches every parent site (`setParent`, `setParentScene`, `getParent`, `removeFromParent`, `getGlobalPosition`, `get_parent`/`set_parent`, alignment). It pairs naturally with this issue, which is already rewriting all the `derived_type()` switches — but it is separable, and doing it first would make this issue's diff considerably smaller. ### Corrections to the issue body above - §3 says `owning_view`'s `markDirty`/`markCompositeDirty` notification is "already redundant with `content_generation`, which views poll." **That is wrong.** `markCompositeDirty` is a bottom-up push up each view's *ancestor* chain — a `Frame(use_render_texture=True)` wrapping a view needs *its* cache invalidated, which a top-down render-time poll cannot do. The registry is load-bearing and is **kept** (it shipped in 85e4bbe as `std::vector<std::weak_ptr<UIGridView>> views`). What this issue deletes is only `primaryView()` (the arbitrary-view fallback) and `UIEntity::get_grid` returning a view. - Scope list should add: `src/UIDrawable.cpp:973` (`removeFromParent` dead `UIGRID` arm) and `src/UICollection.cpp:~1134` (#366).
Author
Owner

Correction to the design note above

One claim in §3 is wrong. I wrote that removeFromParent's derived_type() chain "contains a dead case UIGRID arm — a third live instance of the #355/#366 bug class." It is not dead. The function has a UIGRIDVIEW arm as well (src/UIDrawable.cpp:1021), and the UIGRID arm genuinely fires, because grid children really do get parent = the internal UIGrid today (UIGridPyProperties.cpp:284UICollection.cpp:657). I asserted it after reading only the first two arms. Detail in the #366 thread.

The ParentRef argument is unaffected — arguably strengthened. removeFromParent currently needs three arms plus a parent_scene pre-check to handle what is conceptually one question ("who contains me?"), and it stays correct only because someone remembered to write an arm for both grid-parent conventions. That is exactly the hand-maintained invariant the variant removes. But the scope list addition should read "UIDrawable.cpp:973removeFromParent collapses from a 4-way chain to a std::visit," not "dead arm."

Also confirms Q1: get_global_position() (UIDrawable.cpp:1039) walks the parent chain accumulating p->position. For a grid child that chain terminates at the internal UIGrid, so the value returned today is the child's grid-world point with no camera/zoom/view-offset applied — i.e. it silently ignores every view. It is wrong rather than ambiguous, which is what makes Q1 a real decision and not a cleanup.

## Correction to the design note above One claim in §3 is wrong. I wrote that `removeFromParent`'s `derived_type()` chain "contains a dead `case UIGRID` arm — a third live instance of the #355/#366 bug class." **It is not dead.** The function has a `UIGRIDVIEW` arm as well (`src/UIDrawable.cpp:1021`), and the `UIGRID` arm genuinely fires, because grid children really do get `parent` = the internal UIGrid today (`UIGridPyProperties.cpp:284` → `UICollection.cpp:657`). I asserted it after reading only the first two arms. Detail in the #366 thread. The `ParentRef` argument is unaffected — arguably strengthened. `removeFromParent` currently needs **three** arms plus a `parent_scene` pre-check to handle what is conceptually one question ("who contains me?"), and it stays correct only because someone remembered to write an arm for both grid-parent conventions. That is exactly the hand-maintained invariant the variant removes. But the scope list addition should read "`UIDrawable.cpp:973` — `removeFromParent` collapses from a 4-way chain to a `std::visit`," not "dead arm." Also confirms **Q1**: `get_global_position()` (`UIDrawable.cpp:1039`) walks the parent chain accumulating `p->position`. For a grid child that chain terminates at the internal UIGrid, so the value returned today is the child's grid-world point with no camera/zoom/view-offset applied — i.e. it silently ignores every view. It is wrong rather than ambiguous, which is what makes Q1 a real decision and not a cleanup.
Author
Owner

Unblocked: #364 landed, and it answers both open questions (2026-07-13)

Decision (repo owner, 2026-07-13): children move from GridData to GridView. Entities stay on GridData. Shipped on feat/355-grid-input-revival; full write-up in the #364 thread. This supersedes §2 and §3 of my design note above, and resolves both blocking questions.

Q1 (global_position under N views) -- ANSWERED by construction

The ambiguity existed only because one child could be seen through N cameras. A child now belongs to exactly one view, so it has exactly one camera and one screen pixel. Option (b) from the design note ("return the grid-world point; screen resolution requires naming a view") is moot -- the view is named, by ownership.

The correct behavior is now well-defined and unimplemented: get_global_position() (UIDrawable.cpp:1039) still just accumulates p->position up the parent chain, so for a grid child it returns the raw grid-world point with no camera transform applied -- it ignores center, zoom, camera_rotation, and the view's own position. It needs to apply the inverse of UIGridView::localToGridWorld at the grid boundary and then continue accumulating. That is a small, well-specified piece of work rather than a design question, and it should land as part of this issue (or its own -- it is independent of the type split).

Q2 (ParentRef variant: inside this issue, or before it?) -- MOOT

The variant existed to represent a non-drawable parent (weak_ptr<GridData>) for grid children. There are no grid children on GridData any more, and GridData holds no UIDrawable of any kind. So once UIGrid sheds UIDrawable, there is no third parent kind to represent -- the parent is a Frame, a GridView, or a Scene, which is what the existing parent + parent_scene pair already covers.

ParentRef remains a nice cleanup (removeFromParent is still a chain of derived_type() arms plus a parent_scene pre-check, which is one question asked three ways), but it is no longer a prerequisite for anything. It should be its own Refactoring & Cleanup issue, done whenever, and it is not in this issue's scope.

Corrections to the issue body, cumulative

Two things in the body above are now stale and will mislead whoever implements this:

  1. §3 "owning_view is deleted (resolves #359 by removal)" is wrong. #359 was resolved the other way: the N-view registry (GridData::views, std::vector<std::weak_ptr<UIGridView>>) shipped in 85e4bbe and is load-bearing -- markCompositeDirty is a bottom-up push up each view's ancestor chain, which a top-down content_generation poll cannot replace. What this issue deletes is only primaryView() (the arbitrary-view fallback) and UIEntity::get_grid returning a view.
  2. The scope list should drop children from GridData -- #364 already moved it, along with UIGrid::get_children, UIGrid::render's child-drawing block, and UIGrid::resize's aligned-child loop. UIDrawable::removeFromParent's UIGRID arm and set_parent's _GridData arm are already gone too. The surgery left for this issue is correspondingly smaller: delete UIGrid's drawable half (render/click_at/get_bounds/camera/render-textures), make GridData public and standalone-constructible, point Entity.grid at the GridData, and rename tp_name to GridView with Grid as an alias.

Also worth knowing before starting

#364 exposed that UIGridView::render's #351 early-out never consulted its own render_dirty (fixed there), and that .parent returns a fresh wrapper on every read so child.parent is parent is always False for every type (#369, not fixed). The latter matters here: entity.grid returning a GridData will have the same identity problem unless #369 is addressed, i.e. e.grid is e.grid will be False.

## Unblocked: #364 landed, and it answers both open questions (2026-07-13) **Decision (repo owner, 2026-07-13): `children` move from `GridData` to `GridView`. Entities stay on `GridData`.** Shipped on `feat/355-grid-input-revival`; full write-up in the #364 thread. This supersedes §2 and §3 of my design note above, and resolves **both** blocking questions. ### Q1 (`global_position` under N views) -- ANSWERED by construction The ambiguity existed only because one child could be seen through N cameras. A child now belongs to exactly one view, so it has exactly one camera and one screen pixel. Option (b) from the design note ("return the grid-world point; screen resolution requires naming a view") is moot -- the view *is* named, by ownership. The correct behavior is now well-defined and unimplemented: `get_global_position()` (`UIDrawable.cpp:1039`) still just accumulates `p->position` up the parent chain, so for a grid child it returns the raw grid-world point with **no camera transform applied** -- it ignores `center`, `zoom`, `camera_rotation`, and the view's own position. It needs to apply the inverse of `UIGridView::localToGridWorld` at the grid boundary and then continue accumulating. That is a small, well-specified piece of work rather than a design question, and it should land as part of this issue (or its own -- it is independent of the type split). ### Q2 (`ParentRef` variant: inside this issue, or before it?) -- MOOT The variant existed to represent a **non-drawable parent** (`weak_ptr<GridData>`) for grid children. There are no grid children on `GridData` any more, and `GridData` holds no `UIDrawable` of any kind. So once `UIGrid` sheds `UIDrawable`, there is no third parent kind to represent -- the parent is a Frame, a GridView, or a Scene, which is what the existing `parent` + `parent_scene` pair already covers. `ParentRef` remains a *nice* cleanup (`removeFromParent` is still a chain of `derived_type()` arms plus a `parent_scene` pre-check, which is one question asked three ways), but it is **no longer a prerequisite for anything**. It should be its own `Refactoring & Cleanup` issue, done whenever, and it is not in this issue's scope. ### Corrections to the issue body, cumulative Two things in the body above are now stale and will mislead whoever implements this: 1. **§3 "`owning_view` is deleted (resolves #359 by removal)" is wrong.** #359 was resolved the *other* way: the N-view registry (`GridData::views`, `std::vector<std::weak_ptr<UIGridView>>`) shipped in 85e4bbe and **is load-bearing** -- `markCompositeDirty` is a bottom-up push up each view's ancestor chain, which a top-down `content_generation` poll cannot replace. What this issue deletes is only `primaryView()` (the arbitrary-view fallback) and `UIEntity::get_grid` returning a view. 2. **The scope list should drop `children` from `GridData`** -- #364 already moved it, along with `UIGrid::get_children`, `UIGrid::render`'s child-drawing block, and `UIGrid::resize`'s aligned-child loop. `UIDrawable::removeFromParent`'s `UIGRID` arm and `set_parent`'s `_GridData` arm are already gone too. The surgery left for this issue is correspondingly smaller: delete `UIGrid`'s drawable half (render/click_at/get_bounds/camera/render-textures), make `GridData` public and standalone-constructible, point `Entity.grid` at the `GridData`, and rename `tp_name` to `GridView` with `Grid` as an alias. ### Also worth knowing before starting #364 exposed that `UIGridView::render`'s #351 early-out never consulted its own `render_dirty` (fixed there), and that `.parent` returns a fresh wrapper on every read so `child.parent is parent` is always False for every type (**#369**, not fixed). The latter matters here: `entity.grid` returning a `GridData` will have the same identity problem unless #369 is addressed, i.e. `e.grid is e.grid` will be False.
Author
Owner

Implemented. Suite 327/327.

UIGrid is deleted, not demoted. Shedding UIDrawable left it as GridData + a texture + a pile of static binding functions — an empty subclass whose only remaining job was to be the thing an aliasing shared_ptr pointed into. So it collapsed:

  • class GridData absorbs ptex/getTexture() (the atlas is data — it is what defines the cell size, which every tile↔pixel conversion in the engine depends on) and carries its own PythonObjectCache serial, which it used to inherit from UIDrawable.
  • struct PyGridData (files src/PyGridData*.cpp, renamed from UIGrid*) holds the binding layer and is never instantiated.
  • PyObjectsEnum::UIGRID is gone, along with all 14 case arms that keyed on it.

The aliasing hack is gone, not relocated

UIGridView::init_with_data used to build a throwaway Python _GridData, steal its GridData base subobject via an aliasing shared_ptr, copy ~15 fields of rendering state out, and Py_DECREF the wrapper. The view now just calls make_shared<GridData>(...). grid_as_uigrid() is deleted from UIEntity.cpp and both open-coded copies in UIGridView.cpp are gone.

One init() now serves both construction modes. A side effect worth naming: view mode was silently a crippled subset. init_explicit_view parsed only {grid, pos, size, zoom, fill_color, name}, so Grid(grid=other, z_index=5) raised TypeError. A second view now takes the same kwargs as the first.

Grid(grid=data, grid_size=(4,4)) is a TypeError rather than silently ignoring one of them — quietly handing back a view of a different-sized map than requested is worse than refusing.

Two latent bugs fell out, both fixed here

  • #370Grid.center_camera() and Grid.size = ... were silent no-ops. Neither was ever defined on the view; both delegated to the ghost camera. Reproduced on master: center_camera((5,5)) leaves center_x at 50.0. This is exactly the failure mode this issue predicted, and it was live.
  • #371 — the shader uniform binder strcmp'd tp_name == "mcrfpy.Grid" (the view's name) and then cast to PyUIGridObject. UB that only worked because both wrappers had identical layout and both classes put UIDrawable at offset 0.

Corrections to this issue's plan

  • §3 "owning_view is deleted" — already done, differently. #359 shipped as a view registry (std::vector<weak_ptr<UIGridView>>), not a single weak_ptr, and the registry is load-bearing: it is how a data-layer mutation invalidates every view's cache and pushes dirt up each view's own ancestor chain. What this change removes is primaryView()'s use as an identity oracle, which was the genuinely wrong part — views.front() was an arbitrary answer dressed up as a deterministic one.
  • entity.gridGridData landed as specified, and is the one user-visible break. Migration is player.grid.center_camera(...)view.center_camera(...). Everything that is actually about the map — at(), entities, layers, compute_fov(), find_path(), step() — is unchanged.
  • view.grid → it's view.grid_data. The issue's examples say g.grid; the shipped accessor has been grid_data since #252 and stays that way.
  • Naming: canonical tp_name is now mcrfpy.GridView; mcrfpy.Grid is bound to the same type object. No DeprecationWarning, as decided. Note this does change type(g).__name__ to 'GridView' and repr() to <GridView ...>.

Validation (all from the issue's own list)

tests/regression/issue_361_griddata_is_not_a_widget_test.py, 25 assertions:

  • appending a GridData to a scene or a Frame raises TypeError; it exposes no widget property at all and is not a Drawable
  • a standalone GridData(...) with no view is writable, steppable, FOV-computable and pathable — the case the old static_cast<UIGrid*>(this) made undefined
  • two views over one GridData: independent cameras (pan and zoom), shared entities and cells
  • mcrfpy.Grid is mcrfpy.GridView; isinstance(Grid(...), GridView)
  • #370's no-ops now actually move the camera and resize the widget
  • render: a second view of one map paints a second region; mutating the shared map repaints both; idle frames stay byte-identical (the #351 early-out survives)
## Implemented. Suite 327/327. `UIGrid` is **deleted**, not demoted. Shedding `UIDrawable` left it as `GridData` + a texture + a pile of static binding functions — an empty subclass whose only remaining job was to be the thing an aliasing `shared_ptr` pointed into. So it collapsed: - `class GridData` absorbs `ptex`/`getTexture()` (the atlas is data — it is what defines the cell size, which every tile↔pixel conversion in the engine depends on) and carries its own `PythonObjectCache` serial, which it used to inherit from `UIDrawable`. - `struct PyGridData` (files `src/PyGridData*.cpp`, renamed from `UIGrid*`) holds the binding layer and is never instantiated. - `PyObjectsEnum::UIGRID` is gone, along with all 14 `case` arms that keyed on it. ### The aliasing hack is gone, not relocated `UIGridView::init_with_data` used to build a throwaway Python `_GridData`, steal its `GridData` base subobject via an aliasing `shared_ptr`, copy ~15 fields of rendering state out, and `Py_DECREF` the wrapper. The view now just calls `make_shared<GridData>(...)`. `grid_as_uigrid()` is deleted from `UIEntity.cpp` and both open-coded copies in `UIGridView.cpp` are gone. One `init()` now serves both construction modes. A side effect worth naming: **view mode was silently a crippled subset**. `init_explicit_view` parsed only `{grid, pos, size, zoom, fill_color, name}`, so `Grid(grid=other, z_index=5)` raised `TypeError`. A second view now takes the same kwargs as the first. `Grid(grid=data, grid_size=(4,4))` is a `TypeError` rather than silently ignoring one of them — quietly handing back a view of a *different-sized* map than requested is worse than refusing. ### Two latent bugs fell out, both fixed here - **#370** — `Grid.center_camera()` and `Grid.size = ...` were **silent no-ops**. Neither was ever defined on the view; both delegated to the ghost camera. Reproduced on master: `center_camera((5,5))` leaves `center_x` at `50.0`. This is exactly the failure mode this issue predicted, and it was live. - **#371** — the shader uniform binder `strcmp`'d `tp_name == "mcrfpy.Grid"` (the **view's** name) and then cast to `PyUIGridObject`. UB that only worked because both wrappers had identical layout *and* both classes put `UIDrawable` at offset 0. ### Corrections to this issue's plan - **§3 "`owning_view` is deleted" — already done, differently.** #359 shipped as a *view registry* (`std::vector<weak_ptr<UIGridView>>`), not a single `weak_ptr`, and the registry is load-bearing: it is how a data-layer mutation invalidates every view's cache and pushes dirt up each view's own ancestor chain. What this change removes is `primaryView()`'s use as an *identity* oracle, which was the genuinely wrong part — `views.front()` was an arbitrary answer dressed up as a deterministic one. - **`entity.grid` → `GridData`** landed as specified, and is the one user-visible break. Migration is `player.grid.center_camera(...)` → `view.center_camera(...)`. Everything that is actually about the map — `at()`, `entities`, `layers`, `compute_fov()`, `find_path()`, `step()` — is unchanged. - **`view.grid` → it's `view.grid_data`.** The issue's examples say `g.grid`; the shipped accessor has been `grid_data` since #252 and stays that way. - **Naming:** canonical `tp_name` is now `mcrfpy.GridView`; `mcrfpy.Grid` is bound to the same type object. No `DeprecationWarning`, as decided. Note this does change `type(g).__name__` to `'GridView'` and `repr()` to `<GridView ...>`. ### Validation (all from the issue's own list) `tests/regression/issue_361_griddata_is_not_a_widget_test.py`, 25 assertions: - appending a `GridData` to a scene *or* a Frame raises `TypeError`; it exposes no widget property at all and is not a `Drawable` - a standalone `GridData(...)` **with no view** is writable, steppable, FOV-computable and pathable — the case the old `static_cast<UIGrid*>(this)` made undefined - two views over one `GridData`: independent cameras (pan *and* zoom), shared entities and cells - `mcrfpy.Grid is mcrfpy.GridView`; `isinstance(Grid(...), GridView)` - #370's no-ops now actually move the camera and resize the widget - render: a second view of one map paints a second region; mutating the shared map repaints both; idle frames stay byte-identical (the #351 early-out survives)
john closed this issue 2026-07-13 11:44:13 +00:00
Sign in to join this conversation.
No milestone
No project
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.

Reference
john/McRogueFace#361
No description provided.