feat(triggers): skill triggers fire from ModernUO SkillEvents.SkillUsed; XmlSpawner outcome and value-window semantics - #4
Merged
Conversation
… SDK 18.10.0 to match Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Replace SkillTrigger's (SkillName)(-1) sentinel with an explicit AnySkill flag, add SkillOutcome (Any/Success/Failure) and MaxSkillValue so the grammar becomes skill:<Skill>[+|-]:<range>:<min>[-<max>]:<los>:<cooldown>. Add a pure MatchesContext helper for skill/outcome/window checks, and extend TriggerContext with SkillSuccess/SkillValue for Task 2 to populate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…host seeds the clock ModernSpawnerEvents becomes a one-way bridge: Configure() subscribes once to Server.Misc.SkillEvents.SkillUsed and forwards player attempts to the trigger system. The old C# event and the "call this from SkillCheck.cs" shim had no callers and are gone. ModernSpawnerConfiguration.Configure() now wires it. TriggerSystem.OnSkillUse takes (Mobile, Skill, bool) and fills the context with UsedSkill, SkillValue and SkillSuccess, so Task 1's outcome filter and value window are reachable end to end. Allocation is unchanged: the per-spawner TriggerContext that was already built is the only one. The test host seeds Core._now the way production does (Server.dll grants ModernSpawner.Tests InternalsVisibleTo) and gains AdvanceClock, so cooldowns and any absolute-clock comparison behave instead of reading as "never elapsed". SkillTriggerTests drives the real SkillCheck.Mobile_SkillCheckDirectTarget handler and covers range, outcome filtering, creatures, and the cooldown gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…context; OnSkillUse on ITriggerSystem
Review round 1 on the skill-trigger dispatch path, which runs on every player
skill attempt server-wide.
Skill.Value was read once per map-matching spawner; each read re-derives the
stat-scaled value through NonRacialValue and adds the racial bonus, so it is now
hoisted alongside skillName and read once for the whole dispatch.
Each map-matching spawner also allocated a TriggerContext before any trigger was
consulted. A cheap indexed pre-scan for MatchesSkill(skillName) now runs first,
so a spawner holding triggers for other skills allocates nothing. The pre-scan
records the first matching index and the evaluation loop resumes there rather
than rescanning from zero; both loops are indexed, with no enumerator and no
closure.
ITriggerSystem gains OnSkillUse(Mobile, Skill, bool) next to OnSpeech, so the
interface matches what TriggerSystem exposes. ModernSpawnerEvents.OnSkillUsed
uses `is not { Player: true }` instead of a lifted bool? comparison, and
ModernSpawnerTestServer.AdvanceClock documents that the clock only moves forward
and is shared, so tests must assert elapsed intervals rather than absolute
deadlines.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…mmar Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…ops iterate a snapshot; fixed test clock
SkillTrigger.Parse threw ArgumentOutOfRangeException on an empty value segment
("skill:Mining:10::false:5"): IndexOf('-', 1) rejects startIndex 1 on a
zero-length string. That took down the whole ActivateTriggers pass instead of
skipping one definition. An empty window is now malformed and returns null.
RequireLOS checked visibility, not line of sight. Mobile.CanSee(Item) ends in
item.Visible and BaseSpawner sets Visible = false, so a RequireLOS skill or
proximity trigger could never fire for a player. Both now use Mobile.InLOS.
SkillTrigger.Evaluate runs MatchesContext immediately after the spawner/Running
guard, so an attempt this trigger does not react to - the common case on a
server-wide dispatch - pays nothing for the cooldown, map, range and LOS checks.
OnSkillUse and CheckTimeOfDayTriggers called spawner.Trigger() while enumerating
their dictionaries; Trigger() reaches Spawn() and any attached script, and a
DESPAWN script or a spawned ModernSpawner can delete or register a spawner that
carries a trigger, invalidating the enumerator. Both now copy their entries into
an STArrayPool-rented KeyValuePair buffer, iterate it by index, skip entries
whose spawner has since been deleted or unregistered, and return the buffer
cleared in a finally. OnSkillUse also returns early when nothing is registered,
which is the common case on a shard with no skill triggers.
Parser hardening: Enum.TryParse<SkillName> accepts any numeric string, so both
SkillTrigger.Parse and XmlSpawnerMigrator.MapSkillTrigger now require
Enum.IsDefined; a failed range parse keeps the documented default of 10 rather
than falling to 0 (and so to 1 through Math.Max); MapSkillTrigger rejects an
inverted value window down the same warning path as an unknown skill name, and
documents that an absent min reads as 0. TriggerSystem.ParseTrigger logs a
warning when a registered factory returns null, which was silently swallowed,
and its three Console.WriteLine calls become Logger calls.
The test host seeds a fixed 2020-01-01T12:00:00Z rather than DateTime.UtcNow, so
wall-clock-sensitive triggers take the same branch on every run, and AdvanceClock
ignores a non-positive span per its documented forward-only contract.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
The audit-status column in product-spec.md records what the audit at the pinned commit found, not what this branch has since built. The skill row was flipped to "Implemented" when the wiring landed, which erases the finding the column exists to carry; it goes back to "Stubbed", with the as-built description staying in the v1-target cell, now noting that RequireLOS is line of sight rather than visibility. architecture.md marks the skill trigger "yes, tested" alongside kill, and its skill bullet says spawners with no matching trigger allocate nothing (they still pay one map compare and a linear scan of their trigger list) rather than "pay nothing"; the InLOS and snapshot-dispatch facts are recorded there too. xmlspawner-migration.md keeps the "never fired" claim but states it as what was verified: the XmlSpawner sources, ServUO and the ModernUO port alike, declare the parsed skill-trigger fields and read them, but never assign them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wires
skill:triggers to ModernUO'sSkillEvents.SkillUsed(#2636) so they fire for real, and gives them XmlSpawner's semantics.309fcfeb2(post-#2636main); test SDK 18.10.0 to match ModernUO's test projects.ModernSpawnerEvents.Configure()subscribesSkillEvents.SkillUsedand forwards player attempts (creatures are ignored; they roll skill checks every swing) toTriggerSystem.OnSkillUse(Mobile, Skill, bool success).TriggerContextcarriesSkillSuccessandSkillValue.SkillTriggergains an outcome (+success only,-failure only, none for either), a value window, and anAnySkillflag replacing the(SkillName)(-1)cast and theAlchemysentinel. Grammar stays positional and grows compatibly:skill:<Skill>[+|-]:<range>:<min>[-<max>]:<los>:<cooldown>; every existing definition parses with unchanged meaning andParse(Serialize(x))round-trips every field. XmlSpawner's own skill trigger never fired in ServUO (itsSkill[+/-][,min,max]parser was unwired), so only the intended semantics carry over.SkillTriggerattribute onto the grammar, using the node's proximity range.Core._now, via theInternalsVisibleTofrom #2636) and exposes a one-wayAdvanceClock, so cooldown behaviour is testable and time-window triggers do not branch on the wall clock; end-to-end tests drive realSkillCheckhandlers on a placedPlayerMobile.RequireLOSon skill and proximity triggers now usesMobile.InLOS(it usedCanSee, which ends inItem.Visible, and spawners are invisible, so it could never pass for a player). The skill and time-of-day dispatch loops iterate a pooled snapshot of the registry, so a trigger action that deletes or registers a spawner cannot invalidate the enumeration.SkillTrigger.Parserejects empty and out-of-range segments instead of throwing;TriggerSystem.ParseTriggerwarns when a definition is rejected; the migrator warns on unknown skills and inverted windows.Test plan
Anyflag, and every existing definition.Mining,Mining+,Magery-,50,90; unknown skill name and inverted window are skipped with a warning.RequireLOSnow callingMobile.InLOShas no test:Map.LineOfSightneeds the tile matrix the test host deliberately does not load. Verified by reading.dotnet build ModernSpawner.slnxclean; full suite green (464).