erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

docs+fix(chargen): CC1/CC2 review closeout — R2/R3 residuals closed, ledger final

Browse source 
Both narrow re-reviews returned CLOSED. This closeout takes the two cheap
re-review residuals before CC3 takes references to the shared model:

R2: every array handed into the typed chargen model is now wrapped in
Array.AsReadOnly at the projection seam — a T[] behind IReadOnlyList<T>
was still downcast-mutable, and ChargenOptions is a process-shared
singleton graph.

R3: the no-Chorizite-leak guard now also walks public fields; every
current type uses properties, but a public field would have slipped
through the property-only walk.

Ledger: CC1 fix-round sha corrected to cb4703e8 (the cell previously
cited the pre-amend 459a87f2), CC1/CC2 rows flipped to REVIEW-CLOSED
with the re-review outcomes, R1 (retail refunds +1 credit on a
both-tier cost miss; port charges 0 — unreachable via retail's own
listbox, noted for CC3) and the Olthoi-locked-to-template-0 decomp fact
recorded for CC3/CC4.

Core.Tests 4736/1 skip, Content.Tests 145/0, Release.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-15 13:41:37 +02:00
parent 55fc51ed8c
commit f3ef7baae2
3 changed files with 37 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

4
docs/plans/2026-08-15-character-creation-campaign.md
View file

@ -228,8 +228,8 @@ the user gate.
| Slice | Status | Commits | Review | Notes | | Slice | Status | Commits | Review | Notes |
|---|---|---|---|---| |---|---|---|---|---|
| CC1 | implemented; review fix round complete | `04450041`, `459a87f2` | dispatched 2026-08-15 | Core model (no Chorizite leak) + Content projector; 31 math units + 6 installed-DAT gates (13 heritages). FINDING for CC3: each human heritage's "Adventurer" template IS retail's Custom entry point — attributes at the 10-floor (60/330), a real TemplateCG row, not a UI special case. **Review fix round (`459a87f2`):** F1 doc corrected — Custom IS template index 0 (the Adventurer row), per `gmCGProfessionPage::UpdateProfession @ 0x004821b0` (case 0 → button 0x100003d9 / `ID_CharGen_CustomText`) and `CharGenState::SetTemplate @ 0x005C5A60` (commits via `CharGenState::ApplyTemplate @ 0x005C5080`, i.e. selecting Custom resets sliders to the floor spread, it does not bypass templates); F2 two-tier skill-cost fallback implemented (`ChargenOptions.GlobalSkillCostsBySkillId` from portal.dat 0x0E000004, `ChargenSkillCreditMath` checks heritage list then global list) + installed-DAT completeness assertion recording reality: the global SkillTable prices 38/54 advancement skill ids, every one of the 13 heritages ships EXACTLY one heritage-specific override (always also present in the global table), and 16 skill ids are genuinely uncostable in both tiers (retail's -1 case) — see `ChargenTableReaderInstalledDatTests.InstalledHeritages_SkillCostFallbackCoversTheKnownUncostableSkillSet`; F3 every `ChargenTableReader` collection is now frozen at projection (`ToFrozenDictionary`/`ToArray`, matching `MagicCatalog`'s pattern) including both `ChargenOptions.Empty` dictionaries; F4 a reflection guard test (`ChargenNoChoriziteLeakTests`) pins the no-Chorizite-leak contract by walking every public `AcDream.Core.CharGen` member; F5 `HasAnyAppearanceOptions`'s doc reworded to state precisely what it proves (an OR across eight lists, omitting the three color lists) + a new installed-DAT gate records per-list reality — found COMPLETE, every gender of every heritage has non-empty lists across all eight plus the three color lists, even the sparse Gear Knight/Olthoi variants; F6 `TryGetHeritage`/`TryGetStarterArea` annotated `[MaybeNullWhen(false)]` (matching the house `EmptyDatReaderWriter` pattern), all affected call sites (more than the originally estimated five) fixed across both test projects. Filed CC7 risk item 8: ACE's `PlayerFactory` heritage-override branch over-deducts skill credits when specializing a heritage-priced skill (references/ACE/Source/ACE.Server/Factories/PlayerFactory.cs:184-211) — a retail-legal build may be rejected by local ACE at the CC7 connected gate; this is an ACE bug, not an acdream defect. | | CC1 | REVIEW-CLOSED 2026-08-15 | `04450041`, `cb4703e8` | CLOSED (fix round + narrow re-review; every citation independently re-derived) | Core model (no Chorizite leak) + Content projector; 31 math units + 6 installed-DAT gates (13 heritages). FINDING for CC3: each human heritage's "Adventurer" template IS retail's Custom entry point — attributes at the 10-floor (60/330), a real TemplateCG row, not a UI special case. **Review fix round (`cb4703e8`):** F1 doc corrected — Custom IS template index 0 (the Adventurer row), per `gmCGProfessionPage::UpdateProfession @ 0x004821b0` (case 0 → button 0x100003d9 / `ID_CharGen_CustomText`) and `CharGenState::SetTemplate @ 0x005C5A60` (commits via `CharGenState::ApplyTemplate @ 0x005C5080`, i.e. selecting Custom resets sliders to the floor spread, it does not bypass templates); F2 two-tier skill-cost fallback implemented (`ChargenOptions.GlobalSkillCostsBySkillId` from portal.dat 0x0E000004, `ChargenSkillCreditMath` checks heritage list then global list) + installed-DAT completeness assertion recording reality: the global SkillTable prices 38/54 advancement skill ids, every one of the 13 heritages ships EXACTLY one heritage-specific override (always also present in the global table), and 16 skill ids are genuinely uncostable in both tiers (retail's -1 case) — see `ChargenTableReaderInstalledDatTests.InstalledHeritages_SkillCostFallbackCoversTheKnownUncostableSkillSet`; F3 every `ChargenTableReader` collection is now frozen at projection (`ToFrozenDictionary`/`ToArray`, matching `MagicCatalog`'s pattern) including both `ChargenOptions.Empty` dictionaries; F4 a reflection guard test (`ChargenNoChoriziteLeakTests`) pins the no-Chorizite-leak contract by walking every public `AcDream.Core.CharGen` member; F5 `HasAnyAppearanceOptions`'s doc reworded to state precisely what it proves (an OR across eight lists, omitting the three color lists) + a new installed-DAT gate records per-list reality — found COMPLETE, every gender of every heritage has non-empty lists across all eight plus the three color lists, even the sparse Gear Knight/Olthoi variants; F6 `TryGetHeritage`/`TryGetStarterArea` annotated `[MaybeNullWhen(false)]` (matching the house `EmptyDatReaderWriter` pattern), all affected call sites (more than the originally estimated five) fixed across both test projects. Filed CC7 risk item 8: ACE's `PlayerFactory` heritage-override branch over-deducts skill credits when specializing a heritage-priced skill (references/ACE/Source/ACE.Server/Factories/PlayerFactory.cs:184-211) — a retail-legal build may be rejected by local ACE at the CC7 connected gate; this is an ACE bug, not an acdream defect. **Narrow re-review CLOSED:** the reviewer retro-graded F2 to HIGH (under the base commit 37 of 38 costable skills were charged zero) and confirmed the SkillBase.SpecializedCost->PrimaryCost mapping dodged the UpgradeCostFromTrainedToSpecialized trap. Residuals: R1 retail refunds +1 credit on a both-tier miss (port charges 0; unreachable via retail’s own skills listbox — NOTE FOR CC3 if any path ever exposes the 16 uncostable ids); R2 list downcast-mutability and R3 field-walking in the leak guard CLOSED at the merge-closeout commit (Array.AsReadOnly at every projection seam; GetFields walk added). Decomp fact for CC4: ApplyTemplate force-sets template_=0 for heritage 0xc/0xd — both Olthoi variants are hard-locked to Custom/template 0. |
| CC2 | implemented (campaign-cc2); review in flight | `5eaad2c8` | dispatched 2026-08-15 | Byte-exact 0xF656 (19-term checksum vs CG_Pack accumulator), shared 0xF643 type, correlation latch, status events + contract amendment. Core.Net 993 / Runtime 1667 / Launcher.Core 323, Windows+WSL | | CC2 | REVIEW-CLOSED, MERGED 2026-08-15 (`55fc51ed`) | `5eaad2c8`, `e77ebf10`, `95e95bb6` | PASS then CLOSED (fix round: F1 latch-scope narrowing + overwrite pin test, F2 register AD-100, F3 ACE double-NameInUse note, F4 creationFailed{code,reason,name}, F5 pointer, retail-discriminator citations) | Byte-exact 0xF656 (19-term checksum vs CG_Pack accumulator), shared 0xF643 type, correlation latch, status events + contract amendment. Core.Net 993 / Runtime 1667 / Launcher.Core 323, Windows+WSL |
| CC3 | — | | | | | CC3 | — | | | |
| CC4 | — | | | | | CC4 | — | | | |
| CC5 | — | | | | | CC5 | — | | | |

40
src/AcDream.Content/CharGen/ChargenTableReader.cs
View file

@ -16,7 +16,7 @@ namespace AcDream.Content.CharGen;
/// presentation-free <see cref="AcDream.Core.CharGen.ChargenOptions"/> tree. /// presentation-free <see cref="AcDream.Core.CharGen.ChargenOptions"/> tree.
/// Mirrors <c>MagicCatalog.Load</c>'s shape: one static entry point over /// Mirrors <c>MagicCatalog.Load</c>'s shape: one static entry point over
/// <see cref="IDatReaderWriter"/>, every returned collection is frozen at /// <see cref="IDatReaderWriter"/>, every returned collection is frozen at
/// projection (<c>ToFrozenDictionary</c> / <c>ToArray</c>, matching /// projection (<c>ToFrozenDictionary</c> / <c>Array.AsReadOnly</c>, matching
/// <c>MagicCatalog</c>'s pattern), and no Chorizite types cross into the /// <c>MagicCatalog</c>'s pattern), and no Chorizite types cross into the
/// returned model. Cross-checked against ACE's /// returned model. Cross-checked against ACE's
/// <c>ACE.DatLoader.FileTypes.CharGen</c> + /// <c>ACE.DatLoader.FileTypes.CharGen</c> +
@ -94,7 +94,7 @@ public static class ChargenTableReader
} }
return new ChargenOptions( return new ChargenOptions(
starterAreas, Array.AsReadOnly(starterAreas),
heritagesById.ToFrozenDictionary(), heritagesById.ToFrozenDictionary(),
globalSkillCosts.ToFrozenDictionary()); globalSkillCosts.ToFrozenDictionary());
} }
@ -110,7 +110,7 @@ public static class ChargenTableReader
position.Frame.Origin, position.Frame.Origin,
position.Frame.Orientation); position.Frame.Orientation);
} }
return new ChargenStarterArea(index, area.Name.Value, locations); return new ChargenStarterArea(index, area.Name.Value, Array.AsReadOnly(locations));
} }
private static ChargenHeritageOptions ProjectHeritage(uint heritageId, HeritageGroupCG cg) private static ChargenHeritageOptions ProjectHeritage(uint heritageId, HeritageGroupCG cg)
@ -138,10 +138,10 @@ public static class ChargenTableReader
cg.EnvironmentSetupId.DataId, cg.EnvironmentSetupId.DataId,
cg.AttributeCredits, cg.AttributeCredits,
cg.SkillCredits, cg.SkillCredits,
cg.PrimaryStartAreas.ToArray(), Array.AsReadOnly(cg.PrimaryStartAreas.ToArray()),
cg.SecondaryStartAreas.ToArray(), Array.AsReadOnly(cg.SecondaryStartAreas.ToArray()),
skillCosts.ToFrozenDictionary(), skillCosts.ToFrozenDictionary(),
templates, Array.AsReadOnly(templates),
gendersByKey.ToFrozenDictionary()); gendersByKey.ToFrozenDictionary());
} }
@ -166,8 +166,8 @@ public static class ChargenTableReader
template.Quickness, template.Quickness,
template.Focus, template.Focus,
template.Self), template.Self),
normalSkills, Array.AsReadOnly(normalSkills),
primarySkills); Array.AsReadOnly(primarySkills));
} }
private static ChargenGenderOptions ProjectGender(int genderKey, SexCG sex) private static ChargenGenderOptions ProjectGender(int genderKey, SexCG sex)
@ -221,20 +221,20 @@ public static class ChargenTableReader
sex.MotionTable.DataId, sex.MotionTable.DataId,
sex.CombatTable.DataId, sex.CombatTable.DataId,
ProjectObjDesc(sex.BaseObjDesc), ProjectObjDesc(sex.BaseObjDesc),
sex.HairColors.ToArray(), Array.AsReadOnly(sex.HairColors.ToArray()),
hairStyles, Array.AsReadOnly(hairStyles),
sex.EyeColors.ToArray(), Array.AsReadOnly(sex.EyeColors.ToArray()),
eyeStrips, Array.AsReadOnly(eyeStrips),
noseStrips, Array.AsReadOnly(noseStrips),
mouthStrips, Array.AsReadOnly(mouthStrips),
ProjectGearList(sex.Headgears), ProjectGearList(sex.Headgears),
ProjectGearList(sex.Shirts), ProjectGearList(sex.Shirts),
ProjectGearList(sex.Pants), ProjectGearList(sex.Pants),
ProjectGearList(sex.Footwear), ProjectGearList(sex.Footwear),
sex.ClothingColors.ToArray()); Array.AsReadOnly(sex.ClothingColors.ToArray()));
} }
private static ChargenGearOption[] ProjectGearList(List<GearCG> gearList) private static IReadOnlyList<ChargenGearOption> ProjectGearList(List<GearCG> gearList)
{ {
var result = new ChargenGearOption[gearList.Count]; var result = new ChargenGearOption[gearList.Count];
for (int i = 0; i < gearList.Count; i++) for (int i = 0; i < gearList.Count; i++)
@ -242,7 +242,7 @@ public static class ChargenTableReader
GearCG gear = gearList[i]; GearCG gear = gearList[i];
result[i] = new ChargenGearOption(gear.Name.Value, gear.ClothingTable.DataId, gear.WeenieDefault); result[i] = new ChargenGearOption(gear.Name.Value, gear.ClothingTable.DataId, gear.WeenieDefault);
} }
return result; return Array.AsReadOnly(result);
} }
private static CoreChargenObjDesc ProjectObjDesc(DatObjDesc objDesc) private static CoreChargenObjDesc ProjectObjDesc(DatObjDesc objDesc)
@ -271,6 +271,10 @@ public static class ChargenTableReader
animPartChanges[i] = new ChargenAnimPartChange(change.PartIndex, change.PartId.DataId); animPartChanges[i] = new ChargenAnimPartChange(change.PartIndex, change.PartId.DataId);
} }
return new CoreChargenObjDesc(objDesc.PaletteId.DataId, subPalettes, textureChanges, animPartChanges); return new CoreChargenObjDesc(
objDesc.PaletteId.DataId,
Array.AsReadOnly(subPalettes),
Array.AsReadOnly(textureChanges),
Array.AsReadOnly(animPartChanges));
} }
} }

17
tests/AcDream.Core.Tests/CharGen/ChargenNoChoriziteLeakTests.cs
View file

@ -11,10 +11,13 @@ namespace AcDream.Core.Tests.CharGen;
/// future edit from putting e.g. a <c>DatReaderWriter.Enums.SkillId</c> /// future edit from putting e.g. a <c>DatReaderWriter.Enums.SkillId</c>
/// directly on a public property. This test walks every public type in the /// directly on a public property. This test walks every public type in the
/// <c>AcDream.Core.CharGen</c> namespace and asserts that no public /// <c>AcDream.Core.CharGen</c> namespace and asserts that no public
/// property, indexer, constructor parameter, or method return/parameter /// property, field, indexer, constructor parameter, or method
/// type — nor any of their generic type arguments, recursively — comes /// return/parameter type — nor any of their generic type arguments,
/// from the <c>DatReaderWriter</c> assembly or any assembly whose name /// recursively — comes from the <c>DatReaderWriter</c> assembly or any
/// starts with <c>Chorizite</c>. /// assembly whose name starts with <c>Chorizite</c>. (The field walk
/// closes the CC1 re-review's R3 residual: every current type uses
/// properties, but <c>public SkillId Foo;</c> would otherwise slip
/// through.)
/// </summary> /// </summary>
public sealed class ChargenNoChoriziteLeakTests public sealed class ChargenNoChoriziteLeakTests
{ {
@ -51,6 +54,12 @@ public sealed class ChargenNoChoriziteLeakTests
} }
} }
foreach (FieldInfo field in type.GetFields(
BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static))
{
CheckSite(field.FieldType, $"{type.FullName}.{field.Name} (field)", offenders);
}
foreach (ConstructorInfo ctor in type.GetConstructors( foreach (ConstructorInfo ctor in type.GetConstructors(
BindingFlags.Public | BindingFlags.Instance)) BindingFlags.Public | BindingFlags.Instance))
{ {