fix(chargen): Campaign CC CC6a review fix round — F1-F12

Addresses the CC6a dual-lens review (architectural PASS with reservations,
retail fidelity PASS with reservations, merge after F1/F2/F3).

F1 (BLOCKING) - AlternateSetup/setupId tested the wrong sentinel (0)
instead of retail's INVALID_DID (0xFFFFFFFF, CharGenState::GetSetupID
@0x005C5B22). A hair style storing that value would have been adopted as
a literal Setup id, nulling Get<Setup> and killing the whole preview.
Fixed both sites with a new InvalidDid constant; added two hand-built
tests plus an installed-DAT sweep of every hair style across all 26
heritage/gender combinations (869 selections, zero unresolved Setup ids).

F2 (BLOCKING) - TS-82's register row, ChargenClothingTable.cs's doc, and
the plan's ledger row all understated Undead's measured clothing-coverage
gap as "headgear/trousers/footwear" (3 slots) with a self-contradicting
"4 of 4 non-shirt slots" aside. Corrected everywhere to the true measured
ALL FOUR slots (headgear, trousers, shirt, footwear).

F3 (BLOCKING) - the palette-math "three independent sources" claim
overcounted: ACViewer's ClothingTableList.xaml.cs:97 computes a different
expression for a different problem, and its vendored PaletteSet.cs is
ACE's own file, not an independent implementation. Rewrote the evidence
paragraph in ChargenPalSetMath.cs to the two sources that actually hold
(decomp control flow + ACE's "Taken from acclient.c" port).

F4 (MEDIUM) - ChargenPreviewEntityBuilder.TryBuild did unlocked dat reads;
DatCollection is not thread-safe and every sibling dat-touching resolver
in this layer takes a shared datLock. Added a required datLock parameter;
every dat read now happens inside one lock, mirroring
RetailPaperdollPoseApplicator.Apply's shape.

F5 (LOW) - noted the pre-existing Streaming.LandblockBuildFactoryTests
timing flake in the ledger so a future session doesn't chase it.

F6 (LOW) - fixed ChargenPreviewCamera.cs's rotation doc, which cited a
nonexistent identifier in a dimensionally-wrong expression; corrected to
retail's actual DoRotation @0x0047CAC7 per-tick formula.

F7 (LOW-MEDIUM) - the TS-82 measurement was WriteLine-only; pinned with
real assertions (zero gaps for the 9 standard heritages, exactly the 4
measured Undead table ids on both genders). Kept the existing env-gated
skip pattern (confirmed house convention).

F8 (LOW) - the inner PalSet-miss loop recorded-and-continued past a miss;
retail's own loop returns immediately on a miss (~0x005A7B32), aborting
every remaining choice in that garment. Changed continue to break; added
a test proving a subsequent present PalSet is correctly not applied.

F9 (LOW) - fixed three dangling <see cref="...Compose"/> doc references
(the method is TryCompose).

F10 (LOW) - the packed (byte)(range/8) narrowing was unchecked; a real
NumColors of 2048 happened to wrap to the correct "whole palette" 0
sentinel by unchecked-cast accident. Replaced with explicit PackOffset/
PackNumColors helpers that document the 2048->0 equivalence deliberately
and throw on any other unrepresentable shape.

F11/F12 (LOW, CC6b scope) - noted in the plan's CC6b row: the second
m_alternateSetupID override source is unmodelled, and a shared
RetailHeldPose helper is worth extracting before a fourth consumer.

Test counts: Core.Tests 4772/1 skip (+5), Content.Tests 147/0 (+1),
App.Tests 5121/6 skips (unchanged; F5's named flake did not reproduce) -
zero failures, full solution Release build green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-15 17:51:14 +02:00
parent 55bfd9ca82
commit 1774d8b298
11 changed files with 561 additions and 120 deletions

View file

@ -53,7 +53,7 @@ public sealed class ChargenPreviewEntityBuilderTests
var animations = new RetailAnimationLoader(adapter);
var entity = ChargenPreviewEntityBuilder.TryBuild(
adapter, animations, appearance, heritageId: 1u, Quaternion.Identity);
adapter, animations, appearance, heritageId: 1u, Quaternion.Identity, new object());
Assert.NotNull(entity);
Assert.NotEmpty(entity!.MeshRefs);
@ -85,7 +85,7 @@ public sealed class ChargenPreviewEntityBuilderTests
ClothingTablesMissingBaseEffectForSetup: []);
var entity = ChargenPreviewEntityBuilder.TryBuild(
adapter, animations, bogusAppearance, heritageId: 1u, Quaternion.Identity);
adapter, animations, bogusAppearance, heritageId: 1u, Quaternion.Identity, new object());
Assert.Null(entity);
}
@ -115,7 +115,7 @@ public sealed class ChargenPreviewEntityBuilderTests
Assert.True(composed);
var entity = ChargenPreviewEntityBuilder.TryBuild(
adapter, animations, appearance, heritageId: 12u, Quaternion.Identity);
adapter, animations, appearance, heritageId: 12u, Quaternion.Identity, new object());
// Just proves the Olthoi branch doesn't throw / silently fall through to
// "no mesh" — the exact pose DID differs internally (0x10000011 vs

View file

@ -13,19 +13,52 @@ namespace AcDream.Content.Tests.CharGen;
/// everywhere, mid shade" selection and asserts it resolves with no missing
/// PalSet or ClothingTable dat ids — the CC6a task's explicit acceptance
/// bar ("every heritage/gender's default selection resolves to a complete
/// description with no missing dat ids"). Also records (without asserting
/// zero — see the class doc on <see cref="ChargenClothingTable"/>'s
/// deliberate scope cut) how many clothing slots have no
/// <c>ClothingBaseEffects</c> entry for their own gender's body Setup, so a
/// future session can see at a glance whether CC6a's decision to skip
/// retail's Setup-substitution fallback chain ever actually costs
/// coverage on the real dat.
/// description with no missing dat ids"). ALSO pins the TS-82 measurement
/// with real assertions (not WriteLine-only diagnostics, per the CC6a
/// review fix round F7): the nine standard heritages with clothing UI shown
/// resolve zero <c>ClothingBaseEffects</c> gaps, and Undead resolves
/// EXACTLY the four measured gaps on both genders — see the class doc on
/// <see cref="ChargenClothingTable"/>'s deliberate scope cut.
///
/// <para>Env-gated skip (house pattern, matched from
/// <c>ChargenTableReaderInstalledDatTests</c>/<c>ContentConformanceDats</c>):
/// returns green with a console SKIP note when no installed dat directory is
/// configured, rather than a true xUnit Skipped status — no other Content
/// installed-DAT test in this project uses <c>Assert.Skip</c>, so this stays
/// consistent with the rest of the suite rather than introducing a new
/// convention.</para>
/// </summary>
public sealed class ChargenAppearanceCatalogInstalledDatTests
{
private readonly ITestOutputHelper _out;
public ChargenAppearanceCatalogInstalledDatTests(ITestOutputHelper output) => _out = output;
// ACE ACE.Entity.Enum.HeritageGroup ids. Gearknight (6)/Olthoi (12)/
// OlthoiAcid (13) are deliberately not named here — see the WriteLine-only
// comment in the loop below for why they carry no pinned expectation.
private const uint TumerokId = 7u;
private const uint UndeadId = 11u;
/// <summary>
/// The 9 standard heritages whose UI actually shows clothing controls
/// AND whose default gear resolves with zero <c>ClothingBaseEffects</c>
/// gaps (measured, not the full "clothing UI shown" set — Undead is
/// ALSO clothing-UI-shown but is the one real gap, asserted separately
/// below). Aluvian/Gharu'ndim/Sho/Viamontian/Shadowbound/Tumerok/Lugian/
/// Empyrean/Penumbraen = every heritage id 1-10 except Gearknight (6).
/// </summary>
private static readonly uint[] StandardZeroGapHeritageIds = [1u, 2u, 3u, 4u, 5u, TumerokId, 8u, 9u, 10u];
/// <summary>
/// Measured (installed EoR dat, both genders, identical order): Undead's
/// default headgear/trousers/shirt/footwear choices' clothing tables, in
/// the factory's own Headgear→Trousers→Shirt→Footwear composition order.
/// ALL FOUR slots miss — not "headgear/trousers/footwear" (a three-slot
/// undercount an earlier draft of this row stated in error).
/// </summary>
private static readonly uint[] UndeadMeasuredMissingClothingTableIds =
[0x10000009u, 0x100000F9u, 0x10000001u, 0x10000007u];
private static string? ResolveDatDir()
{
string? fromEnv = Environment.GetEnvironmentVariable("ACDREAM_DAT_DIR");
@ -55,8 +88,8 @@ public sealed class ChargenAppearanceCatalogInstalledDatTests
var catalog = new ChargenAppearanceCatalog(adapter);
int composed = 0;
int absentBaseEffectTotal = 0;
var missingSummaries = new List<string>();
var baseEffectGapFailures = new List<string>();
foreach (ChargenHeritageOptions heritage in options.HeritagesById.Values)
{
@ -79,23 +112,134 @@ public sealed class ChargenAppearanceCatalogInstalledDatTests
+ $"missingClothingTables=[{string.Join(",", result.MissingClothingTableIds.Select(id => $"0x{id:X8}"))}]");
}
absentBaseEffectTotal += result.ClothingTablesMissingBaseEffectForSetup.Count;
if (result.ClothingTablesMissingBaseEffectForSetup.Count > 0)
_out.WriteLine(
$"heritage={heritage.Name} (0x{heritage.HeritageId:X}) gender={genderKey} setup=0x{result.SetupId:X8}: "
+ $"{result.ClothingTablesMissingBaseEffectForSetup.Count} clothing table(s) with no "
+ "ClothingBaseEffects entry for this body setup "
+ $"[{string.Join(",", result.ClothingTablesMissingBaseEffectForSetup.Select(id => $"0x{id:X8}"))}]");
// TS-82's pinned measurement — real assertions, not WriteLine-only.
if (StandardZeroGapHeritageIds.Contains(heritage.HeritageId))
{
_out.WriteLine(
$"heritage={heritage.Name} gender={genderKey} setup=0x{result.SetupId:X8}: "
+ $"{result.ClothingTablesMissingBaseEffectForSetup.Count} clothing table(s) with no "
+ "ClothingBaseEffects entry for this body setup "
+ $"[{string.Join(",", result.ClothingTablesMissingBaseEffectForSetup.Select(id => $"0x{id:X8}"))}]");
if (result.ClothingTablesMissingBaseEffectForSetup.Count != 0)
{
baseEffectGapFailures.Add(
$"heritage={heritage.Name} gender={genderKey}: expected ZERO ClothingBaseEffects "
+ $"gaps (a standard heritage with clothing UI shown), measured "
+ $"{result.ClothingTablesMissingBaseEffectForSetup.Count}: "
+ $"[{string.Join(",", result.ClothingTablesMissingBaseEffectForSetup.Select(id => $"0x{id:X8}"))}]");
}
}
else if (heritage.HeritageId == UndeadId)
{
if (!result.ClothingTablesMissingBaseEffectForSetup.SequenceEqual(UndeadMeasuredMissingClothingTableIds))
{
baseEffectGapFailures.Add(
$"heritage=Undead gender={genderKey}: expected EXACTLY "
+ $"[{string.Join(",", UndeadMeasuredMissingClothingTableIds.Select(id => $"0x{id:X8}"))}], measured "
+ $"[{string.Join(",", result.ClothingTablesMissingBaseEffectForSetup.Select(id => $"0x{id:X8}"))}]");
}
}
// Gearknight/Olthoi/OlthoiAcid: retail hides the clothing UI
// entirely for these three (gmCGAppearancePage::Update
// @0x0047E8F0's SetVisible(0) branches), so a real chargen
// selection never reaches this composer's clothing slots for
// them — no pinned expectation either way, WriteLine above
// is diagnostic only.
}
}
_out.WriteLine($"composed {composed} heritage/gender selections.");
Assert.True(
missingSummaries.Count == 0,
"Missing dat ids found:\n" + string.Join('\n', missingSummaries));
Assert.True(
baseEffectGapFailures.Count == 0,
"TS-82 measurement drifted from its pinned expectation:\n" + string.Join('\n', baseEffectGapFailures));
Assert.True(composed >= 13, $"Expected at least 13 heritage/gender combinations, composed {composed}.");
}
/// <summary>
/// CC6a review fix round F1: retail's Setup-id "unset" sentinel is
/// <c>INVALID_DID</c> (0xFFFFFFFF), not 0
/// (<c>CharGenState::GetSetupID @ 0x005C5B22</c>). Sweeps EVERY hair
/// style of all 26 heritage/gender combinations and asserts the composed
/// SetupId always resolves to a REAL installed Setup dat entry — proving
/// neither sentinel value, wherever a hair style's <c>AlternateSetup</c>
/// field happens to store one, ever reaches <c>Get&lt;Setup&gt;</c> as a
/// literal id.
/// </summary>
[Fact]
public void EveryHairStyleOfEveryHeritageGender_ComposesToARealInstalledSetupId()
{
string? datDir = ResolveDatDir();
if (datDir is null)
{
_out.WriteLine("SKIP: installed retail DAT directory is unavailable.");
return;
}
using var dats = new DatCollection(datDir, DatAccessType.Read);
using var adapter = new DatCollectionAdapter(dats);
ChargenOptions options = ChargenTableReader.Load(adapter);
Assert.NotEmpty(options.HeritagesById);
var catalog = new ChargenAppearanceCatalog(adapter);
int sweptHairStyles = 0;
var unresolvedSetups = new List<string>();
foreach (ChargenHeritageOptions heritage in options.HeritagesById.Values)
{
foreach ((int genderKey, ChargenGenderOptions gender) in heritage.GendersByKey)
{
for (uint hairStyleIndex = 0; hairStyleIndex < (uint)gender.HairStyles.Count; hairStyleIndex++)
{
ChargenAppearanceSelection selection = ChargenAppearanceSelection.Default with
{
HairStyle = hairStyleIndex,
SkinShade = 0.5,
};
bool ok = ChargenAppearanceFactory.TryCompose(
options, heritage.HeritageId, genderKey, selection,
catalog, catalog, out ChargenAppearanceResult result);
Assert.True(ok);
sweptHairStyles++;
if (adapter.Get<DatReaderWriter.DBObjs.Setup>(result.SetupId) is null)
{
unresolvedSetups.Add(
$"heritage={heritage.Name} gender={genderKey} hairStyle={hairStyleIndex}: "
+ $"composed SetupId=0x{result.SetupId:X8} does not resolve to an installed Setup");
}
}
// Every gender is swept even with zero hair styles (still
// exercises the "no hair style selected" default-setup path).
if (gender.HairStyles.Count == 0)
{
bool ok = ChargenAppearanceFactory.TryCompose(
options, heritage.HeritageId, genderKey,
ChargenAppearanceSelection.Default with { SkinShade = 0.5 },
catalog, catalog, out ChargenAppearanceResult result);
Assert.True(ok);
sweptHairStyles++;
if (adapter.Get<DatReaderWriter.DBObjs.Setup>(result.SetupId) is null)
{
unresolvedSetups.Add(
$"heritage={heritage.Name} gender={genderKey} (no hair styles): "
+ $"composed SetupId=0x{result.SetupId:X8} does not resolve to an installed Setup");
}
}
}
}
_out.WriteLine($"composed {composed} heritage/gender selections; {absentBaseEffectTotal} absent-base-effect slots total.");
_out.WriteLine($"swept {sweptHairStyles} hair-style/no-hair-style selections across 26 heritage/gender combinations.");
Assert.True(
missingSummaries.Count == 0,
"Missing dat ids found:\n" + string.Join('\n', missingSummaries));
Assert.True(composed >= 13, $"Expected at least 13 heritage/gender combinations, composed {composed}.");
unresolvedSetups.Count == 0,
"Composed SetupId(s) that don't resolve to a real installed Setup:\n" + string.Join('\n', unresolvedSetups));
Assert.True(sweptHairStyles > 26, $"Expected more than 26 swept selections (multiple hair styles per gender), got {sweptHairStyles}.");
}
/// <summary>

View file

@ -259,6 +259,49 @@ public sealed class ChargenAppearanceFactoryTests
Assert.Equal(ChargenAppearanceFactory.HumanSetupId, result.SetupId);
}
/// <summary>
/// CC6a review fix round F1: retail's "unset" sentinel for a Setup id is
/// <c>INVALID_DID</c> (0xFFFFFFFF — <c>CharGenState::GetSetupID @
/// 0x005C5B22</c>), not 0. A hair style whose <c>AlternateSetup</c> field
/// stores 0xFFFFFFFF must NOT be adopted as the body Setup id — before
/// this fix the factory would hand 0xFFFFFFFF straight to a caller's
/// <c>Get&lt;Setup&gt;</c>, which nulls, and the whole preview build
/// would fail silently.
/// </summary>
[Fact]
public void TryCompose_HairStyleAlternateSetupIsInvalidDid_IsTreatedAsUnsetNotAdopted()
{
ChargenOptions options = MakeOptions(MakeGender(alternateHairSetup: 0xFFFFFFFFu));
var (pal, clothing) = MakeSources();
var selection = ChargenAppearanceSelection.Default with { HairStyle = 0u };
ChargenAppearanceFactory.TryCompose(
options, HeritageId, GenderKey, selection, pal, clothing, out ChargenAppearanceResult result);
Assert.Equal(BodySetupId, result.SetupId); // gender.SetupId, NOT the INVALID_DID sentinel.
}
/// <summary>
/// Companion to <see cref="TryCompose_BothSetupSourcesZero_FallsBackToHumanSetupId"/>:
/// the resolved Setup id can ALSO be stuck at INVALID_DID (rather than 0)
/// when the gender's own <c>SetupId</c> dat field happens to be
/// 0xFFFFFFFF — the fallback to <see cref="ChargenAppearanceFactory.HumanSetupId"/>
/// must catch that case too.
/// </summary>
[Fact]
public void TryCompose_GenderSetupIdIsInvalidDid_FallsBackToHumanSetupId()
{
ChargenGenderOptions gender = MakeGender() with { SetupId = 0xFFFFFFFFu };
ChargenOptions options = MakeOptions(gender);
var (pal, clothing) = MakeSources(bodySetupId: 0xFFFFFFFFu);
ChargenAppearanceFactory.TryCompose(
options, HeritageId, GenderKey, ChargenAppearanceSelection.Default,
pal, clothing, out ChargenAppearanceResult result);
Assert.Equal(ChargenAppearanceFactory.HumanSetupId, result.SetupId);
}
[Fact]
public void TryCompose_EyeStripSelected_UsesNonBaldObjDesc_WhenHairStyleIsNotBald()
{
@ -368,6 +411,133 @@ public sealed class ChargenAppearanceFactoryTests
Assert.DoesNotContain(result.ObjDesc.SubPalettes, sp => sp.Offset == 10 && sp.NumColors == 2);
}
/// <summary>
/// CC6a review fix round F8: retail's inner subpalette loop
/// (<c>ClothingTable::BuildObjDesc</c> ~0x005A7B24-0x005A7BD3) returns 0
/// IMMEDIATELY when a PalSet read fails for one choice (~0x005A7B32),
/// aborting every REMAINING choice in that garment's palette template —
/// not merely skipping the failed one and continuing. A two-choice
/// template with the FIRST choice's PalSet missing must therefore emit
/// NEITHER choice's subpalette, even though the second choice's own
/// PalSet is present and would resolve fine on its own.
/// </summary>
[Fact]
public void TryCompose_PalSetMissingMidLoop_AbortsRemainingChoicesInThatGarment()
{
const uint missingPalSetId = 0x0F00_00AAu;
const uint presentPalSetId = 0x0F00_00BBu;
var firstChoice = new ChargenClothingSubPaletteChoice(
missingPalSetId, [new ChargenClothingSubPaletteRange(80u, 16u)]);
var secondChoice = new ChargenClothingSubPaletteChoice(
presentPalSetId, [new ChargenClothingSubPaletteRange(160u, 8u)]);
var baseEffects = new Dictionary<uint, ChargenClothingBaseEffect>
{
[BodySetupId] = ChargenClothingBaseEffect.Empty,
};
var templates = new Dictionary<uint, ChargenClothingPaletteTemplate>
{
[7u] = new ChargenClothingPaletteTemplate([firstChoice, secondChoice]),
};
var table = new ChargenClothingTable(baseEffects, templates);
ChargenOptions options = MakeOptions(MakeGender());
var (pal, clothing) = MakeSources();
clothing.Add(HeadgearClothingTableId, table); // override the shared fixture's single-choice table.
pal.Add(presentPalSetId, 0x0400_0055u); // deliberately NOT adding missingPalSetId.
var selection = ChargenAppearanceSelection.Default with
{
HeadgearStyle = 0u,
HeadgearColor = 0u,
HeadgearShade = 0.0,
};
bool ok = ChargenAppearanceFactory.TryCompose(
options, HeritageId, GenderKey, selection, pal, clothing, out ChargenAppearanceResult result);
Assert.True(ok);
Assert.Contains(missingPalSetId, result.MissingPalSetIds);
// Real range (160, 8) would pack to (20, 1) if the second choice were
// (incorrectly) still applied after the first choice's miss.
Assert.DoesNotContain(result.ObjDesc.SubPalettes, sp => sp.Offset == 20 && sp.NumColors == 1);
// Nothing from EITHER choice's own range landed.
Assert.DoesNotContain(result.ObjDesc.SubPalettes, sp => sp.Offset == 10 && sp.NumColors == 2);
}
/// <summary>
/// CC6a review fix round F10: a real dat <c>NumColors</c> of exactly
/// 2048 (256*8) is retail's own "whole palette" value spelled out in
/// real units — it packs to the byte 0 sentinel
/// (<see cref="AcDream.Core.World.PaletteOverride"/>'s documented
/// "Length=0 means entire palette") EXPLICITLY, not via an unchecked
/// narrowing coincidence.
/// </summary>
[Fact]
public void TryCompose_ClothingRangeNumColorsIsWholePaletteSentinel_PacksToZeroExplicitly()
{
var choice = new ChargenClothingSubPaletteChoice(
0x0F00_0003u, [new ChargenClothingSubPaletteRange(0u, 2048u)]);
var baseEffects = new Dictionary<uint, ChargenClothingBaseEffect>
{
[BodySetupId] = ChargenClothingBaseEffect.Empty,
};
var table = new ChargenClothingTable(
baseEffects,
new Dictionary<uint, ChargenClothingPaletteTemplate> { [7u] = new([choice]) });
ChargenOptions options = MakeOptions(MakeGender());
var (pal, clothing) = MakeSources();
clothing.Add(HeadgearClothingTableId, table);
var selection = ChargenAppearanceSelection.Default with
{
HeadgearStyle = 0u,
HeadgearColor = 0u,
HeadgearShade = 0.0,
};
ChargenAppearanceFactory.TryCompose(
options, HeritageId, GenderKey, selection, pal, clothing, out ChargenAppearanceResult result);
Assert.Contains(result.ObjDesc.SubPalettes, sp => sp.Offset == 0 && sp.NumColors == 0);
}
/// <summary>
/// CC6a review fix round F10: a shape the packed *8 byte convention
/// cannot represent losslessly (not a multiple of 8, and not the 2048
/// whole-palette sentinel) must THROW rather than silently truncate via
/// an unchecked <c>(byte)</c> cast.
/// </summary>
[Fact]
public void TryCompose_ClothingRangeDoesNotFitThePackedByteConvention_Throws()
{
var choice = new ChargenClothingSubPaletteChoice(
0x0F00_0003u, [new ChargenClothingSubPaletteRange(0u, 2041u)]); // not a multiple of 8, not 2048.
var baseEffects = new Dictionary<uint, ChargenClothingBaseEffect>
{
[BodySetupId] = ChargenClothingBaseEffect.Empty,
};
var table = new ChargenClothingTable(
baseEffects,
new Dictionary<uint, ChargenClothingPaletteTemplate> { [7u] = new([choice]) });
ChargenOptions options = MakeOptions(MakeGender());
var (pal, clothing) = MakeSources();
clothing.Add(HeadgearClothingTableId, table);
var selection = ChargenAppearanceSelection.Default with
{
HeadgearStyle = 0u,
HeadgearColor = 0u,
HeadgearShade = 0.0,
};
Assert.Throws<ArgumentOutOfRangeException>(() =>
ChargenAppearanceFactory.TryCompose(
options, HeritageId, GenderKey, selection, pal, clothing, out _));
}
[Fact]
public void TryCompose_UnknownClothingTableId_IsRecordedAsMissingAndSkipped()
{