[Bugfix] EntityBehavior RNG is unseeded and non-injectable — no deterministic replay, no A/B control #394

New issue

Open

opened 2026-07-26 20:32:44 +00:00 by john · 0 comments

john commented

2026-07-26 20:32:44 +00:00

Owner

Problem

src/EntityBehavior.cpp:22 seeds the behavior RNG from the OS entropy pool, at namespace scope, with no way to observe or set the seed:

// Thread-local random engine for behavior randomness
static thread_local std::mt19937 rng{std::random_device{}()};

Single call site today — src/EntityBehavior.cpp:69, auto target = candidates[dist(rng)]; — the random choice among candidate targets.

Consequence: two runs of the same scenario, from the same script, with the same map, produce different entity behavior. There is no Python-visible knob and no record of what was used.

Why it matters

Two separate consumers, and the second is the harder requirement:

Deterministic replay. A headless script driven by mcrfpy.step() is otherwise fully deterministic — simulation_time is an integer millisecond counter (GameEngine.h:229), timers run off it (GameEngine.cpp:565), and rendering advances zero simulation time. Behavior RNG is the one remaining source of run-to-run divergence, so a trace cannot be replayed and a disagreement cannot be bisected.
Experimental control in a live (windowed, realtime) simulation. This is the sharper case. When McRogueFace is used as an agent simulation environment (#55, #154, #156), the experiment is "same world, changed agent." If NPC choices differ run to run, an A/B against a changed agent stack has changed two variables, and the comparison is void. Unseeded RNG is not a replay nicety here — it makes the measurement unattributable.

The pattern to follow

PyNoiseSource already models the right shape and should be copied rather than reinvented:

seed is an optional constructor argument; None generates one (src/PyNoiseSource.cpp:174-186)
the generated seed is recorded and exposed read-only (src/PyNoiseSource.cpp:20-21)

That second half is the important one: a run that did not pin a seed should still be able to report which seed it got, so a surprising run can be reproduced after the fact.

Proposed surface

Something in the shape of:

mcrfpy.behavior_seed = 12345    # set
mcrfpy.behavior_seed            # read back; never None after engine init

Open to it living on GridData instead if per-grid streams turn out to be the better granularity — worth a moment's thought, since one global stream means adding an NPC to a scenario perturbs every other NPC's sequence, which is its own reproducibility hazard.

Also in scope (or a deliberate note that it isn't)

src/PySound.cpp:225 has a second unseeded std::mt19937. It is behind --audio-off in headless and therefore harmless today, but it is the same defect and will bite the moment audio is enabled in a measured run. Either fix it here or leave a comment saying why not.

Acceptance

Behavior RNG is seedable from Python, and the effective seed is readable whether or not it was set explicitly
tests/unit/ test: two runs at the same seed produce an identical entity position sequence over N grid.step() calls; two runs at different seeds diverge
Thread-safety story stated explicitly — the current thread_local means a background thread calling grid.step() gets its own stream, which is either correct or a bug depending on the intended model (relates to #219 / the mcrfpy.lock() path)
Decide and document PySound.cpp:225

Blocks

Prerequisite for using McRogueFace as a measured simulation environment (#55, #154). Should land before that work starts, not during it.

## Problem `src/EntityBehavior.cpp:22` seeds the behavior RNG from the OS entropy pool, at namespace scope, with no way to observe or set the seed: ```cpp // Thread-local random engine for behavior randomness static thread_local std::mt19937 rng{std::random_device{}()}; ``` Single call site today — `src/EntityBehavior.cpp:69`, `auto target = candidates[dist(rng)];` — the random choice among candidate targets. Consequence: **two runs of the same scenario, from the same script, with the same map, produce different entity behavior.** There is no Python-visible knob and no record of what was used. ## Why it matters Two separate consumers, and the second is the harder requirement: 1. **Deterministic replay.** A headless script driven by `mcrfpy.step()` is otherwise fully deterministic — `simulation_time` is an integer millisecond counter (`GameEngine.h:229`), timers run off it (`GameEngine.cpp:565`), and rendering advances zero simulation time. Behavior RNG is the one remaining source of run-to-run divergence, so a trace cannot be replayed and a disagreement cannot be bisected. 2. **Experimental control in a live (windowed, realtime) simulation.** This is the sharper case. When McRogueFace is used as an agent simulation environment (#55, #154, #156), the experiment is "same world, changed agent." If NPC choices differ run to run, an A/B against a changed agent stack has changed *two* variables, and the comparison is void. Unseeded RNG is not a replay nicety here — it makes the measurement unattributable. ## The pattern to follow `PyNoiseSource` already models the right shape and should be copied rather than reinvented: - seed is an optional constructor argument; `None` generates one (`src/PyNoiseSource.cpp:174-186`) - **the generated seed is recorded and exposed read-only** (`src/PyNoiseSource.cpp:20-21`) That second half is the important one: a run that did not pin a seed should still be able to report which seed it got, so a surprising run can be reproduced after the fact. ## Proposed surface Something in the shape of: ```python mcrfpy.behavior_seed = 12345 # set mcrfpy.behavior_seed # read back; never None after engine init ``` Open to it living on `GridData` instead if per-grid streams turn out to be the better granularity — worth a moment's thought, since one global stream means adding an NPC to a scenario perturbs every other NPC's sequence, which is its own reproducibility hazard. ## Also in scope (or a deliberate note that it isn't) `src/PySound.cpp:225` has a second unseeded `std::mt19937`. It is behind `--audio-off` in headless and therefore harmless today, but it is the same defect and will bite the moment audio is enabled in a measured run. Either fix it here or leave a comment saying why not. ## Acceptance - [ ] Behavior RNG is seedable from Python, and the effective seed is readable whether or not it was set explicitly - [ ] `tests/unit/` test: two runs at the same seed produce an identical entity position sequence over N `grid.step()` calls; two runs at different seeds diverge - [ ] Thread-safety story stated explicitly — the current `thread_local` means a background thread calling `grid.step()` gets its *own* stream, which is either correct or a bug depending on the intended model (relates to #219 / the `mcrfpy.lock()` path) - [ ] Decide and document `PySound.cpp:225` ## Blocks Prerequisite for using McRogueFace as a measured simulation environment (#55, #154). Should land before that work starts, not during it.