john/McRogueFace

Fork 0

[Bugfix] Shared-data grid path is unwired: Grid(grid=...) never sets owning_view, and _GridData.view is always None #359

New issue

Closed

opened 2026-07-12 19:19:53 +00:00 by john · 1 comment

john commented

2026-07-12 19:19:53 +00:00

Owner

⛔ RESOLVED BY DELETION — DO NOT IMPLEMENT AS WRITTEN (decided 2026-07-12)

owning_view and _GridData.view are DELETED, not wired up. This work moved to #361. Do not "fix" the two defects below by assigning the back-references — that would cement the one-view-per-GridData assumption that #252 exists to remove.

owning_view has exactly two consumers:

UIEntity::get_grid (src/UIEntity.cpp:827) — returns the owning GridView when one exists.

GridData::markDirty/markCompositeDirty (src/GridData.cpp:15, 23) — notifying the view's render cache.

Consumer (2) is already redundant with content_generation, which views poll via last_content_gen (#351).
Consumer (1) is removed by an accepted pre-1.0 API break: entity.grid will return the GridData, not the view. Rationale from the 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 — a single weak_ptr was never going to model that.

With both consumers gone, the back-pointer has no readers and is removed. See #361, which now carries this.

The multi-view regression test described at the bottom of this issue is still wanted — it belongs to #361.

(Original report below, retained for the analysis.)

Summary

The Grid(grid=existing) shared-data path — the whole point of the #252 split (split-screen, minimap, multiple cameras on one dataset) — has two back-references that are never assigned. Strong evidence this path has never been exercised.

1. `init_explicit_view` never sets `owning_view`

src/UIGridView.cpp:475-552 (init_explicit_view, the Grid(grid=other) / GridView(grid=other) path) shares grid_data from an existing _GridData or GridView, but never sets grid_data->owning_view.

Contrast the default path, init_with_data, src/UIGridView.cpp:609:

// Set owning_view back-reference on the GridData
self->data->grid_data->owning_view = self->data;

GridData::owning_view (src/GridData.h:152) is what markDirty() / markCompositeDirty() (GridData.h:169-170) use to notify the view. It drives the #313 data-layer dirty notifications and the #351 render early-out cache (last_content_gen). With it unset, a second view onto shared data is likely to serve a stale cached texture and never repaint when the data changes.

Design question this exposes: owning_view is a single weak_ptr, but N views can share one GridData. Fixing this properly means either a list of views, or inverting the notification (views poll content_generation, which they already cache as last_content_gen — the simpler fix, and it makes owning_view deletable). Prefer the latter. — This is what was chosen; see the resolution above.

2. `_GridData.view` is never assigned

PyUIGridObject carries a std::shared_ptr<UIGridView> view member, placement-new'd in tp_new (src/UIGrid.h:311-314) and exposed as read-only _GridData.view (src/UIGridPyProperties.cpp:371-380, getset at :605-609).

Nothing in the codebase ever assigns it. Grid() goes through UIGridView::init, which builds a _GridData wrapper, steals its GridData sub-object, and Py_DECREFs the wrapper (src/UIGridView.cpp:566-613) — so the wrapper that could have held a view is discarded. _GridData.view is always None.

Either wire it up or delete the member and the getset. Given _GridData is internal and not module-exported, deleting is likely correct. — Deleting was chosen.

Test

tests/regression/issue_XXX_shared_grid_data_test.py: two Grid views over one GridData with different center/zoom; mutate a cell; render both; assert both reflect the change (this currently should fail on the second view). — Still wanted; moved to #361.

Related: #355 (same #252 fallout), #352 (render cache dirty-source tracking), #361 (carries the resolution).

> ## ⛔ RESOLVED BY DELETION — DO NOT IMPLEMENT AS WRITTEN (decided 2026-07-12) > > **`owning_view` and `_GridData.view` are DELETED, not wired up. This work moved to #361. Do not "fix" the two defects below by assigning the back-references — that would cement the one-view-per-GridData assumption that #252 exists to remove.** > > `owning_view` has exactly two consumers: > 1. `UIEntity::get_grid` (src/UIEntity.cpp:827) — returns the owning `GridView` when one exists. > 2. `GridData::markDirty`/`markCompositeDirty` (src/GridData.cpp:15, 23) — notifying the view's render cache. > > Consumer (2) is already redundant with `content_generation`, which views poll via `last_content_gen` (#351). > Consumer (1) is removed by an **accepted pre-1.0 API break: `entity.grid` will return the `GridData`, not the view.** Rationale from the 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 — a single `weak_ptr` was never going to model that. > > With both consumers gone, the back-pointer has no readers and is removed. **See #361, which now carries this.** > > The multi-view regression test described at the bottom of this issue is still wanted — it belongs to #361. --- *(Original report below, retained for the analysis.)* ## Summary The `Grid(grid=existing)` shared-data path — the whole point of the #252 split (split-screen, minimap, multiple cameras on one dataset) — has two back-references that are never assigned. Strong evidence this path has never been exercised. ## 1. `init_explicit_view` never sets `owning_view` `src/UIGridView.cpp:475-552` (`init_explicit_view`, the `Grid(grid=other)` / `GridView(grid=other)` path) shares `grid_data` from an existing `_GridData` or `GridView`, but **never sets `grid_data->owning_view`**. Contrast the default path, `init_with_data`, `src/UIGridView.cpp:609`: ```cpp // Set owning_view back-reference on the GridData self->data->grid_data->owning_view = self->data; ``` `GridData::owning_view` (src/GridData.h:152) is what `markDirty()` / `markCompositeDirty()` (GridData.h:169-170) use to notify the view. It drives the #313 data-layer dirty notifications and the #351 render early-out cache (`last_content_gen`). With it unset, a second view onto shared data is likely to serve a stale cached texture and never repaint when the data changes. **Design question this exposes**: `owning_view` is a single `weak_ptr`, but N views can share one GridData. Fixing this properly means either a list of views, or inverting the notification (views poll `content_generation`, which they already cache as `last_content_gen` — the simpler fix, and it makes `owning_view` deletable). Prefer the latter. — *This is what was chosen; see the resolution above.* ## 2. `_GridData.view` is never assigned `PyUIGridObject` carries a `std::shared_ptr<UIGridView> view` member, placement-new'd in `tp_new` (src/UIGrid.h:311-314) and exposed as read-only `_GridData.view` (src/UIGridPyProperties.cpp:371-380, getset at :605-609). **Nothing in the codebase ever assigns it.** `Grid()` goes through `UIGridView::init`, which builds a `_GridData` wrapper, steals its `GridData` sub-object, and `Py_DECREF`s the wrapper (src/UIGridView.cpp:566-613) — so the wrapper that could have held a `view` is discarded. `_GridData.view` is always `None`. Either wire it up or delete the member and the getset. Given `_GridData` is internal and not module-exported, deleting is likely correct. — *Deleting was chosen.* ## Test `tests/regression/issue_XXX_shared_grid_data_test.py`: two `Grid` views over one GridData with different `center`/`zoom`; mutate a cell; render both; assert both reflect the change (this currently should fail on the second view). — *Still wanted; moved to #361.* Related: #355 (same #252 fallout), #352 (render cache dirty-source tracking), #361 (carries the resolution).

john added the

system:grid

Bugfix

priority:tier1-active

system:rendering

labels

2026-07-12 19:19:53 +00:00

john referenced this issue

2026-07-12 19:20:28 +00:00

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

john referenced this issue

2026-07-12 19:21:02 +00:00

[Bugfix] Grid cell callbacks, child clicks, and cell hover all dead since #252 GridView unification #355

john commented

2026-07-12 20:59:26 +00:00

Author

Owner

Resolution decided 2026-07-12: owning_view is deleted, not wired up. See #361, which now carries this.

The recommendation in the issue body was right, and the audit confirms it. owning_view has exactly two consumers in the tree:

UIEntity::get_grid (src/UIEntity.cpp:827) — returns the owning GridView when one exists, falling back to a bare _GridData wrapper otherwise.
GridData::markDirty / markCompositeDirty (src/GridData.cpp:15, 23) — notifying the view's render cache.

Consumer (2) is already redundant with content_generation, which views poll via last_content_gen (#351).

Consumer (1) is removed by an accepted pre-1.0 API break: entity.grid will return the GridData, not the view. Rationale from the 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. A single weak_ptr back-pointer was never going to model that — which is exactly why it is structurally wrong, not merely unset. With both consumers gone the back-pointer has no readers.

So the two defects reported here (init_explicit_view never setting owning_view; PyUIGridObject::view never assigned) are not fixed — the fields are deleted. Doing this work as a "wire it up" fix would cement the one-view-per-GridData assumption that #252 exists to remove.

Sequencing: #355 first (input moves to GridView), then #361 does the removal along with the rest of the data/drawable split, keeping the API break in one reviewable commit.

**Resolution decided 2026-07-12: `owning_view` is deleted, not wired up. See #361, which now carries this.** The recommendation in the issue body was right, and the audit confirms it. `owning_view` has exactly two consumers in the tree: 1. `UIEntity::get_grid` (src/UIEntity.cpp:827) — returns the owning `GridView` when one exists, falling back to a bare `_GridData` wrapper otherwise. 2. `GridData::markDirty` / `markCompositeDirty` (src/GridData.cpp:15, 23) — notifying the view's render cache. Consumer (2) is already redundant with `content_generation`, which views poll via `last_content_gen` (#351). Consumer (1) is removed by an accepted pre-1.0 API break: **`entity.grid` will return the `GridData`, not the view.** Rationale from the 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. A single `weak_ptr` back-pointer was never going to model that — which is exactly why it is structurally wrong, not merely unset. With both consumers gone the back-pointer has no readers. So the two defects reported here (`init_explicit_view` never setting `owning_view`; `PyUIGridObject::view` never assigned) are **not fixed — the fields are deleted**. Doing this work as a "wire it up" fix would cement the one-view-per-GridData assumption that #252 exists to remove. Sequencing: #355 first (input moves to `GridView`), then #361 does the removal along with the rest of the data/drawable split, keeping the API break in one reviewable commit.

john referenced this issue

2026-07-12 22:04:36 +00:00

[Bugfix] GridData::views leaks expired weak_ptrs — ~UIGridView never unregisters #362

john referenced this issue

2026-07-12 22:05:04 +00:00

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

john referenced this issue

2026-07-13 03:06:20 +00:00

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

john referenced this issue

2026-07-13 11:14:13 +00:00

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