Skip to content

refactor: BaseCreature to the SerializationGenerator with SaveFlag elision - #2592

Draft
kamronbatman wants to merge 11 commits into
mainfrom
feat/basecreature-codegen
Draft

kamronbatman wants to merge 11 commits into
mainfrom
feat/basecreature-codegen

Conversation

@kamronbatman

@kamronbatman kamronbatman commented Aug 23, 2026 •

Copy link
Copy Markdown
Member

Converts BaseCreature's hand-written v22 serialization to the SerializationGenerator (v23), aimed at save freeze time and disk for worlds with 500k–1M creatures.

The win

~40 fields sit behind [SaveFlag] (a single ulong flags word). A creature matching its defaults — most of a live world — writes a 13-byte tail (4 version + 8 flags + 1 default-AI) instead of ~236 bytes, and the writer's work becomes ~40 cheap predicates plus one flags write instead of ~50 unconditional writes. Written ints are [EncodedInt].

Table speeds are not persisted

The speed flags and their load defaults both resolve through GetSpeeds/GetMoveSpeeds: a creature whose speeds match its npc-speeds bucket writes nothing and re-reads the table at load. CurrentSpeed writes only when it differs from PassiveSpeed. Consequences:

  • npc-speeds.json edits now reach existing unmodified spawns on restart (the table is authoritative for untouched creatures).
  • Former paragons elide fully (UnConvert's snap-to-table); live paragons legitimately persist their scaled speeds.

Structure

  • Stateful SpeedClass + cached speed entry: the four per-save table checks (and elided-load defaults) read a SpeedClassEntry cached on the creature instead of walking the dictionaries each time. To keep the cache impossible to hold wrong, SpeedClass is no longer virtual: a type's constant bucket is virtual DefaultSpeedClass, and runtime state changes (boss enrage, [props) assign SpeedClass — the setter invalidates the cache, applies the new bucket's think/move speeds (preserving active/passive mode), and serializes only when it differs from the type default, so a changed bucket survives a save while bucket-matching speeds still elide. GetSpeeds/GetMoveSpeeds remain the override point for bypassing the table, with no cache in that path.

  • The four speeds are one block, and None means custom: either all of active/passive think + move conform to the bucket (elided as a set) or the creature is custom and all four serialize — partial conformance cannot exist on the wire. The old None → type list → Medium chain was construction-time defaulting, so the constructor now resolves it once into SpeedClass itself: at runtime a concrete bucket means table-backed, None means the creature's own speeds are authoritative. Tuning any speed flips the bucket to None (the label never lies), assigning a real bucket un-customs, runtime entry resolution is a single level lookup, and legacy loads demote nonconforming creatures (SetSpeed vendors) to None honestly.

  • Old saves (v0–22) load through the retained legacy Deserialize(reader, version), now assigning raw fields. Shared post-load fixups (stat timers, ChangeAIType, followers, animate-dead registration, unsummon timer, abandoned-pet fallback) moved to a single [AfterDeserialization] hook serving both paths.

  • Delete countdown is a [DeserializeTimer] field (anchored — downtime does not consume it, matching the old remaining-TimeSpan semantics). Stabled/controlled pets never persist one; the 3-day LastOwner fallback lives in the hook. SummonEnd stays [AnchoredDateTime], written only while summoned.

  • Side-effect setters became generated-field hooks: Team, Controlled, Summoned, resistance seeds (UpdateResistances), CurrentSpeed (AI timer notify), Loyalty (clamp via allowFieldChange).

  • Five properties stay hand-written as [SerializableProperty] with useField: — ControlMaster/SummonMaster (follower bookkeeping brackets the assignment), ControlOrder (order logic must run on equal re-assignment for pet commands), Tamable/IsParagon (custom getter / conversion must not run at load). ActiveMoveSpeed/PassiveMoveSpeed collapsed to plain [SerializableField]s (no longer virtual): they read the raw override (0 = inheriting) and CurrentMoveSpeed carries the inherit resolution.

  • CreatureDeathEvent/CreatureDeletedEvent moved to a CreatureEvents host class: the events generator and the serialization generator each emit a [GeneratedCode] partial for the declaring type, and the attribute forbids duplicates (CS0579) — BaseCreature is the first class using both. Can fold back if this is ever fixed generator-side.

  • One serialized master reference: ControlMaster and SummonMaster, when both set, are always the same mobile (Summon assigns both; every management flow set them in lockstep) — they differ only in presence (uncontrolled summons vs pets). One _master ref serializes and fans back out through the Controlled/Summoned flags at load; SetControlMaster now maintains the lockstep itself, deleting six hand-rolled copies of that boilerplate.

  • Missing npc-speeds.json degrades gracefully: a creature constructed without speeds logs debug and defaults to Medium (0.25/0.5) instead of spawning a 0-delay AI timer.

  • m_ fields renamed to _camelCase; MoveSpeedMod-era vestiges stay gone.

Testing

  • New BaseCreatureSerializationTests: default and fully-populated round trips with exact byte-consumption asserts; back-to-back saves byte-identical (anchored timer keeps idle saves stable); bucket assignment and partial-tuning-flips-to-custom round trips. (A byte-authentic fossilized v22 stream was validated through the legacy path during development, then dropped to avoid test bloat.)
  • Full suite passes (1561). Server.Mobiles.BaseCreature.v23.json generated and committed.

🤖 Generated with Claude Code

kamronbatman and others added 11 commits August 23, 2026 10:50
…ision

Converts BaseCreature's hand-written v22 serialization to codegen v23.
Nearly every field sits behind a [SaveFlag], so a creature matching its
defaults writes only [version int][ulong flags][default AI] - 13 bytes -
instead of ~236, and the writer's work is mostly branch-not-taken. With
500k-1M creatures in a world, this is the dominant slice of mobile save
freeze time and disk.

- Speeds serialize only when they differ from the creature's npc-speeds
  values (GetSpeeds/GetMoveSpeeds on both the flag check and the load
  default), so table edits now reach existing unmodified spawns on
  restart, and former paragons (snapped back to table values) elide
  fully. CurrentSpeed writes only when it differs from PassiveSpeed.
- The delete countdown is a [DeserializeTimer] field (anchored);
  stabled/controlled pets never persist one, and the abandoned-pet
  3-day fallback lives in AfterDeserialization for both load paths.
- SummonEnd stays anchored, written only while summoned.
- Old saves (v0-22) load through the retained legacy
  Deserialize(reader, version), now assigning raw fields; the shared
  post-load fixups (stat timers, AI creation, followers, animate-dead
  registration, unsummon timer) moved to [AfterDeserialization].
- Side-effect setters became generated-field hooks (Team, Controlled,
  Summoned, Loyalty clamp, resistance seeds, CurrentSpeed); properties
  whose semantics the hooks cannot express stay hand-written as
  [SerializableProperty] (ControlMaster/SummonMaster bracket the
  assignment with follower bookkeeping, ControlOrder must run on equal
  re-assignment, Tamable/IsParagon/move speeds have custom getters).
- CreatureDeathEvent/CreatureDeletedEvent moved to a CreatureEvents
  host class: the events generator and the serialization generator each
  emit a [GeneratedCode] partial for the declaring type, and the
  attribute forbids duplicates (CS0579).
- m_ fields renamed to _camelCase.

Tests: new-format round trips (default and fully populated) with exact
byte consumption, back-to-back saves byte-identical, and a
byte-authentic fossilized v22 stream loading through the legacy path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ActiveMoveSpeed/PassiveMoveSpeed become plain [SerializableField]s (not
virtual): the properties now read the raw override (0 = inheriting) and
CurrentMoveSpeed carries the inherit resolution - it was the only
production reader of the resolving getters. The <=0 coercion moves to an
allowFieldChange hook. Wire format unchanged (schema diff is empty).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removes narrative and developer commentary (historical essays, changelog
notes, say-what-the-code-does lines, flavor) and condenses the keepers:
constraint statements, wire-format markers, era-behavior notes, and the
weighted-random distribution table. TODOs use the terse house style.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No overrides exist.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Serialization consults the speed table four times per mob per save (the
Should* flag checks) and again through the Default* methods on elided
loads - each a SpeedClass/type dictionary walk. The resolved
SpeedClassEntry is now cached on the creature (one reference; the table
is immutable after Configure), so those become a null-check and field
reads. GetSpeeds/GetMoveSpeeds stay the virtual override point, so
stubs and forks that override them still steer elision; only their
default implementations read the cache. NPCSpeeds' per-call lookups
collapse into FindEntry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A virtual SpeedClass could be overridden dynamically (boss state change)
and silently diverge from the cached speed entry. SpeedClass is now
non-virtual instance state: assigning it invalidates the cached entry,
applies the new bucket's think and move speeds (preserving the
active/passive mode), and serializes only when it differs from the
type's DefaultSpeedClass - so a runtime bucket change survives a save
while its (bucket-matching) speeds still elide. Works from [props too.

Overrides become: DefaultSpeedClass for a type's constant bucket,
SpeedClass assignment for state changes, GetSpeeds/GetMoveSpeeds to
bypass the table entirely - none of which can leave the cache stale.

SpeedClass deserializes before the speed fields (index 7; later indexes
shift by one - v23 was never released, schema regenerated).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Either all four values (active/passive think + move) conform to the
creature's speed entry - elided as a set - or the creature is fully
custom and all four serialize. Partial conformance cannot exist on the
wire, so no value is ever left silently tracking the table beside a
hand-tuned sibling.

"Fully custom" is a real state: SpeedLevel.Custom (None already means
"resolve by type list" for the type-listed species, so it cannot double
as the custom marker). Tuning any speed flips the bucket to Custom (the
label never lies), Custom resolves no entry and short-circuits the
conformance check, a custom creature is its own GetSpeeds reference
(paragon snap becomes a natural no-op), and assigning a real bucket
un-customs it via ApplySpeedClass. A re-entrancy guard keeps the flip
from misreading ApplySpeedClass's half-assigned block, and constructors
seed raw fields so DefaultSpeedClass types do not flip at birth.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The None -> type list -> Medium chain was construction-time defaulting
(so creatures without a bucket or a SetSpeed call never spawn at 0/0),
not a live semantic. The constructor now resolves it once into
_speedClass itself, so at runtime a concrete bucket means table-backed
and None means the creature's own speeds are authoritative - which is
what SpeedLevel.Custom was; it is removed. Runtime entry resolution
collapses to a single level lookup, and the SpeedClass byte still
elides by comparing against the (cached) resolved type default.

Legacy loads guess the type default and demote to None when the loaded
speeds do not conform, so pre-codegen customized creatures (SetSpeed
vendors) come out honestly labeled. A constructor guard keeps a missing
speed table loud instead of spawning 0-delay creatures that spin their
AI timers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
It served its purpose validating the migration during development; the
legacy path is one-time upgrade code and the replica writer was most of
the file.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ControlMaster and SummonMaster, when both set, are always the same
mobile: BaseCreature.Summon assigns both to the caster, and every pet
management flow (transfer, stable, claim, ball of summoning, GM obey,
login overflow) followed SetControlMaster with an identical SummonMaster
assignment. They differ only in presence - uncontrolled summons carry
only a summon master, pets only a control master.

So one _master reference serializes (refreshed at save, fanned back out
through the Controlled/Summoned flags in AfterDeserialization; legacy
loads feed the same path), and SetControlMaster now keeps SummonMaster
in lockstep itself, deleting the six hand-rolled copies of that
boilerplate.

Also: a creature constructed without speeds (missing npc-speeds.json)
now logs debug and defaults to Medium (0.25/0.5) instead of throwing -
this is the place a sane default belongs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kamronbatman

Copy link
Copy Markdown
Member Author

Audit: ControlMaster vs SummonMaster, for the master collapse

The pure generator conversion moved to #2611 (Delta Saves #7, phase 3). This PR now carries two behaviour changes on top of it: SpeedClass and the single-Master collapse. An inventory of every read and write of the two references was done to decide whether the collapse is safe. Summary:

Writes. Every writer sets them together or sets exactly one, gated by the flags:

  • Summon(controlled: true) (every SpellHelper.Summon spell, familiars, steeds, talismans): both = caster.
  • Summon(controlled: false) (Blade Spirits, Energy Vortex, Animated Weapon, Nature's Fury, Vengeful Spirit, Animate Dead), Mirror Image clones, Summon Undead counter summons: SummonMaster only, Summoned = true, never Controlled.
  • SetControlMaster(x) (taming, hires, escorts, vendors, Clockwork Assembly, Horse Breeder): ControlMaster only. The claim/transfer/obey paths then copy it into SummonMaster only if (Summoned).
  • EnragedCreature: SummonMaster only with Summoned = false (MeerMage's summons, deliberately not dispellable and not a pet). OnThink deletes it when the master is gone.
  • BaseTalisman.Summon: controlled summon, then Summoned = false so it behaves as a pet (kill awards, (tame), stable-able) with an unsummon timer.
  • SetControlMaster(null) (release order, loyalty 0, escort/hire abandon, jail): clears ControlMaster only; SummonMaster is kept.

Reads. GetMaster() is flag-gated (Controlled ? CM : Summoned ? SM : null). Eleven sites use CM ?? SM (follower bookkeeping, pack instinct, familiars, guild lookup). Every ungated ControlMaster read only ever sees a value when Controlled, and every ungated SummonMaster read only when Summoned, with one exception: EnragedCreatures.cs (reads SummonMaster with Summoned false, and its line 121 presence test bc.Controlled || bc.SummonMaster != null on other creatures).

The only state with two different non-null masters is reachable but accidental: a talisman summon transferred to another player (Summoned is false, so the transfer does not update SummonMaster) ends with ControlMaster = to, SummonMaster = from for its remaining minutes. The only observable effect is that the original summoner's area spells exempt it. Nothing reads the disagreement on purpose.

A released summon keeps its caster on purpose. After "release" or loyalty 0 on a controlled summon, ControlMaster is null but SummonMaster stays: GetMaster() still credits the caster, BaseAI.IsInvalidSummonTarget still stops it attacking the caster, Notoriety still shields it from other players, the follower slot moves to SummonMaster. That is the one place the collapse must be careful.

Recommendation for this PR: one serialized Master plus the existing Controlled/Summoned flags is sound; no separate "dispellable" flag is needed (the Summoned = false users rely on all of Summoned's side effects, not just dispel, and IsDispellable is already virtual). Rules the fold must implement, which the earlier AfterDeserialization fan-out did not:

  1. SetControlMaster(null) clears Controlled and clears Master only when !Summoned.
  2. EnragedCreature reads and writes Master directly (a Summoned-gated SummonMaster view would delete it on the first OnThink); line 121 becomes bc.Controlled || bc.Master != null.
  3. Load migration: Master = ControlMaster ?? SummonMaster; when both are set and differ (the transfer case), keep ControlMaster and log.
  4. GetMaster() keeps its meaning: Controlled || Summoned ? Master : null.

Sites that then become no-ops and can be deleted: the if (Summoned) SummonMaster = x copies in AnimalTrainer, PlayerMobile auto-claim, BallOfSummoning, TransferItem, OnSpeech (GM obey), and HorseBreederGump's direct Controlled/ControlMaster writes (use SetControlMaster). Optional: the eleven CM ?? SM reads and SpellHelper.GetGuildFor collapse to Master.

Behaviour deltas, both arguably fixes: a transferred talisman summon no longer exempts its original summoner from area harm; a jail-stabled talisman summon no longer holds the summoner's follower slot.

@kamronbatman

Copy link
Copy Markdown
Member Author

Correction to the audit above: a summon can never be released

The "released summon keeps its caster" paragraph is wrong, and with it the !Summoned clause in rule 1. Every path that could clear ControlMaster on a summon kills or deletes it first:

  • Release order: PetOrderHandlers.cs:252-256 does if (Mobile.Summoned) { Mobile.Kill(); return; } before SetControlMaster(null); the think-side DoOrderRelease (PetOrders.cs:479-481) deletes when DeleteOnRelease, which is => _summoned (BaseCreature.cs:1256). Summons are not bondable, so Kill ends in Delete. That is OSI behaviour (dismiss), not a bug.
  • Loyalty drain: needs ten hours of continuous, commandable control; no controlled summon lives that long and is commandable (spell summons are minutes, ethics steeds one hour, necro familiars and talisman summons are Commandable => false). If it fired it would delete anyway.
  • Transfer and stable refuse summons; jail dispels them; death, Dispel, UnsummonTimer all delete.

So Controlled, Summoned and the master are always consistent for every creature except EnragedCreature (and the transferred talisman summon, where the stale SummonMaster is harmless). Revised rules for the collapse:

  1. SetControlMaster(null) clears Master unconditionally. Optional hardening: assert or dismiss when Summoned, so a future caller cannot recreate a split state.
  2. EnragedCreature reads and writes Master directly; its line 121 presence test becomes bc.Controlled || bc.Master != null.
  3. Load migration: Master = ControlMaster ?? SummonMaster; keep ControlMaster and log when both are set and differ.
  4. GetMaster() keeps its meaning: Controlled || Summoned ? Master : null.

Separate bug found (pre-existing, tame pets, not summons): the loyalty drain at BaseCreature.cs:5928-5941 calls c.AIObject.DoOrderRelease() directly instead of setting ControlOrder = Release, so it never reaches HandleReleaseOrder and SetControlMaster(null) (PetOrderHandlers.cs:272). A pet whose loyalty hits zero says it is better off without a master, gets the three-day delete timer and drops its pack, but stays Controlled with its ControlMaster and keeps holding the owner's follower slots, the opposite of the comment's intent. Worth its own fix outside this PR.

kamronbatman added a commit that referenced this pull request Sep 6, 2026
…ave requirement) (#2611)

The pure SerializationGenerator conversion of `BaseCreature`, split out of #2592 so it can serve as the reference for converting every other large hand-written class in the Delta Saves project (#7, phase 3). Two behaviour changes from #2592 are deliberately not here and follow in their own PRs on top of this one: `SpeedClass`, and the collapse of `ControlMaster`/`SummonMaster` into one reference (a creature can lose or keep either independently: Blade Spirits and Energy Vortexes are summoned but never controlled, EnragedCreature and talisman summons keep a summon master with neither flag set).

## What this is

- `BaseCreature` becomes `[SerializationGenerator(23, false)]` with a `[SerializableField]` per serialized slot and `[SaveFlag]` elision on nearly every field, so a stock creature serializes to its version plus flags. Field orders run 0..53 with no gaps (54 fields).
- The hand-written reader stays as `private void Deserialize(IGenericReader reader, int version)` for every pre-codegen version (0..22); post-codegen bumps use `MigrateFrom` from here on. `[AfterDeserialization]` carries the post-load fixups main did after reading (stat timers, AI type, followers, reacquire seeding).
- `ControlMaster` and `SummonMaster` stay two independent fields with main's semantics, each elided when null.
- `DamageMin`/`DamageMax`/`ActiveSpeed`/`PassiveSpeed` are no longer virtual (nothing in the tree overrode them); comments swept to the constraints that matter.
- Direct writes to serialized backing fields outside the generated setters (`SetDamage`, `SetResistance`, the move-speed helpers, the loot flag, feed loyalty, the delete timer) call `this.MarkDirty()`, matching the #2609 standard, so the class is ready for delta saves once `Mobile` is audited.
- Schema `Server.Mobiles.BaseCreature.v23.json` regenerated by the tool (a second run produces no diff).

## Deferred to follow-up PRs

SpeedClass: the serialized `_speedClass` field, `DefaultSpeedClass` replacing the type constant, `ApplySpeedClass`/`OnSpeedClassChange`, "None means custom", the four-speeds-as-one-block elision, `NPCSpeeds.FindEntry(SpeedLevel)`, the constructor fallback to Medium, and their tests.

Master references: serializing one `Master` with a `Controlled`/`Summoned` fan-out and the `SetControlMaster` lockstep.

## Tests

UOContent.Tests 776 / Server.Tests 855 green. `BaseCreatureSerializationTests` covers: a default creature elides to version + flags; a populated creature round-trips with exact byte consumption; back-to-back saves are byte-identical; an uncontrolled summon keeps its SummonMaster; byte-authentic v22 legacy streams (replicas of main's `Serialize`) load through the legacy reader for a wild tamable, a controlled pet, a controlled summon with an anchored `SummonEnd`, and a summon-master-only creature (the EnragedCreature shape); a running delete timer round-trips through `[DeserializeTimer]`; `Friends`, `CurrentWayPoint` and `HomeMap` round-trip; a `BaseVendor` stub round-trips the generated BaseVendor v2 → generated BaseCreature v23 chain.

## Behaviour notes for reviewers

- `ActiveMoveSpeed`/`PassiveMoveSpeed` getters return the raw override (0 = inherit); `CurrentMoveSpeed` is the resolved pace.
- Speeds elided as table defaults re-snap to the current `npc-speeds.json` on load, so table edits reach unmodified spawns on restart.
- `GetSpeeds` no longer throws on the save/load path when the table has no entry for the type: saves elide against the creature's own values and loads keep the stream. Construction still throws (`InvalidOperationException`, was `KeyNotFoundException`). An elided load with no table entry would otherwise resume at speed 0, so `[AfterDeserialization]` logs once and paces it at Medium.
- `virtual` removed from `ActiveSpeed`, `PassiveSpeed`, `DamageMin`, `DamageMax` (no overrides in the tree; forks may have some).
- `ControlMaster`, `SummonMaster`, `ControlOrder`, `Tamable`, `IsParagon` are `[SerializableProperty]` over hand-written setters because follower bookkeeping must run before the assignment, which a `fieldChanged` hook cannot express; the wire format is identical.

## Prerequisites for cherry-picking
#2609 (BaseVendor is already generated on top of BaseCreature) and SerializationGenerator 4.1.0.
kamronbatman added a commit that referenced this pull request Sep 6, 2026
… breeder and notoriety guards (#2613)

Pet and summon bugs found by the master-reference audit for #2592 (comments there have the full inventory). Independent of delta saves.

## Releasing a pet never finished

The Release order ran two half-releases that never met:

- The order handler cleared the master. That set `Controlled` to false, so `Obey` never ran again and the think-side `DoOrderRelease` (re-home, three-day delete timer, backpack drop) was dead code: a released pet kept its pack and never despawned.
- The loyalty drain called `DoOrderRelease` directly, so a pet whose loyalty hit zero got the countdown and dropped its pack but kept its master and its owner's follower slots, the opposite of the code comment's intent.

`DoOrderRelease` is now the whole release (targets, bonding, `SetControlMaster(null)`, re-home, delete or countdown, pack drop) and runs once, synchronously, from the handler or the loyalty drain. Summons still die on release, as before. Two tests pin both entry points: master cleared, follower slots returned, countdown running, home anchored.

This is the one place the `PetOrders` / `PetOrderHandlers` split bit; the wider audit of that duplication is a separate task.

## Summon master follows the pet

Transfer, stable claim, GM "obey" and Ball of Summoning copied `SummonMaster` only when `Summoned`, so a talisman summon (`Summoned` is false, `SummonMaster` set) kept its original summoner after a transfer: two different masters on one creature, with the original summoner's area spells still exempting it. They now mirror the summon master whenever it is set. Jail stabling cleared only `ControlMaster`, so a jailed talisman summon kept charging the summoner's follower slots; it now clears both, like the stable master and auto-stable already do.

## Small ones

- The faction horse breeder set `Controlled`/`ControlMaster` directly; it now goes through `SetControlMaster` and tells the buyer why it refused (1049607) instead of silently deleting the horse.
- `Notoriety` dereferenced `SummonMaster` on a summoned creature without a null check.

## Tests

UOContent.Tests 778 green (two new).
@kamronbatman
kamronbatman marked this pull request as draft September 12, 2026 06:02

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant