fix(chargen): CC1 review fix round — Custom is template 0, SkillTable cost fallback, frozen model
Implements all six Opus review findings against 04450041 (Campaign CC
CC1 chargen data layer):
- F1 (HIGH, blocking): ChargenTemplate's doc claimed "Custom" has no
ChargenTemplate entry and cited two nonexistent addresses. Verified
against the named retail decomp: gmCGProfessionPage::UpdateProfession
@ 0x004821b0 resolves BOTH the highlighted button and the description
string from CharGenState.template_ 0..6, and case 0 is button
0x100003d9 / ID_CharGen_CustomText. Custom IS template index 0 (the
"Adventurer" row CC1 already found sitting at the attribute floor).
CharGenState::SetTemplate @ 0x005C5A60 confirms every button (including
Custom) calls CharGenState::ApplyTemplate @ 0x005C5080 when committing,
so selecting Custom resets the sliders/skills to that row rather than
leaving them untouched.
- F2 (MEDIUM): retail's skill-cost lookup is two-tiered
(ACCharGenData::GetSkillTrainedCost/GetSkillSpecializedCost @
0x005C26D0/0x005C27D0 fall through to the global SkillTable,
portal.dat 0x0E000004, on a heritage-list miss — confirmed against
ACE's identical PlayerFactory.cs precedence). ChargenTableReader now
also projects the global SkillTable into
ChargenOptions.GlobalSkillCostsBySkillId, and
ChargenSkillCreditMath.ComputeSpent/RemainingCredits check the
heritage list first and the global list on a miss. Added an
installed-DAT completeness assertion recording reality: the global
table prices 38/54 advancement skill ids, every one of the 13
installed heritages ships exactly one heritage-specific override
(always also priced globally), and 16 ids are genuinely uncostable in
both tiers. Also filed a CC7 risk-item note: ACE's own heritage-
override branch over-deducts on Specialize (PlayerFactory.cs:184-211)
— a retail-legal build may be rejected by local ACE at the CC7
connected gate; that is an ACE bug, not an acdream defect.
- F3 (MEDIUM): every collection ChargenTableReader hands into the
record model is now frozen at projection (ToFrozenDictionary/ToArray,
matching MagicCatalog's house pattern), including both
ChargenOptions.Empty dictionaries.
- F4 (LOW): added a reflection guard test
(ChargenNoChoriziteLeakTests) that walks every public
AcDream.Core.CharGen member (property/indexer/constructor/method
types, recursively through generic arguments) and fails if any
resolves to the DatReaderWriter or a Chorizite* assembly.
- F5 (LOW): ChargenGenderOptions.HasAnyAppearanceOptions's doc now
states precisely what the installed-DAT gate proves (an OR across
eight lists, for at least one gender per heritage) rather than the
stronger claim it previously made, and explicitly calls out the three
omitted color lists. Added a second installed-DAT gate that records
per-list reality across every gender of every heritage — found
complete, no empty lists anywhere in the installed DAT today.
- F6 (LOW): ChargenOptions.TryGetHeritage/TryGetStarterArea now use
[MaybeNullWhen(false)] instead of null! suppression, matching the
house pattern already used elsewhere in the test suite. Fixed every
call site this surfaced (more than the five originally estimated,
since Content.Tests has TreatWarningsAsErrors).
Core.Tests: 4737 passed / 1 skip (pre-existing, unrelated).
Content.Tests: 145 passed / 0 skip.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
70d52b0da2
commit
cb4703e8d5
11 changed files with 637 additions and 139 deletions
|
|
@ -187,6 +187,34 @@ worktrees). CC4 ∥ CC6a after CC3. CC5 last before CC7.
|
|||
arbiter.
|
||||
7. `references/*` absent in worktrees (except WorldBuilder, uninitialized
|
||||
submodule) — agents read ACE/holtburger from the MAIN checkout path.
|
||||
8. **CC7 landmine (found in the CC1 review fix round, 2026-08-15):** ACE's
|
||||
`PlayerFactory.CreatePlayer` heritage-override branch
|
||||
(references/ACE/Source/ACE.Server/Factories/PlayerFactory.cs:184-211)
|
||||
over-deducts skill credits when specializing a skill the active
|
||||
heritage's own list prices. For a skill priced ONLY by the global
|
||||
SkillTable, ACE correctly computes the incremental specialize cost via
|
||||
`SkillBase.UpgradeCostFromTrainedToSpecialized` (= `SpecializedCost -
|
||||
TrainedCost`) and charges `TrainSkill(trainedCost) +
|
||||
SpecializeSkill(incrementalCost)` = the field's TOTAL, matching retail.
|
||||
But when the heritage's own list has an entry, ACE sets
|
||||
`specializedCost = skillGroup.PrimaryCost` directly — `PrimaryCost` is
|
||||
already the TOTAL cost to reach Specialized (acdream's own
|
||||
`ChargenSkillCost.PrimaryCost` convention, confirmed against retail) —
|
||||
and then still charges `TrainSkill(NormalCost) +
|
||||
SpecializeSkill(PrimaryCost)`, over-deducting by an extra `NormalCost`
|
||||
credits versus what retail's client computed and what the player agreed
|
||||
to spend. Practical impact for CC7's connected gate: a retail-legal
|
||||
character build that specializes a skill the ACTIVE HERITAGE prices
|
||||
(every one of the 13 installed heritages has exactly one such skill —
|
||||
see `ChargenTableReaderInstalledDatTests.InstalledHeritages_SkillCostFallbackCoversTheKnownUncostableSkillSet`)
|
||||
may be REJECTED by local ACE with `FailedToSpecializeSkill` even though
|
||||
acdream sent the byte-correct 0xF656 body. If CC7's gate hits this,
|
||||
it is an ACE-side bug reproduced from its own source, NOT an acdream
|
||||
wire or math defect — do not "fix" acdream's cost math to match ACE's
|
||||
over-deduction. Register: file an AD row if CC7 needs a documented
|
||||
workaround (e.g. picking a Specialized skill combination that avoids
|
||||
the heritage-priced skill for the connected gate) rather than silently
|
||||
adjusting acdream's send.
|
||||
|
||||
## Review protocol
|
||||
|
||||
|
|
@ -200,7 +228,7 @@ the user gate.
|
|||
|
||||
| Slice | Status | Commits | Review | Notes |
|
||||
|---|---|---|---|---|
|
||||
| CC1 | implemented; review in flight | `04450041` | 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 |
|
||||
| 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. |
|
||||
| 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 |
|
||||
| CC3 | — | | | |
|
||||
| CC4 | — | | | |
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue