VisibilityMask has no value equality or hash — identical masks compare unequal #405

New issue

Open

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

john commented

2026-07-27 14:53:26 +00:00

Owner

mcrfpy.VisibilityMask (src/PyVisibilityMask.h/.cpp, branch sim/integration, not yet on master) is described in its own docstring as "an immutable, storable record of which cells were visible, from where, at which frame" — created only by GridData.observe(), meant to be kept around for fog-of-war persistence, replaying an AI's past perception, or checking a recorded belief state against ground truth.

The problem: mcrfpydef::PyVisibilityMaskType (src/PyVisibilityMask.cpp, the type object literal starting ~line 172) sets neither tp_richcompare nor tp_hash, and there's no tp_weaklistoffset either. That means:

The type inherits object's default tp_richcompare/tp_hash, i.e. identity equality and identity hash.
Two VisibilityMask instances with identical origin, fov_radius, tick, cells (and grid_size/light_walls/algorithm) compare unequal with ==, and hash to different values.
The type also isn't weakref-able, so it can't be used as a weak key/value anywhere that pattern would otherwise fit.

Why it matters: the whole point of this type is to be a comparable, storable value. The obvious thing a caller will write — old_belief == new_observation to check whether an agent's recorded perception still matches what it can see now, or deduplicating a history of masks in a set/as dict keys — silently does the wrong thing. It won't raise; it'll just always answer False (or, for a set/dict, never collapse duplicates), because every mask is is-compared instead of compared by content. That's a correctness trap precisely for the persistence/replay use cases the type exists for.

Urgency note: this costs nothing to fix now, before the type has shipped on master or in a release. Adding tp_richcompare/tp_hash after external code exists that relies on (or accidentally depends on) identity semantics is a behavior change on an already-released type — e.g. code that stuffed masks into a dict keyed by id()-equivalent semantics, or that used == as an (accidental) identity check, would change behavior silently. Doing this before release is free; doing it after is not.

Open design question — what participates in equality?

cells (the frozenset) — obviously yes, this is the payload.
origin, fov_radius, grid_size — presumably yes, they're part of what makes the record meaningful (two masks with the same cells but computed from a different origin/radius/grid describe different things, even if they happen to coincide on visible cells).
tick — probably yes, and worth calling out explicitly: two masks with identical cells/origin/fov_radius taken at different frames are arguably still different records (one is stale, one is current), even though "what was visible" is the same set. If replay/history code wants to dedupe by content-ignoring-time, that should be a different comparison than == (e.g. a .same_visibility_as(other) method), not the default equality.
light_walls/algorithm — unclear, and this directly overlaps with #404 (VisibilityMask records light_walls/algorithm even when no FOV computation ran, e.g. for an unbounded mask). If #404 is resolved by reporting None for those fields when they didn't participate in computing the mask, then two unbounded masks would naturally compare equal on those fields regardless of what defaults were passed in — which is the right outcome. Whatever fields end up participating in ==/hash should be decided together with #404's fix, not independently, since the same "did this field actually affect this mask" question underlies both.

Suggested implementation shape (not prescriptive): tp_richcompare doing element-wise comparison of origin, fov_radius, tick, grid_size, cells (and whatever #404 decides for light_walls/algorithm), only for Py_EQ/Py_NE (raise/return NotImplemented for ordering comparisons — there's no natural order for a visibility record); tp_hash combining hashes of the same fields via something like PyObject_Hash composition (careful: hashing a frozenset of tuples is fine, but skip fields that could be unhashable). Add tp_weaklistoffset + a PyObject* weakreflist member to PyVisibilityMaskObject while at it, following the existing shared_ptr-wrapper pattern used elsewhere in the codebase (PyObject_HEAD + ... + PyObject* weakreflist).

Cross-reference: #404 (same type, light_walls/algorithm recorded even when no FOV computation ran) — resolve together, since both hinge on "which fields are actually meaningful parts of the record."

Not fixed yet; raised by a code reviewer during work on the observation seam.

`mcrfpy.VisibilityMask` (`src/PyVisibilityMask.h`/`.cpp`, branch `sim/integration`, not yet on master) is described in its own docstring as "an immutable, storable record of which cells were visible, from where, at which frame" — created only by `GridData.observe()`, meant to be kept around for fog-of-war persistence, replaying an AI's past perception, or checking a recorded belief state against ground truth. **The problem**: `mcrfpydef::PyVisibilityMaskType` (`src/PyVisibilityMask.cpp`, the type object literal starting ~line 172) sets neither `tp_richcompare` nor `tp_hash`, and there's no `tp_weaklistoffset` either. That means: - The type inherits `object`'s default `tp_richcompare`/`tp_hash`, i.e. **identity** equality and identity hash. - Two `VisibilityMask` instances with identical `origin`, `fov_radius`, `tick`, `cells` (and `grid_size`/`light_walls`/`algorithm`) compare **unequal** with `==`, and hash to different values. - The type also isn't weakref-able, so it can't be used as a weak key/value anywhere that pattern would otherwise fit. **Why it matters**: the whole point of this type is to be a comparable, storable value. The obvious thing a caller will write — `old_belief == new_observation` to check whether an agent's recorded perception still matches what it can see now, or deduplicating a history of masks in a `set`/as `dict` keys — silently does the wrong thing. It won't raise; it'll just always answer `False` (or, for a set/dict, never collapse duplicates), because every mask is `is`-compared instead of compared by content. That's a correctness trap precisely for the persistence/replay use cases the type exists for. **Urgency note**: this costs nothing to fix now, before the type has shipped on master or in a release. Adding `tp_richcompare`/`tp_hash` after external code exists that relies on (or accidentally depends on) identity semantics is a **behavior change** on an already-released type — e.g. code that stuffed masks into a `dict` keyed by `id()`-equivalent semantics, or that used `==` as an (accidental) identity check, would change behavior silently. Doing this before release is free; doing it after is not. **Open design question — what participates in equality?** - `cells` (the frozenset) — obviously yes, this is the payload. - `origin`, `fov_radius`, `grid_size` — presumably yes, they're part of what makes the record meaningful (two masks with the same cells but computed from a different origin/radius/grid describe different things, even if they happen to coincide on visible cells). - `tick` — probably **yes**, and worth calling out explicitly: two masks with identical `cells`/`origin`/`fov_radius` taken at different frames are arguably still different *records* (one is stale, one is current), even though "what was visible" is the same set. If replay/history code wants to dedupe by content-ignoring-time, that should be a different comparison than `==` (e.g. a `.same_visibility_as(other)` method), not the default equality. - `light_walls`/`algorithm` — unclear, and this directly overlaps with #404 (VisibilityMask records `light_walls`/`algorithm` even when no FOV computation ran, e.g. for an unbounded mask). If #404 is resolved by reporting `None` for those fields when they didn't participate in computing the mask, then two unbounded masks would naturally compare equal on those fields regardless of what defaults were passed in — which is the right outcome. Whatever fields end up participating in `==`/`hash` should be decided together with #404's fix, not independently, since the same "did this field actually affect this mask" question underlies both. **Suggested implementation shape** (not prescriptive): `tp_richcompare` doing element-wise comparison of `origin`, `fov_radius`, `tick`, `grid_size`, `cells` (and whatever #404 decides for `light_walls`/`algorithm`), only for `Py_EQ`/`Py_NE` (raise/return `NotImplemented` for ordering comparisons — there's no natural order for a visibility record); `tp_hash` combining hashes of the same fields via something like `PyObject_Hash` composition (careful: hashing a frozenset of tuples is fine, but skip fields that could be unhashable). Add `tp_weaklistoffset` + a `PyObject* weakreflist` member to `PyVisibilityMaskObject` while at it, following the existing shared_ptr-wrapper pattern used elsewhere in the codebase (`PyObject_HEAD` + ... + `PyObject* weakreflist`). **Cross-reference**: #404 (same type, `light_walls`/`algorithm` recorded even when no FOV computation ran) — resolve together, since both hinge on "which fields are actually meaningful parts of the record." Not fixed yet; raised by a code reviewer during work on the observation seam.