fix(ui): night-round review — F11/F13/F14/F15 one-liners
F11: logs when a Map tab town-marker's template resolves to something other than a UiButton — that path previously silently skipped the TooltipText write with no diagnostic, leaving a mounted-but-empty tooltip popup indistinguishable from "no template configured". F13: RetailSkillFormula.FormatFormula now reads Attribute1Multiplier/ Attribute2Multiplier/AdditiveBonus/Divisor through the SAME unsigned reinterpretation TryCalculate already uses (this class's own doc comment already stated the invariant; FormatFormula just didn't follow it). A high-bit-set value would previously both mis-gate hasAttr1/ hasAttr2 and print a negative number, out of sync with what TryCalculate actually computes with for the same formula. Added regression tests, empirically verified to fail without the fix. F14: documented the RefreshHouseMarker gap rather than guessing at the byte-decode — Position::get_outside_cell_id @0x004527b0 is itself BN-mangled (its `(eax_2 - eax_2) & objcell_id` return is the same decompiler-obscures-a-real-conditional artifact class this round hit elsewhere) and depends on LandDefs::adjust_to_outside, a genuinely larger port than this round's other findings. HousePosition is wired () => null in production today (ISSUES #413's remaining scope), so this method is currently unreachable; left a TODO citing the retail call chain for whenever that lands. F15: fixed RefreshCoordinatesAndPlayerMarker's gate to AND-on-both- present, matching gmMapUI::Update @0x004a2078's exact `if (m_pCoordinateText != 0 && m_pPlayerLocationIcon != 0)` condition. The prior `_coordinateText is null && _playerIcon is null` check only skipped when BOTH were absent (proceeding whenever EITHER was present), letting coordinate text and the player marker update independently instead of as the single gated unit retail treats them as. Added a regression test (player-icon template resolution failure must also skip the coordinate-text write), empirically verified to fail without the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
c403f57815
commit
0b0c7aa485
4 changed files with 172 additions and 18 deletions
|
|
@ -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();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <c>gmMapUI::Update @0x004a2078</c>'s gate is
|
||||
/// <c>if (m_pCoordinateText != 0 && m_pPlayerLocationIcon != 0)</c>
|
||||
/// — BOTH widgets present, not "at least one". Night-round review F15:
|
||||
/// the prior <c>_coordinateText is null && _playerIcon is null</c>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <c>gmMapUI::Update @0x004a22a6-f6</c>: <c>Position::get_outside_cell_id
|
||||
/// (&m_HousePosition) -> LandDefs::gid_to_lcoord</c> -> the SAME
|
||||
/// <c>(v-0x400)*0.1+0.5</c> transform <see cref="PlaceMarker"/>'s player
|
||||
/// branch uses.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Night-round review F14: this passes <c>housePosition.Value.LandblockId</c>
|
||||
/// straight to <see cref="RadarCoordinates.TryFromCell"/>, SKIPPING the
|
||||
/// <c>Position::get_outside_cell_id @0x004527b0</c> step retail's own
|
||||
/// call chain names. That function is itself BN-mangled (its final
|
||||
/// <c>return ((eax_2 - eax_2) & objcell_id)</c> — 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 <c>LandDefs::adjust_to_outside</c>,
|
||||
/// 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): <see cref="Bindings.HousePosition"/> is wired
|
||||
/// <c>() => null</c> 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 <c>Position::get_outside_cell_id</c> /
|
||||
/// <c>LandDefs::adjust_to_outside</c> (byte-decode required,
|
||||
/// <c>@0x004527b0</c> / call site <c>@0x004a2297</c>) 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 <c>adjust_to_outside</c> 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 <see cref="RadarCoordinates.TryFromCell"/> correctly refuses
|
||||
/// any cell with an envcell low word).
|
||||
/// </remarks>
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -74,6 +74,46 @@ public sealed class RetailSkillFormulaTests
|
|||
Assert.Equal(uint.MaxValue, result);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// F13 (night-round review): <see cref="RetailSkillFormula.FormatFormula"/>
|
||||
/// previously read the raw signed <c>int</c> fields directly instead of
|
||||
/// the SAME unsigned reinterpretation <see cref="RetailSkillFormula.TryCalculate"/>
|
||||
/// uses. A multiplier whose stored bit pattern has the high bit set
|
||||
/// would read as a small negative number here (failing the
|
||||
/// <c>>= 1</c> hasAttr gate, or printing a negative multiplier) instead
|
||||
/// of the huge unsigned value <c>TryCalculate</c> actually computes
|
||||
/// with for the SAME formula.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>Same reinterpretation, but for the divisor and additive-bonus
|
||||
/// suffixes rather than the multiplier — both must read unsigned too.</summary>
|
||||
[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()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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<string>()));
|
||||
|
||||
MapHousePanelController? controller = MapHousePanelController.Bind(rootInfo, layout, callbacks);
|
||||
Assert.NotNull(controller);
|
||||
|
||||
Assert.Null(
|
||||
UiElement.FindDescendant(controller!.Root, MapPageController.PlayerIconId));
|
||||
|
||||
var coordinateText = Assert.IsType<UiText>(
|
||||
UiElement.FindDescendant(controller.Root, MapPageController.CoordinateTextId));
|
||||
Assert.Empty(coordinateText.LinesProvider());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Bind_HouseMarker_NullPosition_StaysHidden()
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue