john/McRogueFace

Fork 0

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

New issue

Closed

opened 2026-07-12 00:26:51 +00:00 by john · 0 comments

john commented

2026-07-12 00:26:51 +00:00

Owner

Symptom

grid.on_cell_click, grid.on_cell_enter, grid.on_cell_exit never fire, and grid.hovered_cell stays None, on any grid created via mcrfpy.Grid(...). Plain grid.on_click DOES fire, which confirms the event pipeline itself is fine.

Scope is wider than originally filed. Everything below is dead for a mcrfpy.Grid:

Broken	Site
`on_cell_click`	PyScene.cpp:262, 275, 286
`on_cell_enter` / `on_cell_exit` / `hovered_cell`	PyScene.cpp:380
hover on child drawables inside a grid (`on_enter`/`on_exit`/`on_move`)	PyScene.cpp:387 (traversal into `grid->children` never reached)
clicks on child drawables inside a grid	UIGridView::click_at swallows them
clicks on entities inside a grid	UIGridView::click_at swallows them

Related same-root-cause bugs split out: #357 (find()/findAll() never search grid entities), #358 (ImGui scene explorer won't expand grids).

Existing test tests/unit/test_grid_cell_events.py masks all of this: it prints "PARTIAL (events may require interactive mode)" and exits 0 when zero events fire (lines 82-86, 112-116). That string is why this shipped broken.

Root cause

Since #252, mcrfpy.Grid(...) constructs a UIGridView (derived_type() == UIGRIDVIEW), but every dispatch site is gated on derived_type() == UIGRID. Scene children are always UIGridViews; the internal UIGrid (mcrfpy._GridData) is never itself a scene-graph member. All six gates are dead code.

UIGridView::click_at (src/UIGridView.cpp:369-373) is a 4-line stub — bounds-check, return this. It never descends into children or entities, and never records a cell.

The part that makes "just add a UIGRIDVIEW branch" wrong

The cell-callback family is split across the seam in the worst possible way:

Storage (on_cell_*_callable, hovered_cell, last_clicked_cell, cell_callback_cache) lives on GridData (src/GridData.h:125-138).
Firing (fireCellClick/Enter/Exit, updateCellHover, refreshCellCallbackCache) lives on UIGrid (src/UIGrid.cpp:1084-1345) — per the comment at GridData.h:140, "because they need access to UIDrawable::serial_number/is_python_subclass".
The camera needed to compute a cell at all lives on UIGridView.

UIGrid::screenToCell reads UIGrid's own center_x/center_y/zoom — legacy fields that are no longer the camera the user sees. Even with the UIGRID branch re-enabled, it would report wrong cells for any panned or zoomed grid. UIGridView::screenToCell (src/UIGridView.cpp:88-109) is the only correct implementation, and it currently has no callers.

Worse: UIGridView::init_with_data (src/UIGridView.cpp:566-613) builds a _GridData Python object, steals its GridData sub-object via an aliasing shared_ptr, and Py_DECREFs the wrapper. So UIGrid::fireCellClick's PythonObjectCache::lookup(this->serial_number) resolves the serial of a discarded wrapper — a class MyGrid(mcrfpy.Grid) subclass instance is a GridView, and its on_cell_click method could never be found even if dispatch reached it. And refreshCellCallbackCache walks the type hierarchy up to sentinel &PyUIGridType (UIGrid.cpp:1113), which a Grid subclass never passes through.

The ownership has to move. It cannot be fixed by adding a branch.

Fix

Input becomes purely a GridView concern — the view owns the camera, so it is the only thing that can map screen to cell, and it is the object the user's Python subclass actually is.

Move hovered_cell, last_clicked_cell, cell_callback_cache, the three on_cell_* callables, and fireCell* / updateCellHover / refreshCellCallbackCache from GridData/UIGrid onto UIGridView. Delete them from UIGrid rather than leaving two copies.
- hovered_cell must be per-view: two views over one GridData (#359) would otherwise ping-pong exit/enter through a shared field on every hover pass.
- grid.on_cell_click keeps working unchanged, because mcrfpy.Grid is the view. view.grid (a _GridData) loses cell callbacks — correct: a data grid has no screen mapping.
Implement UIGridView::click_at properly: children hit-test (reverse z) -> entity hit-test -> record cell via the existing screenToCell -> return this. Port from UIGrid::click_at (src/UIGrid.cpp:525-617) and delete that.
- Reproduce the existing grid-world child coordinate convention exactly. It is inconsistent with UIFrame's frame-local convention, but that is #360 — do not change it in this commit.
Kill the enum gates rather than adding to them. Instead of a second derived_type()==UIGRIDVIEW arm at each of six sites, add virtuals on UIDrawable:
- virtual bool dispatchCellClick(button, action) — default false; GridView overrides.
- virtual void updateHover(sf::Vector2f) — default no-op; Frame and GridView override to recurse into children.
- virtual GridData* asGridData() — default nullptr; unblocks #357 and #358.
This is what stops the family from re-rotting the next time a type is unified.
Tighten tests/unit/test_grid_cell_events.py from "PARTIAL" prints into hard asserts. Add regression coverage for child-click, entity-click, and child-hover inside a grid.

Repro (headless)

import mcrfpy
from mcrfpy import automation

scene = mcrfpy.Scene("t")
grid = mcrfpy.Grid(grid_size=(10, 10), pos=(50, 50), size=(160, 160))
hits = []
grid.on_click = lambda pos, b, a: hits.append(("click", pos, b, a))
grid.on_cell_click = lambda cp, b, a: hits.append(("cell", cp, b, a))
scene.children.append(grid)
scene.activate()
mcrfpy.step(0.05)
automation.click((110, 110))
mcrfpy.step(0.05)
print(hits)
# Actual:   only ("click", ...) PRESSED + RELEASED - no "cell" events
# Expected: cell events at cell (3, 3)

Not in scope

_GridData shedding UIDrawable entirely — the remaining half of #252 — is #361, and it is sequenced after this issue. After this lands, UIGrid has no input responsibilities left, which makes #361 a much smaller surgery.

Found while rewriting the site's systems/input.md docs page (signatures in stubs/API reference are correct; the events just never dispatch).

## Symptom `grid.on_cell_click`, `grid.on_cell_enter`, `grid.on_cell_exit` never fire, and `grid.hovered_cell` stays `None`, on any grid created via `mcrfpy.Grid(...)`. Plain `grid.on_click` DOES fire, which confirms the event pipeline itself is fine. **Scope is wider than originally filed.** Everything below is dead for a `mcrfpy.Grid`: | Broken | Site | |---|---| | `on_cell_click` | PyScene.cpp:262, 275, 286 | | `on_cell_enter` / `on_cell_exit` / `hovered_cell` | PyScene.cpp:380 | | **hover on child drawables inside a grid** (`on_enter`/`on_exit`/`on_move`) | PyScene.cpp:387 (traversal into `grid->children` never reached) | | **clicks on child drawables inside a grid** | UIGridView::click_at swallows them | | **clicks on entities inside a grid** | UIGridView::click_at swallows them | Related same-root-cause bugs split out: #357 (`find()`/`findAll()` never search grid entities), #358 (ImGui scene explorer won't expand grids). Existing test `tests/unit/test_grid_cell_events.py` masks all of this: it prints `"PARTIAL (events may require interactive mode)"` and **exits 0 when zero events fire** (lines 82-86, 112-116). That string is why this shipped broken. ## Root cause Since #252, `mcrfpy.Grid(...)` constructs a **UIGridView** (`derived_type() == UIGRIDVIEW`), but every dispatch site is gated on `derived_type() == UIGRID`. Scene children are always UIGridViews; the internal `UIGrid` (`mcrfpy._GridData`) is never itself a scene-graph member. **All six gates are dead code.** `UIGridView::click_at` (src/UIGridView.cpp:369-373) is a 4-line stub — bounds-check, `return this`. It never descends into children or entities, and never records a cell. ### The part that makes "just add a UIGRIDVIEW branch" wrong The cell-callback family is split across the seam in the worst possible way: - **Storage** (`on_cell_*_callable`, `hovered_cell`, `last_clicked_cell`, `cell_callback_cache`) lives on **GridData** (src/GridData.h:125-138). - **Firing** (`fireCellClick/Enter/Exit`, `updateCellHover`, `refreshCellCallbackCache`) lives on **UIGrid** (src/UIGrid.cpp:1084-1345) — per the comment at GridData.h:140, "because they need access to UIDrawable::serial_number/is_python_subclass". - **The camera needed to compute a cell at all** lives on **UIGridView**. `UIGrid::screenToCell` reads UIGrid's *own* `center_x/center_y/zoom` — legacy fields that are no longer the camera the user sees. Even with the UIGRID branch re-enabled, it would report wrong cells for any panned or zoomed grid. `UIGridView::screenToCell` (src/UIGridView.cpp:88-109) is the only correct implementation, and it currently **has no callers**. Worse: `UIGridView::init_with_data` (src/UIGridView.cpp:566-613) builds a `_GridData` Python object, steals its `GridData` sub-object via an aliasing `shared_ptr`, and `Py_DECREF`s the wrapper. So `UIGrid::fireCellClick`'s `PythonObjectCache::lookup(this->serial_number)` resolves the serial of a **discarded wrapper** — a `class MyGrid(mcrfpy.Grid)` subclass instance is a *GridView*, and its `on_cell_click` method could never be found even if dispatch reached it. And `refreshCellCallbackCache` walks the type hierarchy up to sentinel `&PyUIGridType` (UIGrid.cpp:1113), which a Grid subclass never passes through. The ownership has to move. It cannot be fixed by adding a branch. ## Fix **Input becomes purely a GridView concern** — the view owns the camera, so it is the only thing that can map screen to cell, and it is the object the user's Python subclass actually is. 1. Move `hovered_cell`, `last_clicked_cell`, `cell_callback_cache`, the three `on_cell_*` callables, and `fireCell*` / `updateCellHover` / `refreshCellCallbackCache` from GridData/UIGrid **onto UIGridView**. Delete them from UIGrid rather than leaving two copies. - `hovered_cell` **must** be per-view: two views over one GridData (#359) would otherwise ping-pong exit/enter through a shared field on every hover pass. - `grid.on_cell_click` keeps working unchanged, because `mcrfpy.Grid` **is** the view. `view.grid` (a `_GridData`) loses cell callbacks — correct: a data grid has no screen mapping. 2. Implement `UIGridView::click_at` properly: children hit-test (reverse z) -> entity hit-test -> record cell via the existing `screenToCell` -> return `this`. Port from `UIGrid::click_at` (src/UIGrid.cpp:525-617) and delete that. - **Reproduce the existing grid-world child coordinate convention exactly.** It is inconsistent with UIFrame's frame-local convention, but that is #360 — do not change it in this commit. 3. **Kill the enum gates rather than adding to them.** Instead of a second `derived_type()==UIGRIDVIEW` arm at each of six sites, add virtuals on `UIDrawable`: - `virtual bool dispatchCellClick(button, action)` — default `false`; GridView overrides. - `virtual void updateHover(sf::Vector2f)` — default no-op; Frame and GridView override to recurse into children. - `virtual GridData* asGridData()` — default `nullptr`; unblocks #357 and #358. This is what stops the family from re-rotting the next time a type is unified. 4. Tighten `tests/unit/test_grid_cell_events.py` from "PARTIAL" prints into hard asserts. Add regression coverage for child-click, entity-click, and child-hover inside a grid. ## Repro (headless) ```python import mcrfpy from mcrfpy import automation scene = mcrfpy.Scene("t") grid = mcrfpy.Grid(grid_size=(10, 10), pos=(50, 50), size=(160, 160)) hits = [] grid.on_click = lambda pos, b, a: hits.append(("click", pos, b, a)) grid.on_cell_click = lambda cp, b, a: hits.append(("cell", cp, b, a)) scene.children.append(grid) scene.activate() mcrfpy.step(0.05) automation.click((110, 110)) mcrfpy.step(0.05) print(hits) # Actual: only ("click", ...) PRESSED + RELEASED - no "cell" events # Expected: cell events at cell (3, 3) ``` ## Not in scope `_GridData` shedding `UIDrawable` entirely — the remaining half of #252 — is **#361**, and it is sequenced *after* this issue. After this lands, UIGrid has no input responsibilities left, which makes #361 a much smaller surgery. Found while rewriting the site's systems/input.md docs page (signatures in stubs/API reference are correct; the events just never dispatch).

john added the

system:grid

system:input

Bugfix

priority:tier1-active

labels

2026-07-12 00:26:51 +00:00

john referenced this issue

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

[Bugfix] mcrfpy.find()/findAll() never search entities inside grids (UIGRID gate dead since #252) #357

john referenced this issue

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

[Bugfix] ImGui scene explorer shows no children/entities for grids (UIGRIDVIEW hits default: break) #358

john referenced this issue

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

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

john referenced this issue

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

[Bugfix] Grid children get grid-world pixel coords in click_at, Frame children get frame-local — inconsistent #360

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 changed title from ~~[Bugfix] Grid cell callbacks (on_cell_click/on_cell_enter/on_cell_exit, hovered_cell) never fire since #252 GridView unification~~ to [Bugfix] Grid cell callbacks, child clicks, and cell hover all dead since #252 GridView unification

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

john added a new dependency

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

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

john added a new dependency

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

#360 [Bugfix] Grid children get grid-world pixel coords in click_at, Frame children get frame-local — inconsistent

john added a new dependency

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

#357 [Bugfix] mcrfpy.find()/findAll() never search entities inside grids (UIGRID gate dead since #252)

john added a new dependency

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