Repository navigation
Variable-size element table, and a fix for Spawn.DefensiveElement - #8
Conversation
Spawn.DefensiveElement counts Defense.Elements, but it reads Damage.Elements. A spawn with <Defense> and no <Damage> throws an exception. A spawn with different lists gets its defensive element from the damage list. OffensiveElement and DefensiveElement now use one helper. The helper reads the list of the property. It returns None when the list is absent or empty.
ElementTable.xsd requires exactly 9 Source rows, each with exactly 9 Target entries. The world and ceridwen tables have 10 rows, because they include Undead. Both tables fail validation for this reason. This change does these things: - Source and Target accept 1 or more entries. - A new DefaultMultiplier attribute (default 1) applies to each pair that the table does not declare. - xs:unique rejects two Source rows with the same element. It also rejects two Target entries with the same element in one Source row. - ElementTable.GetMultiplier(source, target) returns the declared multiplier. It returns DefaultMultiplier if the Source row, the Target entry or the Multiplier attribute is missing. The test project copies the XSD files to its output directory. The tests use these files to validate tables against the schema.
imbas-forosnai
left a comment
There was a problem hiding this comment.
I'm Imbas (@imbas-forosnai), the AI collaborator pattern J works with. An Imbas review agent did this review at 0af9eafe, and I checked its findings against the source before posting. It's comment-only: approving in this repo is J's call.
Nothing here blocks the merge. We recommend approving. There is one low and one nit below.
What was checked
- HXML-25.
PickElementreturnsNonefor an absent or empty list, the first element for a one-element list, andPickRandomfor more than one.OffensiveElementbehaves as it does onmainin every case except a nullDamage, wheremainthrew.- On the server,
Monolith.csreadsOffensiveElementonly insideif (spawn.Damage != null)andDefensiveElementonly insideif (spawn.Defense != null). So the only behaviour that changes there is the intended fix. - With
main'sSpawn.csswapped in, 3 of the 4 new spawn tests fail, as the description says.
- On the server,
- HXML-24A.
- Both
xs:uniqueconstraints reject duplicates. Removing either constraint makes its duplicate test fail. - With
main's XSD swapped in, the sparse, 10×10 and same-target tests fail. GetMultiplierreturnsDefaultMultiplierwhen the source row, the target entry or theMultiplieris missing. An explicitMultiplier="0"returns 0.- A full 9×9 table validates under both schemas. Over all 81 pairs,
GetMultipliermatches the server's current lookup. - The element tables from world and ceridwen validate under this schema and fail under
main's.
- Both
- Generated code. Running
tools/regenerate.shon Linux (xscgen 3.0.1356) gives output byte-identical tosrc/Objects/ElementTable.csin this PR. - Tests. Against ceridwen HEAD, 103 pass and 3 fail. The 3 are the variant-count tests you named. They fail because
xml-build.ymlchecks out ceridwen at HEAD and the tests hardcode 176.mainwould fail them too if CI ran again; its last green run was 08-17.
Low: Source/@Element has no default, so the duplicate-source check has a hole
ElementTable.xsd, in ElementTableSourceElement, declares <xs:attribute name="Element" type="hyb:ElementType" /> with no default and no use="required". Target/@Element does have default="None", so the unique check sees two Targets that have no Element as duplicates of None, and rejects them.
A Source row with no Element gives the unique check no value, so the check skips that row. As a result:
- Two such rows validate.
- One such row next to
<Source Element="None">also validates.
The unnamed row deserialises with Element = None. GetMultiplier matches on s.Element == source and ignores ElementSpecified, so the unnamed row acts as the None row, and whichever matching row comes first wins. In a test, GetMultiplier(None, Wind) returned the unnamed row's 7.
This optionality was already in the schema before this PR, and no real data hits it. But it is a hole in the new "no duplicate Source" check. To fix it, add default="None" (as Target has) or use="required", and regenerate. It could also wait for branch B, which reworks this type. That choice is yours.
Nit: two test comments narrate history
tests/ElementTables.cs:68: "The schema required exactly 9 … until HXML-24"tests/SpawnElements.cs:47: "Until HXML-25, DefensiveElement counted…"
The house comment rule asks for no "was previously X" narration. The test names already say what each test checks. Optional.
For when HXML-7 lands (not a defect)
The server's current lookup (Creature.cs ~1078) gives 0 for a Target with no Multiplier. GetMultiplier gives DefaultMultiplier. That only matters once the server switches over. Today the server's lookup finds nothing in either real table, so element multipliers don't apply yet.
Source/@element has no default and is optional. A Source row without Element gives the xs:unique check no value, so the check skips the row. Two such rows pass validation. Each row deserializes as None. Source/@element is now use="required". The generator removes the ElementSpecified property. No code uses it. A new test makes sure that a Source row without Element fails validation. This change also removes two test comments that describe old behavior. The test names tell what each test checks.
|
Thank you for the review. Commit
The note about the server lookup is recorded for the server change that will use |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
==========================================
+ Coverage 61.11% 62.90% +1.78%
==========================================
Files 43 43
Lines 1538 1531 -7
Branches 349 351 +2
==========================================
+ Hits 940 963 +23
+ Misses 484 457 -27
+ Partials 114 111 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Imbas again (@imbas-forosnai). I checked
Nothing outstanding from our side. Approval is J's call in this repo (or Ange's). |
Summary
This PR makes two changes. Neither change breaks existing data or code.
1. Spawn.DefensiveElement reads the wrong list (HXML-25)
Spawn.DefensiveElementcountsDefense.Elements, but it readsDamage.Elements. The server uses this property inMonolith.csto set the defensive element of a spawned monster. The results are:<Defense>and no<Damage>throwsNullReferenceException.InvalidOperationException.Current content does not cause these errors. In
worldandceridwen, each spawn has the same elements in<Damage>and<Defense>.OffensiveElementandDefensiveElementnow use one helper. The helper reads the list of the property. It returnsNonewhen the list is absent or empty.2. Variable-size element table (HXML-24, part A)
ElementTable.xsdrequires exactly 9Sourcerows, each with exactly 9Targetentries. The element tables inworldandceridwenhave 10 rows, because they includeUndead. Both tables fail schema validation for this reason.SourceandTargetaccept 1 or more entries.DefaultMultiplierattribute on<ElementTable>(default1) applies to each pair that the table does not declare.xs:uniquerejects twoSourcerows with the same element. It also rejects twoTargetentries with the same element in one row.Source/@Elementis nowuse="required". Before this change, aSourcerow withoutElementgave thexs:uniquecheck no value, so the check skipped the row. The generator removes theElementSpecifiedproperty. No code uses it.ElementTable.GetMultiplier(source, target), returns the declared multiplier. It returnsDefaultMultiplierif the row, the entry or theMultiplierattribute is missing.The
ElementTypeenum does not change in this PR. A later PR changes the element set.Generated code
tools/regenerate.shregeneratessrc/Objects/ElementTable.cs. The only change is the newDefaultMultiplierproperty (17 lines).The regeneration ran on Windows. On Windows, the script writes a different header path and different line endings (HXML-26). These differences were removed, so the file is the same as the output of a Linux run.
Tests
main.GetMultiplier. The test project now copiessrc/XSD/*.xsdto its output directory, so the tests validate against the schema that ships.main:LoadDataContainsExpectedData,LoadDataAsyncContainsExpectedDataandGetTotal. PR XML: derive totals in tests from the actual state of ceridwen #9 fixes them.worldandceridwenelement tables pass validation against the new schema.Not in this PR
1.0.0-alpha2..First()and a fixed table name (HXML-7). After this PR is released, the server can useGetMultiplier.