[Refactor] Complete #252: GridData becomes a public data type, GridView is the only drawable, Grid becomes an alias #361
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Depends on
Reference
john/McRogueFace#361
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The gap
#252's design (docs/GRID_ENTITY_OVERHAUL_ROADMAP.md, Phase 4) states plainly:
What actually shipped is the compat-shim half. The Python name moved to the view (
PyUIGridViewType.tp_name == "mcrfpy.Grid"; the old type became internalmcrfpy._GridData), but the C++ class never gave up its drawable half:UIGridstill ownsrender(),click_at(),get_bounds(),box,ptex, its owncenter_x/center_y/zoom/camera_rotation, its ownperspective_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-
_GridDatarender path is real, and it is maintainedThis is not merely vestigial.
UIGrid::render()is alive (src/UIGrid.cpp:97), andscene.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:
and GridData.h:149 says so outright: "covering both render paths (a bare
_GridDatarendered directly, and the normal GridView)." A bare_GridDatarendered 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 alwaysUIGRIDVIEW.Worse,
UIGridView::init_with_data(src/UIGridView.cpp:566-613) constructs a full_GridDataPython object, steals itsGridDatasub-object via an aliasingshared_ptr, copies the rendering state out, andPy_DECREFs the wrapper. So everyGridDatain the engine is physically the base sub-object of a hiddenUIGridwhoseUIDrawablehalf —serial_number,is_python_subclass,click_callable— belongs to a discarded wrapper. That's whyUIGrid::fireCellClick'sPythonObjectCache::lookup(this->serial_number)can never find a user'sGridsubclass: it looks up an object that no longer exists.Resolution (decided 2026-07-12)
Semantically,
GridDatais a map, not a widget — closer to aHeightMapor aDiscreteMapthan to anything on screen. OnlyGridViewis a widget.Gridis a legacy holdover meaning "a GridData and a GridView together."1.
UIGridshedsUIDrawable; the data type goes publicGridDatabecomes a first-class, standalone-constructible public type (mcrfpy.GridData), not the internalmcrfpy._GridData. It has no position, no size, no camera, norender().This ordering is forced, not preferred. Today
GridDatais never independently heap-allocated (always aUIGridbase sub-object — the comment at GridData.cpp:10 relies on it), sostatic_cast<UIGrid*>(this)inmarkDirtyis currently valid. A publicGridData()constructor makes that downcast undefined behavior the first time a user allocates one standalone. SheddingUIDrawableis therefore a prerequisite for exposingGridData, not cleanup that follows it.2.
Entity.gridreturns theGridData, not the viewUIEntity::get_grid(src/UIEntity.cpp:816-860) currently returns the owningGridViewand falls back to a bare_GridDatawrapper when there isn't one. It must return theGridData.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
GridViewfrom 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_viewis deleted (resolves #359 by removal)GridData::owning_viewis a singleweak_ptr, which is structurally wrong the moment two views share oneGridData— the entire premise of #252. It has exactly two consumers:UIEntity::get_grid— removed by (2) above.GridData::markDirty/markCompositeDirtynotifying the view's render cache — already redundant withcontent_generation, which views poll vialast_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.
Gridbecomes an alias, and is NOT deprecatedmcrfpy.Gridis alreadyPyUIGridViewType— the samePyTypeObject, just withtp_name = "mcrfpy.Grid". The legacy hazard is the C++ dualism, not the spelling.tp_nametomcrfpy.GridView.mcrfpy.Gridas an alias bound to the same type object (not a subclass — soisinstanceworks both directions and there is no MRO to explain).DeprecationWarning. OnceUIGridis 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.GridViewholdsshared_ptr<GridData>, so the data outlives any Python referent to it. No lifetime work needed.Resulting surface:
Scope
src/UIDrawable.cpp— ~14case 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 armsrc/UICollection.cpp:72, 166, 1134— conversion, append (must now reject withTypeError), statssrc/ImGuiSceneExplorer.cpp:128, 288— see #358src/McRFPy_API.cpp:1634— wrapper construction; plus type registration (moveGridDatafrominternal_types[]toexported_types[], add theGridalias binding)src/GridData.cpp:12,21— drop thestatic_cast<UIGrid*>(this)downcasts and theowning_viewnotificationssrc/UIEntity.cpp:816-860—get_gridreturnsGridData; delete the owning-view branch and thegrid_as_uigridfallbacksrc/UIGridView.cpp:682, 855-881+src/UIGridView.h:208-218— deleteowning_viewassignment/reset logicPyObjectsEnum::UIGRIDentirely if nothing needs it (src/UIDrawable.h:27-38)UIGrid::render,UIGrid::click_at,UIGrid::get_bounds,UIGrid::move/resize, and the whole duplicate camera/render-texture blockGridDataneeds a real class docstring (it is a public type now);Griddocumented as "aGridViewthat creates its ownGridData"Sequencing
Do #355 first. #355 moves the cell-callback family and child/entity hit-testing off
UIGridand ontoUIGridView(where the camera lives, and where the live Python object actually is). After it lands,UIGridhas 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_viewrather than fixing it).Validation
cd tests && python3 run_tests.py)scene.children.append(data)raisesTypeErrorrather than rendering a stale-camera duplicateGridViews over oneGridDatarender independently, with independent camerasmcrfpy.GridData(...)with no view at all can be mutated, stepped, and queried without UB (this is what the oldstatic_castmade impossible)entity.gridreturnsGridData;isinstance(mcrfpy.Grid(...), mcrfpy.GridView)isTrue[Refactor] Complete #252: _GridData (UIGrid) sheds UIDrawable — data is data, GridView is the only drawableto [Refactor] Complete #252: GridData becomes a public data type, GridView is the only drawable, Grid becomes an aliasDesign 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
UIGridViewactually caches, and what each is a function of:renderTexture/rotationTexturelast_center_x/y,last_zoom,last_camera_rotation,last_box_size,last_content_genptexperspective_entity,perspective_enabledEvery one is camera- or presentation-local. Two views over one GridData legitimately produce two different images — a minimap at
zoom=0.25beside a main view at1.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
GridDatatherefore forces one of two bad outcomes: a single shared raster (kills split-screen/minimap outright), orGridDataholding amap<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_generationis a monotone counter that stays valid with no views, and theviewsfan-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 ownbox, its ownptex, 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
UIGridViewrather than the internal UIGrid — the architecturally correct answer, may fall out of #361." That option is not available.childrenlives onGridData(GridData.h:130), besideentities— shared world content, in grid-world coordinates (#360). AUIDrawablehas exactly oneparentweak_ptr, but aGridDatamay 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 theviewsregistry — structurally identical to whatGridData::markCompositeDirtyalready does for entity moves. #364 is a prerequisite decision for this issue, not a consequence of it: onceUIGridshedsUIDrawable,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'smarkCompositeDirty()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)UIDrawabledoes not have one parent field today. It has two, mutually exclusive and hand-maintained:setParent()callsparent_scene.clear();setParentScene()callsparent.reset(). So a non-drawable parent (Scene) is already a supported concept — the existing approach is just "bolt on a field per parent kind." Addinggrid_owneras 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:
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 throughviews(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-rolledderived_type()chain atUIDrawable.cpp:973, which contains a deadcase UIGRIDarm — a third live instance of the #355/#366 bug class, in a third file. See #366.getGlobalPosition()— parent offset / none / grid-world→screen transform.parentgetter — returnsFrame/Scene/GridData. OnceGridDatais public (this issue), that is a real named object, not a hole.Open questions (blocking implementation)
Q1.
global_positionof 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_positionhas 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:primaryView()precisely because arbitrary-view choices are wrong;view.to_screen(child));Q2. Does the
ParentRefrefactor 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 thederived_type()switches — but it is separable, and doing it first would make this issue's diff considerably smaller.Corrections to the issue body above
owning_view'smarkDirty/markCompositeDirtynotification is "already redundant withcontent_generation, which views poll." That is wrong.markCompositeDirtyis a bottom-up push up each view's ancestor chain — aFrame(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 in85e4bbeasstd::vector<std::weak_ptr<UIGridView>> views). What this issue deletes is onlyprimaryView()(the arbitrary-view fallback) andUIEntity::get_gridreturning a view.src/UIDrawable.cpp:973(removeFromParentdeadUIGRIDarm) andsrc/UICollection.cpp:~1134(#366).Correction to the design note above
One claim in §3 is wrong. I wrote that
removeFromParent'sderived_type()chain "contains a deadcase UIGRIDarm — a third live instance of the #355/#366 bug class." It is not dead. The function has aUIGRIDVIEWarm as well (src/UIDrawable.cpp:1021), and theUIGRIDarm genuinely fires, because grid children really do getparent= 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
ParentRefargument is unaffected — arguably strengthened.removeFromParentcurrently needs three arms plus aparent_scenepre-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—removeFromParentcollapses from a 4-way chain to astd::visit," not "dead arm."Also confirms Q1:
get_global_position()(UIDrawable.cpp:1039) walks the parent chain accumulatingp->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.Unblocked: #364 landed, and it answers both open questions (2026-07-13)
Decision (repo owner, 2026-07-13):
childrenmove fromGridDatatoGridView. Entities stay onGridData. Shipped onfeat/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_positionunder N views) -- ANSWERED by constructionThe 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 accumulatesp->positionup the parent chain, so for a grid child it returns the raw grid-world point with no camera transform applied -- it ignorescenter,zoom,camera_rotation, and the view's own position. It needs to apply the inverse ofUIGridView::localToGridWorldat 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 (
ParentRefvariant: inside this issue, or before it?) -- MOOTThe variant existed to represent a non-drawable parent (
weak_ptr<GridData>) for grid children. There are no grid children onGridDataany more, andGridDataholds noUIDrawableof any kind. So onceUIGridshedsUIDrawable, there is no third parent kind to represent -- the parent is a Frame, a GridView, or a Scene, which is what the existingparent+parent_scenepair already covers.ParentRefremains a nice cleanup (removeFromParentis still a chain ofderived_type()arms plus aparent_scenepre-check, which is one question asked three ways), but it is no longer a prerequisite for anything. It should be its ownRefactoring & Cleanupissue, 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:
owning_viewis 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 in85e4bbeand is load-bearing --markCompositeDirtyis a bottom-up push up each view's ancestor chain, which a top-downcontent_generationpoll cannot replace. What this issue deletes is onlyprimaryView()(the arbitrary-view fallback) andUIEntity::get_gridreturning a view.childrenfromGridData-- #364 already moved it, along withUIGrid::get_children,UIGrid::render's child-drawing block, andUIGrid::resize's aligned-child loop.UIDrawable::removeFromParent'sUIGRIDarm andset_parent's_GridDataarm are already gone too. The surgery left for this issue is correspondingly smaller: deleteUIGrid's drawable half (render/click_at/get_bounds/camera/render-textures), makeGridDatapublic and standalone-constructible, pointEntity.gridat theGridData, and renametp_nametoGridViewwithGridas an alias.Also worth knowing before starting
#364 exposed that
UIGridView::render's #351 early-out never consulted its ownrender_dirty(fixed there), and that.parentreturns a fresh wrapper on every read sochild.parent is parentis always False for every type (#369, not fixed). The latter matters here:entity.gridreturning aGridDatawill have the same identity problem unless #369 is addressed, i.e.e.grid is e.gridwill be False.Implemented. Suite 327/327.
UIGridis deleted, not demoted. SheddingUIDrawableleft it asGridData+ a texture + a pile of static binding functions — an empty subclass whose only remaining job was to be the thing an aliasingshared_ptrpointed into. So it collapsed:class GridDataabsorbsptex/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 ownPythonObjectCacheserial, which it used to inherit fromUIDrawable.struct PyGridData(filessrc/PyGridData*.cpp, renamed fromUIGrid*) holds the binding layer and is never instantiated.PyObjectsEnum::UIGRIDis gone, along with all 14casearms that keyed on it.The aliasing hack is gone, not relocated
UIGridView::init_with_dataused to build a throwaway Python_GridData, steal itsGridDatabase subobject via an aliasingshared_ptr, copy ~15 fields of rendering state out, andPy_DECREFthe wrapper. The view now just callsmake_shared<GridData>(...).grid_as_uigrid()is deleted fromUIEntity.cppand both open-coded copies inUIGridView.cppare gone.One
init()now serves both construction modes. A side effect worth naming: view mode was silently a crippled subset.init_explicit_viewparsed only{grid, pos, size, zoom, fill_color, name}, soGrid(grid=other, z_index=5)raisedTypeError. A second view now takes the same kwargs as the first.Grid(grid=data, grid_size=(4,4))is aTypeErrorrather 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
Grid.center_camera()andGrid.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))leavescenter_xat50.0. This is exactly the failure mode this issue predicted, and it was live.strcmp'dtp_name == "mcrfpy.Grid"(the view's name) and then cast toPyUIGridObject. UB that only worked because both wrappers had identical layout and both classes putUIDrawableat offset 0.Corrections to this issue's plan
owning_viewis deleted" — already done, differently. #359 shipped as a view registry (std::vector<weak_ptr<UIGridView>>), not a singleweak_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 isprimaryView()'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→GridDatalanded as specified, and is the one user-visible break. Migration isplayer.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'sview.grid_data. The issue's examples sayg.grid; the shipped accessor has beengrid_datasince #252 and stays that way.tp_nameis nowmcrfpy.GridView;mcrfpy.Gridis bound to the same type object. NoDeprecationWarning, as decided. Note this does changetype(g).__name__to'GridView'andrepr()to<GridView ...>.Validation (all from the issue's own list)
tests/regression/issue_361_griddata_is_not_a_widget_test.py, 25 assertions:GridDatato a scene or a Frame raisesTypeError; it exposes no widget property at all and is not aDrawableGridData(...)with no view is writable, steppable, FOV-computable and pathable — the case the oldstatic_cast<UIGrid*>(this)made undefinedGridData: independent cameras (pan and zoom), shared entities and cellsmcrfpy.Grid is mcrfpy.GridView;isinstance(Grid(...), GridView)