diff --git a/src/AcDream.App/Net/RetailSkillFormula.cs b/src/AcDream.App/Net/RetailSkillFormula.cs index de9842e2..70dc47ef 100644 --- a/src/AcDream.App/Net/RetailSkillFormula.cs +++ b/src/AcDream.App/Net/RetailSkillFormula.cs @@ -130,10 +130,24 @@ internal static class RetailSkillFormula { ArgumentNullException.ThrowIfNull(formula); - bool hasAttr1 = formula.Attribute1Multiplier >= 1 - && formula.Attribute1 != 0; - bool hasAttr2 = formula.Attribute2Multiplier >= 1 - && formula.Attribute2 != 0; + // F13 (night-round review): read the SAME unsigned reinterpretation + // TryCalculate above uses — this class's own doc comment already + // states the invariant ("DAT reader fields are signed storage + // views... deliberately reinterpreted as retail's unsigned W/X/Y/Z + // words") but this method previously read the raw signed int + // fields directly. A high-bit-set multiplier/divisor/bonus would + // both mis-gate hasAttr1/hasAttr2 (reads negative, failing the + // >= 1 check TryCalculate's unsigned reinterpretation would have + // passed) and print the wrong (negative) number — out of sync with + // the value TryCalculate actually computes with for the SAME + // formula. + uint x = unchecked((uint)formula.Attribute1Multiplier); + uint y = unchecked((uint)formula.Attribute2Multiplier); + uint w = unchecked((uint)formula.AdditiveBonus); + uint divisor = unchecked((uint)formula.Divisor); + + bool hasAttr1 = x >= 1 && formula.Attribute1 != 0; + bool hasAttr2 = y >= 1 && formula.Attribute2 != 0; if (!hasAttr1 && !hasAttr2) return null; @@ -144,9 +158,9 @@ internal static class RetailSkillFormula if (hasAttr1) { string name1 = AttributeName(formula.Attribute1); - text.Append(formula.Attribute1Multiplier <= 1 + text.Append(x <= 1 ? name1 - : $"({formula.Attribute1Multiplier} x {name1})"); + : $"({x} x {name1})"); if (hasAttr2) text.Append(" + "); } @@ -154,17 +168,17 @@ internal static class RetailSkillFormula if (hasAttr2) { string name2 = AttributeName(formula.Attribute2); - text.Append(formula.Attribute2Multiplier <= 1 + text.Append(y <= 1 ? name2 - : $"({formula.Attribute2Multiplier} x {name2})"); + : $"({y} x {name2})"); } if (hasAttr1 && hasAttr2) text.Append(')'); - if (formula.Divisor != 1) - text.Append($" / {formula.Divisor}"); - if (formula.AdditiveBonus != 0) - text.Append($"+{formula.AdditiveBonus}"); + if (divisor != 1) + text.Append($" / {divisor}"); + if (w != 0) + text.Append($"+{w}"); text.Append(" )"); return text.ToString(); } diff --git a/src/AcDream.App/UI/Layout/MapPageController.cs b/src/AcDream.App/UI/Layout/MapPageController.cs index b3bfc397..53c577c1 100644 --- a/src/AcDream.App/UI/Layout/MapPageController.cs +++ b/src/AcDream.App/UI/Layout/MapPageController.cs @@ -243,6 +243,18 @@ public sealed class MapPageController // proven-working skin UiItemSlot already hardcodes. if (marker is UiButton markerButton) markerButton.TooltipText = loc.Name; + else + // F11 (night-round review): silently skipping the runtime- + // text write here would leave the marker's popup mounted + // (AuthoredTooltipRootElementId/LayoutDid are still set + // below) but genuinely EMPTY — a live-DAT template change + // that resolves 0x100001F0 to something other than a + // UiButton would regress every town-marker tooltip with no + // diagnostic signal at all. + Console.WriteLine( + $"[D.2b] Map tab: town marker '{loc.Name}' template " + + $"resolved to {marker.GetType().Name}, not UiButton — " + + "TooltipText cannot be set, marker will show no tooltip."); marker.AuthoredTooltipRootElementId = RetailTooltipPresenter.SharedPopupSkinRootElementId; marker.AuthoredTooltipLayoutDid = RetailTooltipPresenter.SharedPopupSkinLayoutDid; _map!.AddChild(marker); @@ -299,9 +311,21 @@ public sealed class MapPageController : name; } + /// + /// gmMapUI::Update @0x004a2078's gate is + /// if (m_pCoordinateText != 0 && m_pPlayerLocationIcon != 0) + /// — BOTH widgets present, not "at least one". Night-round review F15: + /// the prior _coordinateText is null && _playerIcon is null + /// check only skipped this method when BOTH were absent (De Morgan's: + /// it PROCEEDED whenever EITHER was present), so a page missing one of + /// the two would still write the other's state independently — retail + /// updates NEITHER when either is missing (no coordinate-text write, + /// no marker show/hide) since the whole outside/inside branch, + /// including its inside-branch fallback, lives inside this one gate. + /// private void RefreshCoordinatesAndPlayerMarker() { - if (_coordinateText is null && _playerIcon is null) return; + if (_coordinateText is null || _playerIcon is null) return; bool outside = RadarCoordinates.TryFromCell(_bindings.PlayerCellId(), out RadarCoordinates coords); if (outside) @@ -315,11 +339,47 @@ public sealed class MapPageController // player marker (gmMapUI::Update's else branch, // m_pPlayerLocationIcon->SetVisible(0)). _lastCoordinateText = string.Empty; - if (_playerIcon is not null) - _playerIcon.Visible = false; + _playerIcon.Visible = false; } } + /// + /// gmMapUI::Update @0x004a22a6-f6: Position::get_outside_cell_id + /// (&m_HousePosition) -> LandDefs::gid_to_lcoord -> the SAME + /// (v-0x400)*0.1+0.5 transform 's player + /// branch uses. + /// + /// + /// Night-round review F14: this passes housePosition.Value.LandblockId + /// straight to , SKIPPING the + /// Position::get_outside_cell_id @0x004527b0 step retail's own + /// call chain names. That function is itself BN-mangled (its final + /// return ((eax_2 - eax_2) & objcell_id) — an always-zero + /// subtraction ANDed with the cell id — is textbook Binary Ninja + /// obscuring a real conditional the raw bytes would need to + /// disassemble to recover, the same artifact class F1/F3 hit + /// elsewhere this round) and depends on LandDefs::adjust_to_outside, + /// which takes the position's raw world XYZ (not just the landblock + /// id) — a genuinely different, larger port than this round's other + /// findings, not a one-line fix. Documenting the gap rather than + /// guessing at the byte-decode (per this finding's own explicit + /// escape hatch): is wired + /// () => null in production today (ISSUES #413's remaining + /// owned-house scope), so this whole method is UNREACHABLE live — + /// there is no current behavioral gap to observe, only a latent one + /// for whenever HousePosition gets wired to real HouseData. TODO: + /// when that lands, port Position::get_outside_cell_id / + /// LandDefs::adjust_to_outside (byte-decode required, + /// @0x004527b0 / call site @0x004a2297) instead of + /// passing the raw landblock id through — for a genuinely outdoor + /// house position this simplification is very likely already exact + /// (an outdoor position has nothing for adjust_to_outside to + /// adjust), but that has not been byte-confirmed, and an indoor + /// house-interior recall position would need the real conversion + /// rather than this method's current fail-safe (hide the marker, + /// since correctly refuses + /// any cell with an envcell low word). + /// private void RefreshHouseMarker() { if (_houseIcon is null) return; @@ -331,9 +391,6 @@ public sealed class MapPageController return; } - // Position::get_outside_cell_id(&m_HousePosition) -> gid_to_lcoord - // -> the SAME (v-0x400)*0.1+0.5 transform PlaceMarkerOnMap's player - // branch uses (gmMapUI::Update @0x004a22a6-f6). if (!RadarCoordinates.TryFromCell(housePosition.Value.LandblockId, out RadarCoordinates coords)) { _houseIcon.Visible = false; diff --git a/tests/AcDream.App.Tests/Net/RetailSkillFormulaTests.cs b/tests/AcDream.App.Tests/Net/RetailSkillFormulaTests.cs index 8b4dc7ab..66b10dc1 100644 --- a/tests/AcDream.App.Tests/Net/RetailSkillFormulaTests.cs +++ b/tests/AcDream.App.Tests/Net/RetailSkillFormulaTests.cs @@ -74,6 +74,46 @@ public sealed class RetailSkillFormulaTests Assert.Equal(uint.MaxValue, result); } + /// + /// F13 (night-round review): + /// previously read the raw signed int fields directly instead of + /// the SAME unsigned reinterpretation + /// uses. A multiplier whose stored bit pattern has the high bit set + /// would read as a small negative number here (failing the + /// >= 1 hasAttr gate, or printing a negative multiplier) instead + /// of the huge unsigned value TryCalculate actually computes + /// with for the SAME formula. + /// + [Fact] + public void FormatFormula_MultiplierIsReinterpretedAsRetailUnsignedWord() + { + SkillFormula formula = Formula(w: 0, x: -1, y: 0, z: 1); + formula.Attribute1 = AttributeId.Strength; + + string? text = RetailSkillFormula.FormatFormula(formula); + + Assert.NotNull(text); + Assert.Contains(uint.MaxValue.ToString(), text); + Assert.DoesNotContain("-1", text); + } + + /// Same reinterpretation, but for the divisor and additive-bonus + /// suffixes rather than the multiplier — both must read unsigned too. + [Fact] + public void FormatFormula_DivisorAndAdditiveBonusAreReinterpretedAsRetailUnsignedWords() + { + SkillFormula formula = Formula(w: -1, x: 1, y: 0, z: unchecked((uint)-2)); + formula.Attribute1 = AttributeId.Strength; + + string? text = RetailSkillFormula.FormatFormula(formula); + + Assert.NotNull(text); + Assert.Contains($"/ {unchecked((uint)-2)}", text); + Assert.Contains($"+{uint.MaxValue}", text); + Assert.DoesNotContain("-1", text); + Assert.DoesNotContain("-2", text); + } + [Fact] public void LiveResolverLooksUpTheDatFormulaAndTreatsMissingAttributesAsZero() { diff --git a/tests/AcDream.App.Tests/UI/Layout/MapPageControllerTests.cs b/tests/AcDream.App.Tests/UI/Layout/MapPageControllerTests.cs index a81ddb0d..3b7146cb 100644 --- a/tests/AcDream.App.Tests/UI/Layout/MapPageControllerTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/MapPageControllerTests.cs @@ -229,6 +229,49 @@ public sealed class MapPageControllerTests Assert.False(playerIcon!.Visible); } + [Fact] + public void Refresh_PlayerIconTemplateResolutionFails_CoordinateTextStaysEmptyToo() + { + // F15 (night-round review): gmMapUI::Update @0x004a2078's gate is + // `if (m_pCoordinateText != 0 && m_pPlayerLocationIcon != 0)` — BOTH + // widgets present, not "at least one". A player-icon template + // resolution failure (leaving _playerIcon null, e.g. a future DAT + // regression) must skip the coordinate-text write too, not just the + // marker placement — the OLD `_coordinateText is null && + // _playerIcon is null` gate only skipped when BOTH were absent, so + // it would have written coordinate text here even with a missing + // player icon. + const uint cellId = 0x11CE0001u; // outdoor: TryFromCell succeeds. + Assert.True(RadarCoordinates.TryFromCell(cellId, out _)); + + ElementInfo rootInfo = FixtureLoader.LoadMapHouseHostInfos(); + ImportedLayout layout = FixtureLoader.LoadMapHouseHost(); + var callbacks = new MapHousePanelController.Callbacks( + Toggle: () => { }, + Map: new MapPageController.Bindings( + CurrentCalendar: static () => default, + PlayerCellId: () => cellId, + HousePosition: static () => (CreateObject.ServerPosition?)null, + // Simulates the player icon's own standalone template + // resolution failing (ResolveSwallowedIcon's own null path) + // while every other swallowed-icon/town-marker resolution + // still succeeds normally. + TemplateResolver: (_, e) => e == MapPageController.PlayerIconId + ? null + : new UiText { Width = 10f, Height = 10f, DatElementId = e }), + House: new HousePageController.Bindings(Lines: static () => Array.Empty())); + + MapHousePanelController? controller = MapHousePanelController.Bind(rootInfo, layout, callbacks); + Assert.NotNull(controller); + + Assert.Null( + UiElement.FindDescendant(controller!.Root, MapPageController.PlayerIconId)); + + var coordinateText = Assert.IsType( + UiElement.FindDescendant(controller.Root, MapPageController.CoordinateTextId)); + Assert.Empty(coordinateText.LinesProvider()); + } + [Fact] public void Bind_HouseMarker_NullPosition_StaysHidden() {