Skip to content

Variable-size element table, and a fix for Spawn.DefensiveElement - #8

Merged
baughj merged 3 commits into
mainfrom
feature/element-table-variable-size
Oct 23, 2026
Merged

baughj merged 3 commits into
mainfrom
feature/element-table-variable-size

Conversation

@Caeldeth

@Caeldeth Caeldeth commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR makes two changes. Neither change breaks existing data or code.

1. Spawn.DefensiveElement reads the wrong list (HXML-25)

Spawn.DefensiveElement counts Defense.Elements, but it reads Damage.Elements. The server uses this property in Monolith.cs to set the defensive element of a spawned monster. The results are:

  • A spawn with <Defense> and no <Damage> throws NullReferenceException.
  • A spawn with one defense element and no damage elements throws InvalidOperationException.
  • A spawn with different damage and defense lists gets the wrong defensive element.

Current content does not cause these errors. In world and ceridwen, each spawn has the same elements in <Damage> and <Defense>.

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.

2. Variable-size element table (HXML-24, part A)

ElementTable.xsd requires exactly 9 Source rows, each with exactly 9 Target entries. The element tables in world and ceridwen have 10 rows, because they include Undead. Both tables fail schema validation for this reason.

  • Source and Target accept 1 or more entries.
  • A new DefaultMultiplier attribute on <ElementTable> (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 row.
  • Source/@Element is now use="required". Before this change, a Source row without Element gave the xs:unique check no value, so the check skipped the row. The generator removes the ElementSpecified property. No code uses it.
  • A new method, ElementTable.GetMultiplier(source, target), returns the declared multiplier. It returns DefaultMultiplier if the row, the entry or the Multiplier attribute is missing.

The ElementType enum does not change in this PR. A later PR changes the element set.

Generated code

tools/regenerate.sh regenerates src/Objects/ElementTable.cs. The only change is the new DefaultMultiplier property (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

  • 4 new tests for the spawn elements. 3 of them fail on main.
  • 11 new tests for the element table: schema validation and GetMultiplier. The test project now copies src/XSD/*.xsd to its output directory, so the tests validate against the schema that ships.
  • Full suite: 104 pass, 3 fail. The same 3 tests fail on main: LoadDataContainsExpectedData, LoadDataAsyncContainsExpectedData and GetTotal. PR XML: derive totals in tests from the actual state of ceridwen #9 fixes them.
  • The world and ceridwen element tables pass validation against the new schema.

Not in this PR

  • The package version stays 1.0.0-alpha2.
  • The server still reads the element table with .First() and a fixed table name (HXML-7). After this PR is released, the server can use GetMultiplier.

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 imbas-forosnai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. PickElement returns None for an absent or empty list, the first element for a one-element list, and PickRandom for more than one. OffensiveElement behaves as it does on main in every case except a null Damage, where main threw.
    • On the server, Monolith.cs reads OffensiveElement only inside if (spawn.Damage != null) and DefensiveElement only inside if (spawn.Defense != null). So the only behaviour that changes there is the intended fix.
    • With main's Spawn.cs swapped in, 3 of the 4 new spawn tests fail, as the description says.
  • HXML-24A.
    • Both xs:unique constraints 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.
    • GetMultiplier returns DefaultMultiplier when the source row, the target entry or the Multiplier is missing. An explicit Multiplier="0" returns 0.
    • A full 9×9 table validates under both schemas. Over all 81 pairs, GetMultiplier matches the server's current lookup.
    • The element tables from world and ceridwen validate under this schema and fail under main's.
  • Generated code. Running tools/regenerate.sh on Linux (xscgen 3.0.1356) gives output byte-identical to src/Objects/ElementTable.cs in 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.yml checks out ceridwen at HEAD and the tests hardcode 176. main would 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.
@Caeldeth

Copy link
Copy Markdown
Contributor Author

Thank you for the review. Commit c789917 fixes the two points:

  • Source/@Element is now use="required". The generator removes the ElementSpecified property. A new test, SourceWithoutElementIsInvalid, makes sure that one or two Source rows without Element fail validation. The test fails on the previous schema.
  • The two test comments that describe old behavior are removed.

The note about the server lookup is recorded for the server change that will use GetMultiplier. Today a Target without a Multiplier gives 0 there, and GetMultiplier gives DefaultMultiplier.

@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.90%. Comparing base (3fac74d) to head (c789917).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Extensions/Spawn.cs 61.53% 3 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imbas-forosnai

Copy link
Copy Markdown
Contributor

Imbas again (@imbas-forosnai). I checked c789917 against the previous review:

  • Source/@Element is now use="required", and the generated ElementTableSourceElement drops ElementSpecified. Nothing in xml or hybrasyl-server references ElementSpecified on a source row, so the change compiles cleanly downstream.
  • The real tables still validate: in both world and ceridwen, 10 of 10 Source rows carry an Element.
  • SourceWithoutElementIsInvalid covers both the one-unnamed-row case and the two-unnamed-rows case, which was the hole.
  • Both history comments are gone.
  • CI is green now that HXML-1 is on main, which brings in the derived totals the three variant-count tests needed.

Nothing outstanding from our side. Approval is J's call in this repo (or Ange's).

@baughj
baughj merged commit 8e79faf into main Oct 23, 2026
3 checks passed
@Caeldeth
Caeldeth deleted the feature/element-table-variable-size branch October 24, 2026 20:12

This branch was previously deployed

1 inactive deployment
deploy — c7899170 Deployed Oct 23, 2026 by Caeldeth via build (10.0.x) #121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants