From cb71a3cfad28cc12d2d6ca55e45cd1ba2d54f18c Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Fri, 25 Sep 2026 14:24:55 +0200 Subject: [PATCH] Cut 12.3 fix batch: unify Lane and Lanes on one membership query Self's review found that commit 4e6495dd's fix for F1 replaced the per-lane forward-arithmetic defect with a second, still-independent membership computation: Lane's own half-open admission (Lo <= s < Hi, from a cell's own Extent) versus Lanes' ceil((cell.Lo - centreLateral) / laneSpacing) lane index. The two agree only when a cell's own (Hi - Lo) equals the externally supplied laneSpacing exactly in float -- the identical two-derivations trap Cut 12.2's own fix batch already found and removed from Lane itself, reintroduced one layer up. Confirmed reachable: on commit 4e6495dd, solid5 b=(0,1) s=0.49999997 n=1 leaves the ceil-based centre lane EMPTY while Lane(s) itself finds all 5 cells of the row -- breaking R3 ("the centre lane always meets metal", Commit's own guard) and the 12.2 placement contract. FireControl.Lanes is now built entirely out of FireControl.Lane: it calls Lane(hull, b, centreLateral + k * laneSpacing, ...) once per lane index, so Lane(s) is literally this function's n = 0 case rather than a parallel re-implementation of its rule. Commit's placement and Apply's damage split now read one function. The narrower question left -- can two individually correct Lane() calls, at two different lines, still walk the same cell? -- is resolved by ownership (the lane closest to the centre keeps a contested cell; the centre lane, k = 0, never loses one), not by a competing formula, so the centre lane's final cell set is always exactly Lane(centreLateral)'s own output. Lane() itself is unchanged (still called directly by Commit and by FireControlCut12Tests' edge-seed and interval-edge tests, all still green: HitsLandOnTheFacingEdge, LaneSeamIsHalfOpen, LaneNeverEmptiesAtAnIntervalEdge, Cut12-edge-68442709, Cut12-edge-64078070). Adds LanesQueryIsDisjointMatchesLaneAtCentreAndNeverEmptyOnHull: now that Lanes is a public, pure geometric query, the boundary sweep is a behavioural test of its own contract, no reflection -- over every cell's own shadow edge and float neighbours, at exact-axis, 45-degree, |bx| > |by| bearings and Soul's own two reproduction cases, spread 1-2: (a) no cell is walked by two lanes, (b) the centre lane equals a plain Lane(s) call, (c) the centre lane is never empty when the centre lateral is inside the hull's own shadow. Hand- confirmed to fail on 4e6495dd's ceil-based Lanes (property (b), the same input above) and to pass on this commit (fc123fix2-mut3.log). Adds CommittedCellIsTheFirstCellTheCentreLaneDamages: an end-to-end check through Fire -> Commit -> Apply (damageSpread 0, a single lane) that the committed outcome.Cell is the first cell Apply damages. Full suite: 286 tests, identical across 3 runs. Co-Authored-By: Claude Opus 5.5 --- Assets/Scripts/ServerShared/FireControl.cs | 93 +++++++---- .../FireControlCut123Tests.cs | 146 ++++++++++++++++-- 2 files changed, 190 insertions(+), 49 deletions(-) diff --git a/Assets/Scripts/ServerShared/FireControl.cs b/Assets/Scripts/ServerShared/FireControl.cs index 2949ee8cd..77250d825 100644 --- a/Assets/Scripts/ServerShared/FireControl.cs +++ b/Assets/Scripts/ServerShared/FireControl.cs @@ -613,8 +613,15 @@ public static class FireControl // offset by repeated addition can shift a side lane by an ulp or two, and a shifted s(li) can land inside a // NEIGHBOURING cell's half-open interval that its own, unshifted s(li) was never meant to reach -- the same // "two derivations of one quantity" defect Cut 12.2's own fix batch found and removed from Lane itself - // (S1 above), now recurring one layer up. See `Lanes` below, which decides each cell's lane membership with - // one computation instead of one comparison per (cell, lane) pair. + // (S1 above), now recurring one layer up. + // Self's review (2026-09-25): the first fix for this replaced the per-lane Lane() loop with a ceil-based + // index formula that computed lane membership from a cell's own Extent directly -- but that formula and + // Lane's own half-open test (Lo <= s < Hi) were still two separate computations, agreeing only when a + // cell's own (Hi - Lo) equalled laneSpacing exactly in float. That is the identical trap one level removed, + // not removed. `Lanes` below is the actual single owner: it calls Lane() itself once per lane index, so + // Commit's placement (Lane, the n = 0 case) and Apply's damage split (Lanes, every case) read one function, + // and resolves the resulting rare cross-lane collision by ownership rather than by a competing formula -- + // see `Lanes`. private static void Apply(PendingShot shot) { if (!shot.Outcome.Hit) return; @@ -676,49 +683,67 @@ public static class FireControl target.DamageHull(hullDamage); } - // F1 fix batch (Soul's second pass, 2026-09-25): lane membership decided ONCE per cell, replacing the old - // loop that called Lane() once per lane index and let each call test its own forward-accumulated lateral - // against a cell's interval independently of every other lane. Here every candidate cell computes its own - // lane index directly from its own (unmodified) Extent interval: k = ceil((Lo - centreLateral) / - // laneSpacing) is the unique integer for which centreLateral + k * laneSpacing falls in [Lo, Hi) -- Lane's - // own half-open admission -- derived by one division and one Ceiling call from values neither multiplied - // nor summed by any other lane's arithmetic. A single Math.Ceiling call returns exactly one integer, so a - // cell can be assigned to at most one lane by construction, not by float luck: disjointness no longer - // depends on laneSpacing surviving repeated addition without drifting off a neighbour's boundary. - // When k == 0 this is the same real-valued condition Lane() tests for centreLateral itself - // (Lo <= centreLateral < Hi, i.e. Lo - centreLateral in (-laneSpacing, 0]), so the centre lane's cell set - // is unchanged from calling Lane(hull, bearing, centreLateral, ...) directly -- the 12.2 placement contract - // (HitsLandOnTheFacingEdge) still holds. laneSpacing is still used, unchanged, to place the admitted lane's - // own line for AlongBearing's slab solve -- geometry, not membership -- so a lane's entry/exit values and - // its walked-contiguity break are exactly as before. - private static int[] Lanes(HullData hull, float2 b, float centreLateral, int n, float laneSpacing, LaneCell[][] buffers) + // Self's fix (2026-09-25): the ONE public membership query behind both Commit's placement and Apply's + // damage split. Lane(s) is exactly this function's n = 0 case -- Lanes calls Lane(hull, b, centreLateral + + // k * laneSpacing, ...) once per lane index k, so a lane's own cell set can never disagree with what + // Lane(s) alone returns for that s: there is no second formula for "does this cell belong to this lane", + // only Lane's own half-open Extent test, called at 2n+1 different lines. This is what removes the trap a + // ceil-based index formula reintroduced one level up from Cut 12.2's own crash (see Apply's comment above): + // that formula and Lane's own test were still two computations, agreeing only when a cell's own (Hi - Lo) + // equalled the externally supplied laneSpacing exactly in float. + // What is left, once every lane's own result is individually exact, is a DIFFERENT and much narrower + // question: can two separate calls to Lane(), at two different (and individually correct) lines, still walk + // the SAME cell? Yes, rarely -- laneSpacing (F2's shadow width, one value shared by every lane) can differ + // by a few ulps from one specific cell's own computed (Hi - Lo), the same float drift Soul's second pass + // found. Lanes resolves that by ownership, not by a competing membership rule: a cell walked by more than + // one lane keeps the lane closest to the centre (ties are impossible -- equal |k| means the same lane), and + // the centre lane (k = 0) never loses a cell to a side lane. Its own final cell set is therefore always + // exactly Lane(centreLateral)'s own output, unmodified -- R3 (the centre lane always meets metal, Commit's + // own guard) and the 12.2 placement contract (HitsLandOnTheFacingEdge) hold structurally, not by chance. A + // side lane that cedes a cell simply omits it from its own walk; the walk's existing contiguity rule (a + // cell more than 1e-4 past the previous SURVIVING cell's exit ends the lane) then decides on its own whether + // the lane continues past the gap that leaves -- no new rule, the same one `Lane` already applies at a + // genuine gap. laneSpacing itself is untouched: it is still the one F2 value that places every lane's own + // query line. + public static int[] Lanes(HullData hull, float2 b, float centreLateral, int n, float laneSpacing, LaneCell[][] buffers) { - var ell = Lateral(b); - var coords = hull.Shape.Coordinates; var laneCount = 2 * n + 1; - var counts = new int[laneCount]; - for (var i = 0; i < coords.Length; i++) + var raw = new LaneCell[laneCount][]; + var rawCount = new int[laneCount]; + for (var li = 0; li < laneCount; li++) { - var c = (float2) coords[i]; - var lateral = Extent(c, ell); - var k = (int) Math.Ceiling((lateral.Lo - centreLateral) / laneSpacing); - if (k < -n || k > n) continue; - var li = k + n; + var k = li - n; var s = centreLateral + k * laneSpacing; - AlongBearing(c, b, ell, s, out var entry, out var exit); - buffers[li][counts[li]++] = new LaneCell { Cell = coords[i], Entry = entry, Exit = exit, Projection = dot(c, b) }; + raw[li] = ArrayPool.Shared.Rent(hull.Shape.Coordinates.Length); + rawCount[li] = Lane(hull, b, s, raw[li]); + } + + // Ownership: whichever lane is closest to the centre keeps a cell two lanes both walked; the centre + // lane (k = 0, |k| = 0) can never be outranked. + var ownerLane = new Dictionary(); + for (var li = 0; li < laneCount; li++) + { + var k = Math.Abs(li - n); + for (var i = 0; i < rawCount[li]; i++) + { + var cell = raw[li][i].Cell; + if (!ownerLane.TryGetValue(cell, out var curLi) || Math.Abs(curLi - n) > k) + ownerLane[cell] = li; + } } var walked = new int[laneCount]; for (var li = 0; li < laneCount; li++) { - Array.Sort(buffers[li], 0, counts[li]); - walked[li] = counts[li] > 0 ? 1 : 0; - for (var i = 1; i < counts[li]; i++) + var w = 0; + for (var i = 0; i < rawCount[li]; i++) { - if (buffers[li][i].Entry > buffers[li][i - 1].Exit + 1e-4f) break; - walked[li]++; + if (ownerLane[raw[li][i].Cell] != li) continue; // ceded to a closer lane -- this lane's own gap + if (w > 0 && raw[li][i].Entry > buffers[li][w - 1].Exit + 1e-4f) break; + buffers[li][w++] = raw[li][i]; } + walked[li] = w; + ArrayPool.Shared.Return(raw[li]); } return walked; } diff --git a/tests/Aetheria.Shared.Tests/FireControlCut123Tests.cs b/tests/Aetheria.Shared.Tests/FireControlCut123Tests.cs index 0b2ebf4db..ea151fcf9 100644 --- a/tests/Aetheria.Shared.Tests/FireControlCut123Tests.cs +++ b/tests/Aetheria.Shared.Tests/FireControlCut123Tests.cs @@ -953,21 +953,137 @@ public sealed class FireControlCut123Tests : IDisposable // Lateral's draw visits many different points along the hull's shadow), and ample penetration. Heavy // uniform armour so every reached cell fires exactly one ArmorDamage event, and a cell reached by two lanes // in the SAME shot fires twice. - // F1's own structural fix (`FireControl.Lanes`) assigns each cell to exactly one lane index by a single - // computation, so after that fix a wrong `laneSpacing` value can misassign a cell to the wrong lane or drop - // it, but can no longer make two lanes claim the SAME cell -- double-admission is not a reachable outcome - // of a bad spacing constant any more, only of a defect in the single-computation assignment itself. Hand - // verification (fc123fix2-mut.log) confirms this split: reverting the spacing to one cell (R0), or to a - // wrong formula (R1c/R1e/R1h, all `dotnet test`), is caught by the pre-existing damage-shape tests - // (`DirectHitAtAnAngledBearingMatchesAnIndependentModel`, `FootprintWidensAtAnAngledBearingComparedToAxisAligned`, - // and others) rather than by this test -- this test never failed for any of those four reverts, because the - // hull it drove genuinely was never struck twice under any of them. A 1% narrowing (R1b) survives the - // whole 97-test suite, including this one; recorded as an unresolved survivor rather than claimed dead (an - // honest gap, not a claimed kill), left for Soul's own mutation pass. This test's own job is narrower than - // that: it is a standing regression guard on the disjointness invariant itself, through the real - // Fire/Commit/Apply pipeline, so a future change that reintroduces per-lane admission testing (the shape of - // the original F1 defect) has a chance of being caught here even though it can no longer be manufactured by - // hand through a spacing-constant mutation. + // F1's own structural fix (`FireControl.Lanes`, now built on top of `FireControl.Lane` itself -- see that + // function's own comment) makes double-admission unreachable from a bad `laneSpacing` value: a wrong + // spacing can misassign a cell to the wrong lane or drop it, but can no longer make two lanes claim the + // SAME cell. Hand verification (fc123fix2-mut.log) confirms this split: reverting the spacing to one cell + // (R0), or to a wrong formula (R1c/R1e/R1h, all `dotnet test`), is caught by the pre-existing damage-shape + // tests (`DirectHitAtAnAngledBearingMatchesAnIndependentModel`, + // `FootprintWidensAtAnAngledBearingComparedToAxisAligned`, and others) rather than by this test -- this test + // never failed for any of those four reverts, because the hull it drove genuinely was never struck twice + // under any of them. A 1% narrowing (R1b) survives the whole 97-test suite, including this one; recorded as + // an unresolved survivor rather than claimed dead (an honest gap, not a claimed kill), left for Soul's own + // mutation pass. This test's own job is narrower than that: it is a standing regression guard on the + // disjointness invariant itself, through the real Fire/Commit/Apply pipeline, so a future change that + // reintroduces per-lane admission testing (the shape of the original F1 defect) has a chance of being + // caught here even though it can no longer be manufactured by hand through a spacing-constant mutation. + + // ===================== F1 (Self's review): Lane and Lanes are one membership query ===================== + // Self's finding, 2026-09-25: the first F1 fix (commit 4e6495dd) removed the per-lane forward-arithmetic + // defect but replaced it with a SECOND membership formula -- lane index k = ceil((cell.Lo - centreLateral) + // / laneSpacing), agreeing with FireControl.Lane's own half-open admission (Lo <= s < Hi) only when a + // cell's own (Hi - Lo) equals laneSpacing exactly in float. `FireControl.Lanes` is now built entirely out + // of `FireControl.Lane` -- it calls Lane() once per lane index, so Lane(s) IS the n = 0 case, not a second + // implementation of it, and any remaining cross-lane collision (still possible, since laneSpacing is one + // shared value and one specific cell's own Hi - Lo can still drift a few ulps from it) is resolved by + // ownership (closest to the centre wins; the centre lane never loses), not by a competing formula. This + // test pins that contract directly through the PUBLIC FireControl.Lanes and FireControl.Lane, no reflection + // and no production Lane/Apply used to build an expected value -- the assertions are properties of the + // functions' own output: (a) no cell is ever walked by two lanes, (b) the centre lane's cells are always + // exactly what a plain Lane(s) call returns, (c) the centre lane is never empty when the centre lateral is + // inside the hull's own shadow (Extent/Lateral's documented formula -- ell = (-by, bx), h = (|ellx|+|elly|) + // / 2, interval = [dot(c,ell)-h, dot(c,ell)+h) -- is independently recomputed here purely to build candidate + // laterals). Hand-confirmed to fail on commit 4e6495dd's ceil-based Lanes (fc123fix2-mut3.log): property (b) + // fails partway through the deterministic sweep below (solid5, b=(0,1), s=0.49999997, n=1 -- the ceil + // formula leaves the centre lane empty while Lane(s) itself finds all 5 cells of the row, the exact "centre + // lane always meets metal" break the ceil design could cause). Soul's own two reproduction cases below + // happen to still agree on that specific ceil build; the sweep is what catches it. + [Fact] + public void LanesQueryIsDisjointMatchesLaneAtCentreAndNeverEmptyOnHull() + { + static float2 Ell(float2 b) => float2(-b.y, b.x); + static (float Lo, float Hi) CellExtent(int2 c, float2 ell) + { + var h = (Math.Abs(ell.x) + Math.Abs(ell.y)) / 2f; + var centre = dot((float2) c, ell); + return (centre - h, centre + h); + } + static float Spacing(float2 b) { var ell = Ell(b); return Math.Abs(ell.x) + Math.Abs(ell.y); } + + void Check(string name, Shape shape, float2 b, float s, int n) + { + var hull = new HullData { Name = name, HullType = HullType.Ship, Shape = shape }; + var spacing = Spacing(b); + var laneCount = 2 * n + 1; + var buffers = new LaneCell[laneCount][]; + for (var li = 0; li < laneCount; li++) buffers[li] = new LaneCell[shape.Coordinates.Length]; + var walked = FireControl.Lanes(hull, b, s, n, spacing, buffers); + + // (a) no cell appears in two lanes. + var seen = new HashSet(); + for (var li = 0; li < laneCount; li++) + for (var i = 0; i < walked[li]; i++) + Assert.True(seen.Add(buffers[li][i].Cell), + $"{name} b=({b.x:R},{b.y:R}) s={s:R} n={n}: cell {buffers[li][i].Cell} walked by two lanes (lane {li - n})"); + + // (b) the centre lane equals a plain Lane(s) call. + var solo = new LaneCell[shape.Coordinates.Length]; + var soloWalked = FireControl.Lane(hull, b, s, solo); + Assert.True(soloWalked == walked[n], $"{name} b=({b.x:R},{b.y:R}) s={s:R} n={n}: centre lane walked {walked[n]}, Lane(s) walked {soloWalked}"); + for (var i = 0; i < soloWalked; i++) + Assert.Equal(solo[i].Cell, buffers[n][i].Cell); + + // (c) the centre lane is never empty when s is inside the hull's own shadow. + var ell = Ell(b); + var insideShadow = shape.Coordinates.Any(c => { var (lo, hi) = CellExtent(c, ell); return s >= lo && s < hi; }); + if (insideShadow) + Assert.True(walked[n] > 0, $"{name} b=({b.x:R},{b.y:R}) s={s:R} n={n}: centre lane empty although s is inside the hull's own shadow"); + } + + // Soul's own concrete reproductions (SoulApply123b.Disjoint, 2026-09-25). + Check("solid5", SolidShape(5, 5), float2(0, 1), -0.50000006f, 1); + Check("solid9", SolidShape(9, 9), normalize(float2(-0.69478226f, 0.7192201f)), -5.619354f, 1); + + // A deterministic sweep at every cell's own shadow edge (Lo and Hi) and its immediate float neighbours, + // across several hulls, bearings (exact axis, 45 degrees, |bx| > |by|, and Soul's own angled example), + // and spreads 1-2. + foreach (var (name, shape) in new (string, Shape)[] { ("solid5", SolidShape(5, 5)), ("solid9", SolidShape(9, 9)), ("solid3x11", SolidShape(3, 11)) }) + { + var coords = shape.Coordinates; + foreach (var b in new[] { float2(0, 1), float2(1, 0), normalize(float2(1, 1)), normalize(float2(2, 1)), normalize(float2(-0.69478226f, 0.7192201f)) }) + { + var ell = Ell(b); + foreach (var c in coords) + { + var (lo, hi) = CellExtent(c, ell); + foreach (var edge in new[] { lo, hi, MathF.BitIncrement(lo), MathF.BitDecrement(lo), MathF.BitIncrement(hi), MathF.BitDecrement(hi) }) + for (var n = 1; n <= 2; n++) + Check(name, shape, b, edge, n); + } + } + } + } + + // F1 (Self's review): the committed Cell (Commit's own placement) is the first cell Apply damages in the + // centre lane -- end to end through Fire -> Commit -> Apply, no reflection. DamageSpread 0 (a single lane, + // the centre lane) so the first ArmorDamage event of the whole shot is unambiguously the centre lane's own + // first cell -- Apply's damage loop processes lanes index 0 upward, so with a spread this check would be + // reading a SIDE lane's own first event, not the centre lane's. Heavy uniform armour and ample penetration + // so that first event is well defined. + [Fact] + public void CommittedCellIsTheFirstCellTheCentreLaneDamages() + { + var bearings = new[] + { + float2(0, 1), normalize(float2(1, 1)), normalize(float2(-1, 1)), + normalize(float2(2, 1)), normalize(float2(1, 2)) + }; + foreach (var travelDirection in bearings) + { + var shape = SolidShape(9, 9); + var e = Build(TestSettings(), shape, precision: 1f, penetration: 100f, damageSpread: 0f); + e.Shooter.Position = e.Target.Position - float3(travelDirection.x, 0, travelDirection.y) * 100f; + foreach (var c in shape.Coordinates) { e.Target.Armor[c.x, c.y] = 1000f; e.Target.MaxArmor[c.x, c.y] = 1000f; } + + var firstHit = new int2?[1]; + using var s = e.Target.ArmorDamage.Subscribe(x => { if (firstHit[0] == null) firstHit[0] = x.pos; }); + var outcome = FireUntilHit(e, damageOverride: 50f); + Assert.NotNull(outcome); + Assert.True(outcome.Hit); + Assert.NotNull(firstHit[0]); + Assert.Equal(outcome.Cell, firstHit[0].Value); + } + } [Fact] public void NoCellIsStruckByTwoLanesInOneShotAtAnyBearing() {