GridData.observe(origin=...) mutates the grid's shared FOV buffer #402

New issue

Open

opened 2026-07-27 14:27:04 +00:00 by john · 0 comments

john commented

2026-07-27 14:27:04 +00:00

Owner

Found while implementing GridData.observe() / mcrfpy.VisibilityMask for the simulation-harness observation seam (see src/PyGridDataMethods.cpp:PyGridData::py_observe, src/PyVisibilityMask.{h,cpp}).

observe() is documented as producing an inert, side-effect-free snapshot ("the returned dict holds no live references and is unaffected by later grid or entity changes"). But when called with an origin, it calls straight into GridData::computeFOV() (src/GridData.cpp:138), exactly the same code path used by compute_fov()/is_in_fov() and by entity FOV updates (UIEntity.cpp:112, :1311, GridLayers.cpp:301). There is exactly one TCOD FOV map per GridData, and computeFOV() overwrites it in place.

So a nominally read-only projection has a real side effect: if a background observer (an NPC, an AI perception query, a simulation harness taking a snapshot from some other position) calls grid.observe(origin=npc_pos), it silently clobbers whatever FOV state the player/renderer had just computed for that grid. The next is_in_fov() call or FOV-driven render reads the observer's FOV, not the player's.

This is currently only documented, not fixed — py_observe's MCRF_NOTE says: "observe(origin=...) recomputes the grid's FOV state exactly as compute_fov() does -- it is the same shared per-grid TCOD FOV buffer. observe() with no origin touches no FOV state at all."

Suggested fix shapes (pick one, or propose better):

Give GridData a scratch/second TCOD FOV map used only by observe(), leaving the "live" FOV map (used by compute_fov()/rendering) untouched.
Save the current FOV map's contents before computeFOV() inside observe() and restore it afterward.

Either way, observe(origin=...) should become genuinely side-effect-free, matching what its docstring already promises.

Type label: filing as Minor Feature rather than Bugfix — the current behavior matches its own documentation (it's a documented caveat, not an unnoticed bug), and fixing it requires adding real capacity (a second FOV buffer or save/restore), not correcting a wrong computation.

Found while implementing `GridData.observe()` / `mcrfpy.VisibilityMask` for the simulation-harness observation seam (see `src/PyGridDataMethods.cpp:PyGridData::py_observe`, `src/PyVisibilityMask.{h,cpp}`). `observe()` is documented as producing an inert, side-effect-free snapshot ("the returned dict holds no live references and is unaffected by later grid or entity changes"). But when called with an `origin`, it calls straight into `GridData::computeFOV()` (`src/GridData.cpp:138`), exactly the same code path used by `compute_fov()`/`is_in_fov()` and by entity FOV updates (`UIEntity.cpp:112`, `:1311`, `GridLayers.cpp:301`). There is exactly one TCOD FOV map per `GridData`, and `computeFOV()` overwrites it in place. So a nominally read-only projection has a real side effect: if a background observer (an NPC, an AI perception query, a simulation harness taking a snapshot from some other position) calls `grid.observe(origin=npc_pos)`, it silently clobbers whatever FOV state the player/renderer had just computed for that grid. The next `is_in_fov()` call or FOV-driven render reads the observer's FOV, not the player's. This is currently only documented, not fixed — `py_observe`'s MCRF_NOTE says: "observe(origin=...) recomputes the grid's FOV state exactly as compute_fov() does -- it is the same shared per-grid TCOD FOV buffer. observe() with no origin touches no FOV state at all." Suggested fix shapes (pick one, or propose better): - Give `GridData` a scratch/second TCOD FOV map used only by `observe()`, leaving the "live" FOV map (used by `compute_fov()`/rendering) untouched. - Save the current FOV map's contents before `computeFOV()` inside `observe()` and restore it afterward. Either way, `observe(origin=...)` should become genuinely side-effect-free, matching what its docstring already promises. Type label: filing as Minor Feature rather than Bugfix — the current behavior matches its own documentation (it's a documented caveat, not an unnoticed bug), and fixing it requires adding real capacity (a second FOV buffer or save/restore), not correcting a wrong computation.