Xiaochuang Yuan commited on
Commit
b53d4ed
ยท
1 Parent(s): a3ba9ba

docs(engine-followups): refresh triage against post-merge OpenRA-Rust HEAD 8ca8989

Browse files

The previous triage doc was authored against `OpenRA-Rust/main` HEAD
`feb8981` (pre-engine-feature-wave merge); 5 of the 7 findings were
classified "unreproducible โ€” feature lives only on engine-feature-wave".
That branch is now merged (HEAD `8ca8989`), so each finding has a real
post-merge status:

- #1 (tick rate): DOC-ONLY, fixed in CLAUDE.md (separate commit).
- #2 (ore density clamp at 12): BROKEN, doc-fixed in schema.py
docstring (separate commit).
- #3 (nuke kills not credited): FIXED via engine fix +
test_nuke_credits_kills.rs (engine-side commit).
- #4 (explicit harvest drifts): BROKEN; pinned by ignored test
test_harvest_explicit_target.rs (engine-side commit). TODO engine
fix: add bound_patch to Activity::Harvest.
- #5 (ReturnFire stance): PARTIALLY FIXED; needs empirical re-measure
on combat-kite-and-pull. TODO.
- #6 (arena mpspawn rotation): DOC-ONLY, fixed in ENGINE_FOLLOWUPS.md
(separate commit). Engine doesn't offset pre-placed actors; what
varies per seed is the auto-MCV's mpspawn pick.
- #7 (proc overlapping ore_patches disc): BROKEN; pinned by ignored
test test_proc_overlapping_patch_warns.rs (engine-side commit).
TODO engine fix: add oramap::validate_layout.

Includes "TODOs surfaced" + "Pre-existing test failures" sections so
follow-up agents have a clean handoff. Engine wheel rebuilt cleanly:
maturin develop --release printed `Installed openra_train-0.1.0`.

Files changed (1) hide show
  1. docs/ENGINE_FOLLOWUPS_TRIAGE.md +320 -0
docs/ENGINE_FOLLOWUPS_TRIAGE.md ADDED
@@ -0,0 +1,320 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ # Engine Follow-ups โ€” Triage against `OpenRA-Rust/main` HEAD `8ca8989` (post-merge)
2
+
3
+ Cross-checks the seven findings in `docs/ENGINE_FOLLOWUPS.md` against
4
+ `OpenRA-Rust/main` HEAD `8ca8989` (the engine-feature-wave merge,
5
+ 2026-05-23). The previous version of this doc (HEAD `feb8981`)
6
+ classified five findings as "unreproducible โ€” feature lives only on
7
+ `engine-feature-wave`"; that branch is now merged so each finding has
8
+ a real status against the current `main`.
9
+
10
+ ## Executive summary
11
+
12
+ | # | Finding | Post-merge status | Action |
13
+ |---|---------|-------------------|--------|
14
+ | 1 | Tick-rate doc drift | DOC-ONLY (engine constant is 30/step) | **FIXED** โ€” `OpenRA-Bench/CLAUDE.md` updated |
15
+ | 2 | Ore density clamps at 12/cell | BROKEN (clamp still at 12) | DOC-FIXED โ€” `schema.py::ore_patches` docstring covers the recipe |
16
+ | 3 | Nuke AoE kills not credited to `kills_per_player` | **FIXED** (engine fix shipped this run) | Engine fix + 2 Rust tests landed |
17
+ | 4 | Explicit `harvest(unit_id, x, y)` drifts off target | BROKEN (harv re-binds to nearest patch after first deposit) | Failing `#[ignore]`d test pinned; **TODO engine fix** |
18
+ | 5 | ReturnFire stance breaks kite-and-pull | PARTIALLY FIXED (stance:1 fix landed); residual is balance/empirical | TODO empirical re-measure of `combat-kite-and-pull` |
19
+ | 6 | Arena generator's default 4-corner mpspawns | DOC-ONLY (auto-MCV varies per seed; engine doesn't offset pre-placed actors) | **FIXED** โ€” `ENGINE_FOLLOWUPS.md` rewording |
20
+ | 7 | Pre-placed `proc` overlapping `ore_patches:` | BROKEN (patch is seeded, but proc footprint blocks pathing โ€” silent sterilisation) | Failing `#[ignore]`d test pinned; **TODO validator** |
21
+
22
+ Wheel rebuilt cleanly: `Installed openra_train-0.1.0` on
23
+ `maturin develop --release` against HEAD `8ca8989`. `openra-sim` lib
24
+ + integration tests are green except for known pre-existing
25
+ fixture/environment failures (see end of doc).
26
+
27
+ ---
28
+
29
+ ## Finding 1 โ€” Tick-rate discrepancy (doc drift)
30
+
31
+ **Status on `main`:** DOC-ONLY (engine constant is 30/step, not 90).
32
+
33
+ `openra-train/src/env.rs:33` declares `pub const
34
+ DEFAULT_TICKS_PER_STEP: u32 = 30;` and `Env::step` (env.rs:351-364)
35
+ advances exactly that many ticks per `step()`. The bench's
36
+ `RustEnvHandle.__init__` does not call `with_ticks_per_step` so every
37
+ `env.step` advances 30 ticks. Under `step_until_event` (interrupt
38
+ mode), the loop runs UP TO `max_ticks` (default 5 in bench code) and
39
+ breaks on the first signal โ€” per-turn advance is variable in
40
+ `[1, 5]`.
41
+
42
+ **Action taken:** `OpenRA-Bench/CLAUDE.md` updated in two spots
43
+ (defective-scenario list and engine-facts list). The "~90 ticks/turn"
44
+ phrase is gone; replaced with the actual `DEFAULT_TICKS_PER_STEP`
45
+ constant + a note to read `info["ticks_advanced"]` for interrupt-mode
46
+ runs.
47
+
48
+ ---
49
+
50
+ ## Finding 2 โ€” Ore density clamps at 12 cells
51
+
52
+ **Status on `main`:** BROKEN, but documented.
53
+
54
+ Clamp lives at `openra-sim/src/resource.rs:94-95`:
55
+ ```rust
56
+ let per_cell = ((patch.amount.max(1) + passable_cells - 1) / passable_cells)
57
+ .clamp(1, 12) as u8;
58
+ ```
59
+ Ceiling is 12 ore/cell; raising the cap is a real (though small)
60
+ balance change. The cheapest mitigation is the docstring (option
61
+ (c) in the spec) โ€” already applied to `schema.py`.
62
+
63
+ **Action taken:** `openra_bench/scenarios/schema.py::MapDef.ore_patches`
64
+ docstring now reproduces the clamp formula and the recommended ratio
65
+ `amount โ‰ˆ 12 ยท ฯ€ ยท radiusยฒ ยท 1.2`, plus the 50 cr/ore conversion. New
66
+ econ packs that use `ore_patches:` get the recipe inline.
67
+
68
+ **Optional follow-up (NOT in this commit):** raise the cap to 24/cell
69
+ in `resource.rs:95` and update the `seed_patch_fills_disk` density
70
+ assertion. Out of scope for triage; documented for whoever wants to
71
+ pick it up.
72
+
73
+ **Bench-side workarounds in place:** `econ-mine-and-grow.yaml`,
74
+ `econ-multi-patch-allocation.yaml`, `econ-second-base-race.yaml`,
75
+ `mcv-deploy-second-base.yaml` already inflate `amount` to fill the
76
+ cap; the new docstring formalises that recipe so future packs don't
77
+ re-discover the footgun.
78
+
79
+ ---
80
+
81
+ ## Finding 3 โ€” Nuke AoE kills not credited to `kills_per_player`
82
+
83
+ **Status on `main`:** **FIXED** in this triage pass.
84
+
85
+ Pre-fix `world.rs::detonate_nuke` (lines 2101-2141) removed dead
86
+ actors via `self.actors.remove(&id)` without ever incrementing
87
+ `kills_per_player`. The `_owner` parameter was even prefix-`_`'d to
88
+ silence the unused-variable warning, telegraphing the bug.
89
+
90
+ **Engine fix shipped:** `world.rs::detonate_nuke` now uses `owner`
91
+ and credits `kills_per_player` once per dead actor whose owner is
92
+ NOT the firing owner (mirrors the projectile-resolve scoping โ€”
93
+ friendly-fire kills do NOT credit, so an agent can't game
94
+ `units_killed_gte` by nuking its own units).
95
+
96
+ **Pinning:** `OpenRA-Rust/openra-sim/tests/test_nuke_credits_kills.rs`
97
+ โ€” two tests:
98
+ - `nuke_credits_each_enemy_kill_to_firing_owner` โ€” fire mslo at a
99
+ cluster of 5 enemy e1s, assert `kills_for_player(owner)` rises by
100
+ exactly 5.
101
+ - `nuke_does_not_credit_friendly_fire_kills` โ€” fire mslo at agent's
102
+ own e1 cluster, assert no credit.
103
+
104
+ **Existing test still green:** `test_superweapons.rs::mslo_nuke_kills_enemy_cluster`
105
+ runs unchanged (the new credit logic is additive).
106
+
107
+ **Bench-side cleanup queued (TODO, NOT this commit):**
108
+ - `OpenRA-Bench/openra_bench/scenarios/packs/spec-nuke-strike.yaml`
109
+ currently uses `enemy_buildings_destroyed_gte` with a `silo`-cluster
110
+ workaround (lines ~62-70 by triage finding #3). With the fix in
111
+ place, the hard-tier predicate can switch to `units_killed_gte: 5`
112
+ against an infantry cluster โ€” restoring the original "nuke clears
113
+ infantry cluster" semantics.
114
+
115
+ ---
116
+
117
+ ## Finding 4 โ€” Explicit `harvest(unit_id, x, y)` orders drift off target
118
+
119
+ **Status on `main`:** BROKEN.
120
+
121
+ `world.rs::order_harvest` (lines 1372-1402) is idempotent and stores
122
+ `last_harvest_cell: Some(target)` โ€” so the FIRST `FindingOre` cycle
123
+ honours the explicit target. But `tick_harvesters` (line 2261) calls
124
+ `terrain.find_nearest_resource(center, 15)` with `center =
125
+ last_harvest_cell.or(loc)`, and `last_harvest_cell` is overwritten
126
+ each successful harvest tick (line 2328) to the actually-harvested
127
+ cell. After the first deposit, the harv's "explicit target" has
128
+ already drifted to wherever it last mined; subsequent FindingOre
129
+ cycles search around that point and pick up whatever ore is closest,
130
+ without remembering which patch the original `harvest()` order
131
+ selected. With multiple patches inside a 15-cell window, the
132
+ explicit allocation evaporates within ~30 ticks.
133
+
134
+ **Pinning:** `openra-sim/tests/test_harvest_explicit_target.rs::explicit_harvest_target_stays_bound_to_far_patch`
135
+ โ€” `#[ignore]`d. Sets up a near patch (10,15) and far patch (40,15),
136
+ issues an explicit Harvest order targeting the FAR patch, runs 3000
137
+ ticks, asserts every harvested cell stays within Chebyshev distance
138
+ โ‰ค 4 of the far patch. Currently the harv would re-bind to whichever
139
+ patch is closer to its mining position. Flip to `#[test]` after the
140
+ fix lands.
141
+
142
+ **TODO engine fix:** add `bound_patch: Option<(x, y, radius)>` to
143
+ `Activity::Harvest`, set by `order_harvest` from the explicit target
144
+ (radius derived from the registered `OrePatchDef` containing the
145
+ cell, default 3). FindingOre's resource search restricts to cells
146
+ inside the patch disc when `bound_patch` is set. Cleared by a new
147
+ Harvest order to a different target or by `stop`. Multi-step change
148
+ (touches `Activity::Harvest` shape, `order_harvest`, `tick_harvesters`,
149
+ all `#[derive]`'d serialization tests) โ€” not a one-liner; pinned-test
150
+ + TODO is the right form.
151
+
152
+ **Bench-side workarounds:** `econ-multi-patch-allocation.yaml:65-87,
153
+ 212-216` pre-stages one harv beside each patch and relies on the
154
+ auto-route binding each to the nearest proc โ€” the workaround dodges
155
+ the explicit-allocation discipline by hand-placing harvs at the
156
+ target. Once the engine fix lands, the pack docstring and pre-staged
157
+ positions can be removed; `harvest()` becomes load-bearing again.
158
+
159
+ ---
160
+
161
+ ## Finding 5 โ€” ReturnFire-stance auto-fire breaks kite-and-pull
162
+
163
+ **Status on `main`:** PARTIALLY FIXED โ€” empirical re-measure needed.
164
+
165
+ The original finding's option (b) โ€” "Pure ReturnFire that fires only
166
+ after taking hits" โ€” landed in commit `9e999e2 fix(engine): clarify
167
+ stance โ€” stance:1 true return-fire-only`. Pinned by
168
+ `openra-sim/tests/test_stance_semantics.rs::test_stance_1_return_fire_only_against_passive_enemy`.
169
+ A stance:1 unit next to a passive (stance:0) enemy now holds fire
170
+ indefinitely.
171
+
172
+ But the **scenario** breaks for a different reason: once a stance:1
173
+ raider takes one hit from a 3tnk (hunt bot), it auto-fires forever
174
+ (the 60-tick window self-refreshes on each subsequent hit). So the
175
+ 1v1 kite question becomes a balance question: does a passive
176
+ ReturnFire 2tnk out-trade an aggressive 3tnk by auto-fire alone? On
177
+ the OLD stance:1 (auto-engage on any in-range enemy) the answer was
178
+ yes โ€” the original finding's empirical claim. On the NEW stance:1
179
+ the answer depends on exact weapon DPS values which changed across
180
+ the wave merges; uncertain without a fresh empirical run.
181
+
182
+ **TODO empirical re-measure:** run the stand-still policy
183
+ (no agent orders) on `combat-kite-and-pull.yaml` against `main` HEAD
184
+ `8ca8989`. Suggested test:
185
+ `OpenRA-Rust/openra-sim/tests/test_kite_1v1.rs` โ€” 1 2tnk at (10,10)
186
+ stance:1 vs 1 3tnk at (40,10) stance:3 on a 60ร—30 arena, run 500
187
+ frames with no orders, assert 2tnk dies. If it passes, finding #5 is
188
+ genuinely fixed and the bench's 3-raider focus-fire workaround in
189
+ `combat-kite-and-pull.yaml:181-183` can be reverted to a true 1v1.
190
+ If it fails, the residual is balance: tune the 2tnk's `90mm` reload
191
+ delay or damage in `gamerules.rs`.
192
+
193
+ **Bench-side workaround:** `combat-kite-and-pull.yaml` uses a
194
+ 3-raider stack to substitute focus-fire micro for distance-control
195
+ (documented inline). Workaround is correct given current uncertainty;
196
+ remove only after the empirical re-measure resolves.
197
+
198
+ ---
199
+
200
+ ## Finding 6 โ€” Arena generator's default 4-corner mpspawns rotate per-seed
201
+
202
+ **Status on `main`:** DOC-ONLY (engine code does not offset
203
+ pre-placed actor coords).
204
+
205
+ Re-reading the engine source: `build_scenario_actor`
206
+ (`openra-train/src/env.rs:2338+`) places each scenario actor at its
207
+ literal `sa.position` โ€” there is NO offset math anywhere. The
208
+ finding's claim "authored `position: [6, 5]` actually appears at
209
+ y=29-33 on some seeds" is not substantiated by the code. The
210
+ practical effect that DOES exist: `assign_spawn_points`
211
+ (`world.rs:4684+`) picks one mpspawn per seed for the auto-spawned
212
+ MCV, deterministic on `seed`. So when a pack relies on the
213
+ auto-MCV's corner being a specific one (not its position absolute),
214
+ that DOES vary per seed.
215
+
216
+ **Action taken:** `docs/ENGINE_FOLLOWUPS.md` finding #6 reworded to
217
+ remove the misleading "engine offsets pre-placed actors" claim.
218
+
219
+ **Bench-side workaround:** the `spawns: [[6, 5]]` declaration in
220
+ `perception-target-vs-fog.yaml:62-68, 157, 200, 252` (and any other
221
+ arena-generator pack that hand-places agent buildings AND relies on
222
+ a fixed auto-MCV corner) is correct and stays.
223
+
224
+ **Optional follow-up (NOT in this commit):** change
225
+ `openra_bench/mapgen.py::_default_spawns` default to a single centred
226
+ mpspawn so position invariants hold without the workaround. Touches
227
+ default behaviour for every existing arena-generator pack โ€” out of
228
+ scope for a doc-cleanup pass.
229
+
230
+ ---
231
+
232
+ ## Finding 7 โ€” Pre-placed `proc` inside an `ore_patches:` disc silently empties patch
233
+
234
+ **Status on `main`:** BROKEN (with refined understanding).
235
+
236
+ `env.rs::build_world_for_episode` lines 958-1029 seed ore patches
237
+ FIRST (line 966-974: `seed_ore_patch` for each declared patch), then
238
+ inject scenario actors. A `proc` placed inside a patch disc occupies
239
+ its footprint cells via `terrain.occupy_footprint`, marking them
240
+ ground-impassable WITHOUT clearing the resource layer. Result: ore
241
+ IS placed (the original finding's "silently zero ore" wording is
242
+ slightly off), but `find_path` cannot reach those cells from the
243
+ harvester. The harvester loops "FindingOre โ†’ can't path โ†’ fail"
244
+ indefinitely on the cells under the proc footprint.
245
+
246
+ **Pinning:** `openra-data/tests/test_proc_overlapping_patch_warns.rs`
247
+ โ€” `#[ignore]`d. Loads a scenario with `proc` at (40,10) and
248
+ `ore_patches: [{x:40, y:10, amount:5000, radius:3}]`, asserts the
249
+ (currently nonexistent) `validate_layout` returns a warning string
250
+ containing `proc_overlaps_patch`. Flip to `#[test]` after the
251
+ validator ships.
252
+
253
+ **TODO engine fix:** add `oramap::validate_layout(&MapDef) -> Vec<String>`
254
+ that returns one warning per overlapping `(building, patch)` pair.
255
+ Surface via `OpenRAEnv.last_warnings` at world-build. Cheap; the
256
+ optional second fix (skip ore-seeding on cells already occupied by a
257
+ building footprint, or re-order seed-after-actor-injection) is more
258
+ invasive โ€” leave for later.
259
+
260
+ **Bench-side workaround:** trivial (place the proc just outside the
261
+ patch radius). Already widely applied; no specific files to clean
262
+ up.
263
+
264
+ ---
265
+
266
+ ## Pre-existing test failures (NOT this triage pass)
267
+
268
+ Documented for clarity so they aren't conflated with finding-related
269
+ failures:
270
+
271
+ - `openra-sim` lib test `gamerules::tests::defaults_have_all_common_units`
272
+ โ€” MCV vs Vehicle kind classification regression, predates this
273
+ pass (logged in `OpenRA-Bench/ENGINE_AUDIT.md`).
274
+ - `openra-sim` integration tests `sync_hash_verify` (ร—2),
275
+ `debug_orders_and_hashes` โ€” sync-hash reference fixtures stale
276
+ after recent merges.
277
+ - `openra-sim` integration test `tank_attack_terminates_when_no_armament_loaded`
278
+ + asset-loading data tests (`extract_*`, `find_*`,
279
+ `parse_infantry_yaml`, `e1_rifleman_typed_fields`,
280
+ `render_*`, etc.) โ€” all need the vendored OpenRA mod dir at
281
+ `OpenRA-Rust/vendor/OpenRA/mods/ra/` (not present in this
282
+ workspace; environmental, not code-related).
283
+ - `openra-data` `test_per_player_starting_cash`, etc. โ€” earlier these
284
+ failed with `MissingBaseMap` because the `rush-hour-arena.oramap`
285
+ fixture wasn't in `openra-data/tests/fixtures/`. Fixed in this pass
286
+ by copying the map from `OpenRA-Bench/data/maps/`. All four
287
+ per_player_starting_cash + 3 ore_patches data tests are green now.
288
+
289
+ ## Summary of changes shipped this pass
290
+
291
+ **Engine (OpenRA-Rust):**
292
+ - `openra-sim/src/world.rs::detonate_nuke` โ€” credit nuke AoE kills to
293
+ firing owner (mirrors projectile-resolve path; friendly-fire is
294
+ excluded).
295
+ - `openra-sim/tests/test_nuke_credits_kills.rs` โ€” new (2 tests).
296
+ - `openra-sim/tests/test_harvest_explicit_target.rs` โ€” new
297
+ (`#[ignore]`d, pins finding #4).
298
+ - `openra-data/tests/test_proc_overlapping_patch_warns.rs` โ€” new
299
+ (`#[ignore]`d, pins finding #7).
300
+ - `openra-data/tests/fixtures/rush-hour-arena.oramap` โ€” copied in
301
+ from `OpenRA-Bench/data/maps/` so existing data tests run cleanly
302
+ in this workspace.
303
+
304
+ **Bench (OpenRA-Bench):**
305
+ - `CLAUDE.md` โ€” finding #1 doc fix in two places.
306
+ - `openra_bench/scenarios/schema.py::MapDef.ore_patches` docstring โ€”
307
+ finding #2 doc fix (clamp formula + recommended ratio).
308
+ - `docs/ENGINE_FOLLOWUPS_TRIAGE.md` โ€” this file (refreshed against
309
+ post-merge HEAD).
310
+ - `docs/ENGINE_FOLLOWUPS.md` โ€” finding #6 reworded to drop the
311
+ misleading "engine offsets pre-placed actors" claim.
312
+
313
+ **TODOs surfaced:**
314
+ - finding #4: implement `bound_patch` on `Activity::Harvest` (see test).
315
+ - finding #5: empirical re-measure on `combat-kite-and-pull` to
316
+ verify whether the stance:1 fix is sufficient.
317
+ - finding #7: implement `oramap::validate_layout` (see test).
318
+ - finding #2: optional cap raise from 12 โ†’ 24/cell.
319
+ - spec-nuke-strike.yaml: switch hard-tier predicate to
320
+ `units_killed_gte` now that finding #3 is fixed.