VisibilityMask records light_walls/algorithm even when they had no effect #404

New issue

Open

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

john commented

2026-07-27 14:27:19 +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}).

For an unbounded mask — observe() called with no origin — no FOV computation runs at all (py_observe just fills the visibility plane with 1s; see the has_origin branch around PyGridDataMethods.cpp:240-250, and the MCRF_NOTE "observe() with no origin touches no FOV state at all"). But the light_walls and algorithm parameters are still parsed, defaulted (light_walls=True, algorithm=FOV.BASIC), and passed through to PyVisibilityMask::create(), which stores them unconditionally on the resulting object (PyVisibilityMaskObject::light_walls, ::algorithm; see src/PyVisibilityMask.h:32-33). So mask.light_walls and mask.algorithm read back as True/FOV.BASIC on an unbounded mask even though neither parameter participated in producing it.

VisibilityMask is meant to be "an immutable, storable record of which cells were visible, from where, at which frame" — a record read back later, possibly much later (fog-of-war persistence, replay). Recording FOV parameters that were silently ignored makes that record misleading: a caller inspecting a stored mask has no way to tell "these values reflect how this mask was computed" from "these are just the defaults from an unrelated code path."

Two options to consider (pick one):

Report None for both light_walls and algorithm when the mask has no origin (mirrors how fov_radius is already None for unbounded masks).
Fail Early: reject light_walls/algorithm as errors if passed explicitly while origin is None, since they'd be meaningless arguments.

Filed as tier3/Tiny since it's a correctness-of-metadata issue, not a functional bug — the cell set itself is correct either way.

Found while implementing `GridData.observe()` / `mcrfpy.VisibilityMask` for the simulation-harness observation seam (see `src/PyGridDataMethods.cpp:PyGridData::py_observe`, `src/PyVisibilityMask.{h,cpp}`). For an unbounded mask — `observe()` called with no `origin` — no FOV computation runs at all (`py_observe` just fills the visibility plane with 1s; see the `has_origin` branch around `PyGridDataMethods.cpp:240-250`, and the MCRF_NOTE "observe() with no origin touches no FOV state at all"). But the `light_walls` and `algorithm` parameters are still parsed, defaulted (`light_walls=True`, `algorithm=FOV.BASIC`), and passed through to `PyVisibilityMask::create()`, which stores them unconditionally on the resulting object (`PyVisibilityMaskObject::light_walls`, `::algorithm`; see `src/PyVisibilityMask.h:32-33`). So `mask.light_walls` and `mask.algorithm` read back as `True`/`FOV.BASIC` on an unbounded mask even though neither parameter participated in producing it. `VisibilityMask` is meant to be "an immutable, storable record of which cells were visible, from where, at which frame" — a record read back later, possibly much later (fog-of-war persistence, replay). Recording FOV parameters that were silently ignored makes that record misleading: a caller inspecting a stored mask has no way to tell "these values reflect how this mask was computed" from "these are just the defaults from an unrelated code path." Two options to consider (pick one): 1. Report `None` for both `light_walls` and `algorithm` when the mask has no `origin` (mirrors how `fov_radius` is already `None` for unbounded masks). 2. Fail Early: reject `light_walls`/`algorithm` as errors if passed explicitly while `origin` is `None`, since they'd be meaningless arguments. Filed as tier3/Tiny since it's a correctness-of-metadata issue, not a functional bug — the cell set itself is correct either way.