From a9d1d0a823174399e6b5050c34fe6941c7fbc821 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 7 Sep 2026 17:56:27 +0200 Subject: [PATCH] =?UTF-8?q?fix(vtank):=20slice=207=20round=20E=20item=20D-?= =?UTF-8?q?1=20=E2=80=94=20map=20legacy=20JSON=20recall=20ordinals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-cutover RouteRecallKind was {Lifestone=0, Marketplace=1, PrimaryPortal=2, SecondaryPortal=3} — completely different kinds at the SAME ordinals as today's much larger round-D enum (PrimaryPortalRecall=0, SecondaryPortalRecall=1, LifestoneRecall=2, LifestoneSending=3, ...). LegacyWaypointDocument.Recall was still typed RouteRecallKind, so System.Text.Json deserialized a legacy JSON route's bare "Recall" integer straight into the new enum — silently remapping every one of the four old ordinals to the wrong new recall. Enum.IsDefined never caught it (0..3 are all valid members of the new enum too, just for different spells). The field is now a bare int, translated through an explicit MapLegacyRecall(legacyOrdinal) table in ToWaypoint() that also derives RecallSpellId/RecallSpellName from the mapped kind (the pre-cutover schema predates both fields entirely). Mutation shown to fail first: reverting the source fix (git checkout, patch saved and reapplied) reproduced the bug for all four legacy ordinals — ordinal 2 (old PrimaryPortal) even landed on LifestoneRecall instead of PrimaryPortalRecall, confirming the "still IsDefined, still wrong" failure mode. The new theory asserts against the migrated route's own written .af "rcl" line (RecallDisplayName) rather than the value re-parsed back through LoadCurrent, since the .af format's own name-only round-trip is a separate, pre-existing limitation (no spell id, and "Marketplace Recall" cannot resolve through any catalog because it isn't a real spell) unrelated to this fix. MossTank suite 716 -> 720; App markup/plugin filter holds 243/243. Co-Authored-By: Claude Fable 5.1 --- .../MossTankRouteProfileStore.cs | 102 +++++++++++------- .../NavigationTests.cs | 66 ++++++++++++ 2 files changed, 132 insertions(+), 36 deletions(-) diff --git a/src/AcDream.Plugins.MossTank/MossTankRouteProfileStore.cs b/src/AcDream.Plugins.MossTank/MossTankRouteProfileStore.cs index c741ce04f..778d0e045 100644 --- a/src/AcDream.Plugins.MossTank/MossTankRouteProfileStore.cs +++ b/src/AcDream.Plugins.MossTank/MossTankRouteProfileStore.cs @@ -574,7 +574,19 @@ internal sealed class MossTankRouteProfileStore public bool LegacyReferenceValid { get; set; } = true; public string Text { get; set; } = string.Empty; public int DurationMilliseconds { get; set; } = 5000; - public RouteRecallKind Recall { get; set; } + // Round E item D-1: this field used to be typed RouteRecallKind, + // which let System.Text.Json deserialize the pre-cutover JSON's + // bare integer straight into TODAY's enum. The pre-cutover enum + // was {Lifestone=0, Marketplace=1, PrimaryPortal=2, + // SecondaryPortal=3} — completely different kinds at the SAME + // ordinals as the round-D enum (PrimaryPortalRecall=0, + // SecondaryPortalRecall=1, LifestoneRecall=2, LifestoneSending=3, + // ...), so every legacy route silently remapped to the WRONG new + // recall and Enum.IsDefined never caught it — 0..3 are all still + // valid members of the new enum, just for different spells. Read + // as a bare ordinal here instead and translate it explicitly via + // MapLegacyRecall in ToWaypoint(). + public int Recall { get; set; } public uint RecallSpellId { get; set; } public string RecallSpellName { get; set; } = string.Empty; public float JumpHeadingDegrees { get; set; } @@ -582,42 +594,60 @@ internal sealed class MossTankRouteProfileStore public int JumpChargeMilliseconds { get; set; } = 1000; public RouteJumpDirection JumpDirection { get; set; } - public RouteWaypoint ToWaypoint() => new() + public RouteWaypoint ToWaypoint() { - Type = Enum.IsDefined(Type) ? Type : RouteWaypointType.Point, - Position = new PluginNavigationPosition( - CellId, EastWest, NorthSouth, Elevation, HeadingDegrees, IsOutdoor), - ReferencePosition = new PluginNavigationPosition( - ReferenceCellId, - ReferenceEastWest, - ReferenceNorthSouth, - ReferenceElevation, - ReferenceHeadingDegrees, - ReferenceIsOutdoor), - ObjectId = ObjectId, - ObjectName = ObjectName ?? string.Empty, - LegacyObjectClass = LegacyObjectClass, - LegacyReferenceValid = LegacyReferenceValid, - Text = Text ?? string.Empty, - DurationMilliseconds = Math.Clamp(DurationMilliseconds, 0, 3_600_000), - // Round D item 3 removed RouteRecallKind.Lifestone (the old - // /lifestone-slash-command member, superseded by the real - // LifestoneRecall spell) — a pre-cutover un-migrated JSON - // profile carrying that ordinal now falls back to - // PrimaryPortalRecall instead, the same "still a valid, - // harmless recall kind" sentinel role Lifestone played here. - Recall = Enum.IsDefined(Recall) ? Recall : RouteRecallKind.PrimaryPortalRecall, - RecallSpellId = RecallSpellId, - RecallSpellName = RecallSpellName ?? string.Empty, - JumpHeadingDegrees = float.IsFinite(JumpHeadingDegrees) ? JumpHeadingDegrees : 0f, - JumpRun = JumpRun, - // Round 3 item 5: no load-time clamp here either — the real - // bi.a 2000 ms ceiling is retail's EXECUTION-time behavior - // (NavigationController.TickJump), not a storage-format limit; - // this legacy migration path preserves whatever the pre-cutover - // JSON authored, exactly like the .af load path now does. - JumpChargeMilliseconds = JumpChargeMilliseconds, - JumpDirection = Enum.IsDefined(JumpDirection) ? JumpDirection : RouteJumpDirection.Forward, + RouteRecallKind recall = MapLegacyRecall(Recall); + return new RouteWaypoint + { + Type = Enum.IsDefined(Type) ? Type : RouteWaypointType.Point, + Position = new PluginNavigationPosition( + CellId, EastWest, NorthSouth, Elevation, HeadingDegrees, IsOutdoor), + ReferencePosition = new PluginNavigationPosition( + ReferenceCellId, + ReferenceEastWest, + ReferenceNorthSouth, + ReferenceElevation, + ReferenceHeadingDegrees, + ReferenceIsOutdoor), + ObjectId = ObjectId, + ObjectName = ObjectName ?? string.Empty, + LegacyObjectClass = LegacyObjectClass, + LegacyReferenceValid = LegacyReferenceValid, + Text = Text ?? string.Empty, + DurationMilliseconds = Math.Clamp(DurationMilliseconds, 0, 3_600_000), + Recall = recall, + // The pre-cutover JSON schema predates RecallSpellId/ + // RecallSpellName entirely (both are a round-D addition), + // so a legacy document never carries real values for + // them — derive both from the SAME mapped kind, the one + // table this migration path uses end-to-end. + RecallSpellId = RouteWaypoint.SpellIdForRecall(recall), + RecallSpellName = RouteWaypoint.RecallDisplayName(recall), + JumpHeadingDegrees = float.IsFinite(JumpHeadingDegrees) ? JumpHeadingDegrees : 0f, + JumpRun = JumpRun, + // Round 3 item 5: no load-time clamp here either — the real + // bi.a 2000 ms ceiling is retail's EXECUTION-time behavior + // (NavigationController.TickJump), not a storage-format limit; + // this legacy migration path preserves whatever the pre-cutover + // JSON authored, exactly like the .af load path now does. + JumpChargeMilliseconds = JumpChargeMilliseconds, + JumpDirection = Enum.IsDefined(JumpDirection) ? JumpDirection : RouteJumpDirection.Forward, + }; + } + + // Round E item D-1: the pre-cutover RouteRecallKind ordinals, + // mapped to today's real spell-based kinds. Anything outside that + // 4-value range is impossible (the old enum only ever had 4 + // members) so it falls back to PrimaryPortalRecall — the same + // "still a valid, harmless recall kind" sentinel role the old + // Enum.IsDefined guard played before this fix. + private static RouteRecallKind MapLegacyRecall(int legacyOrdinal) => legacyOrdinal switch + { + 0 => RouteRecallKind.LifestoneRecall, // old Lifestone + 1 => RouteRecallKind.Marketplace, // old Marketplace + 2 => RouteRecallKind.PrimaryPortalRecall, // old PrimaryPortal + 3 => RouteRecallKind.SecondaryPortalRecall, // old SecondaryPortal + _ => RouteRecallKind.PrimaryPortalRecall, }; } } diff --git a/tests/AcDream.Plugins.MossTank.Tests/NavigationTests.cs b/tests/AcDream.Plugins.MossTank.Tests/NavigationTests.cs index 1f359e8ee..eaef36935 100644 --- a/tests/AcDream.Plugins.MossTank.Tests/NavigationTests.cs +++ b/tests/AcDream.Plugins.MossTank.Tests/NavigationTests.cs @@ -567,6 +567,72 @@ public sealed class NavigationTests Assert.Single(target.Waypoints); } + /// + /// D-1 (round E architecture re-check): the pre-cutover + /// RouteRecallKind was {Lifestone=0, Marketplace=1, PrimaryPortal=2, + /// SecondaryPortal=3} — completely different kinds at the SAME + /// ordinals as today's much larger enum (PrimaryPortalRecall=0, + /// SecondaryPortalRecall=1, LifestoneRecall=2, LifestoneSending=3, + /// ...). Before this fix, deserializing a legacy JSON route's bare + /// "Recall" integer straight into the new enum type silently remapped + /// every one of the four old ordinals to the WRONG new recall (and + /// Enum.IsDefined never caught it, since 0..3 are all still valid new + /// members). Each of the four legacy ordinals must migrate to its + /// real VTank recall. + /// + /// Migration writes the converted route straight to the real .af file + /// (MigrateLegacyIfNeeded), and LoadCurrent immediately + /// re-parses THAT file to build the returned + /// — so asserting against the re-parsed waypoint would really be + /// testing the .af "rcl" node's own name-only round-trip (a pre- + /// existing, unrelated limitation: it carries no spell id at all, and + /// "Marketplace Recall" can never resolve through ANY catalog because + /// it is not a real spell — see ), + /// not this fix. The direct, catalog-independent proof that + /// MapLegacyRecall chose the right kind is the WRITTEN .af + /// text itself: its "rcl" line carries exactly + /// for that kind. + /// + [Theory] + [InlineData(0, (int)RouteRecallKind.LifestoneRecall)] // old Lifestone + [InlineData(1, (int)RouteRecallKind.Marketplace)] // old Marketplace + [InlineData(2, (int)RouteRecallKind.PrimaryPortalRecall)] // old PrimaryPortal + [InlineData(3, (int)RouteRecallKind.SecondaryPortalRecall)] // old SecondaryPortal + public void LegacyJsonRouteMigratesOldRecallOrdinalToTheRightNewKind( + int legacyOrdinal, + int expectedKindOrdinal) + { + // xunit only discovers PUBLIC [Theory] methods, and RouteRecallKind + // is internal — a public parameter of that type is a compile error + // (CS0051), so InlineData passes ordinals (public ints) and the + // enum is recovered here instead (matching + // RecallNameAndSpellIdTablesAgree's own pattern above). + var expectedKind = (RouteRecallKind)expectedKindOrdinal; + var storage = new MemoryStorage(); + string legacyKey = LegacyRouteByCharacterKey("Barris"); + storage.Text[legacyKey] = $$""" + { + "Mode": 1, + "Waypoints": [ + { "Type": 2, "EastWest": 1.0, "NorthSouth": 2.0, "Recall": {{legacyOrdinal}} } + ] + } + """; + + var store = new MossTankRouteProfileStore(new FakeHost(new FakeAutomation(), storage)); + Assert.True(store.BindCharacter("Barris")); + var target = new NavigationSettings(); + Assert.True(store.LoadCurrent(target, MetafSerializer.NoOpSpells.Instance)); + + RouteWaypoint waypoint = Assert.Single(target.Waypoints); + Assert.Equal(RouteWaypointType.Recall, waypoint.Type); + + string fileName = "navs/" + VtankProfileDirectory.AutoCharacterFileName( + "Barris", string.Empty, "af"); + Assert.True(storage.Text.TryGetValue(fileName, out string? af)); + Assert.Contains($"{{{RouteWaypoint.RecallDisplayName(expectedKind)}}}", af); + } + /// /// Reproduces MossTankRouteProfileStore's pre-cutover named-profile JSON /// hash key (its own LegacyProfileKey is private; the format is