From 309fcfeb27aa7cc943d44e8c6e17d2ae2e4d687a Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:58:44 -0700 Subject: [PATCH] feat(skills): SkillEvents.SkillUsed for cross-assembly subscribers; InternalsVisibleTo ModernSpawner.Tests (#2636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Two small additive changes that an external content assembly (ModernSpawner) needs, as separable commits. **1. `SkillEvents.SkillUsed`** (`Projects/UOContent/Skills/SkillEvents.cs`, namespace `Server.Misc`): a plain C# event `Action` raised once per skill attempt from each of the four `Mobile_SkillCheck*` handlers, with the handler's own result. Attempts the handler resolves without a roll (too difficult, no challenge) raise too, so a grandmaster's trivial success and a guaranteed combat roll are observable. Not raised when the mobile lacks the skill. Each handler keeps its logic in a private core method and raises on the way out, so there is exactly one raise per attempt and `CheckSkill` itself is unchanged. - **Why a plain event and not a `[GeneratedEvent]`:** generated events are compile-time static dispatch inside the UOContent compilation, so a subscriber in another assembly cannot use `[OnEvent]`. Shape follows `HelpEvents`. - **Why "used", not "gained":** this is the XmlSpawner skill-trigger semantic (it wrapped the same four handlers and passed their result as `success`; its grammar was `Skill[+/-]` for success-only or failure-only). Gains are already observable through the existing skill-change notification on `Mobile`. - **Cost:** one delegate null-check per attempt when nothing is subscribed; no boxing, no closure, no allocation. The handlers sit on the combat swing path. - **Exception contract:** subscriber exceptions propagate, matching `EventSink`/`HelpEvents`; no try/catch by design. **2. `InternalsVisibleTo("ModernSpawner.Tests")`** on `Server.csproj`, beside the existing `Server.Tests`/`UOContent.Tests` entries, so an external test host can seed `Core._now` the way the engine's own test initializers do. Separable; a public test seam on `Core` would serve the same need without naming a downstream assembly. ## Open question The payload is the `Skill` object plus a positional `bool`. A `readonly struct` args type passed `in` would leave room to add `chance` or the target later without breaking subscribers. Happy to change before merge. ## Test plan - [x] `UOContent.Tests`: 4 tests — a rolled attempt raises once with the returned outcome; each short-circuit path (no challenge, too difficult, on both the direct and value-window handlers) raises with the handler's result; a direct `CheckSkill` call does not raise; no subscriber does not throw. Full suite green. - [x] `Server` and `UOContent` build clean with `TreatWarningsAsErrors`. - [ ] CI --- Projects/Server/Server.csproj | 3 + .../Tests/Skills/SkillEventsTests.cs | 124 ++++++++++++++++++ Projects/UOContent/Skills/SkillCheck.cs | 28 ++++ Projects/UOContent/Skills/SkillEvents.cs | 23 ++++ dev-docs/events.md | 11 ++ 5 files changed, 189 insertions(+) create mode 100644 Projects/UOContent.Tests/Tests/Skills/SkillEventsTests.cs create mode 100644 Projects/UOContent/Skills/SkillEvents.cs diff --git a/Projects/Server/Server.csproj b/Projects/Server/Server.csproj index dfef0af46..8ec416218 100644 --- a/Projects/Server/Server.csproj +++ b/Projects/Server/Server.csproj @@ -50,5 +50,8 @@ <_Parameter1>UOContent.Tests + + <_Parameter1>ModernSpawner.Tests + diff --git a/Projects/UOContent.Tests/Tests/Skills/SkillEventsTests.cs b/Projects/UOContent.Tests/Tests/Skills/SkillEventsTests.cs new file mode 100644 index 000000000..91fba77bc --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Skills/SkillEventsTests.cs @@ -0,0 +1,124 @@ +using Server; +using Server.Misc; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class SkillEventsTests +{ + private sealed class Recorder + { + public Mobile From; + public Skill Skill; + public bool Success; + public int Calls; + + public void Handle(Mobile from, Skill skill, bool success) + { + From = from; + Skill = skill; + Success = success; + Calls++; + } + } + + [Fact] + public void DirectTarget_RolledAttempt_RaisesOnceWithTheReturnedOutcome() + { + var from = new Mobile(); + var skill = from.Skills[SkillName.Mining]; + var recorder = new Recorder(); + + SkillEvents.SkillUsed += recorder.Handle; + try + { + var rolled = SkillCheck.Mobile_SkillCheckDirectTarget(from, SkillName.Mining, null, 0.5); + Assert.Equal(1, recorder.Calls); + Assert.Same(from, recorder.From); + Assert.Same(skill, recorder.Skill); + Assert.Equal(rolled, recorder.Success); + + Assert.False(SkillCheck.Mobile_SkillCheckDirectTarget(from, SkillName.Mining, null, 0.0)); + Assert.Equal(2, recorder.Calls); + Assert.False(recorder.Success); + } + finally + { + SkillEvents.SkillUsed -= recorder.Handle; + from.Delete(); + } + } + + [Fact] + public void ShortCircuits_StillRaise_WithTheHandlerOutcome() + { + var from = new Mobile(); + var recorder = new Recorder(); + + SkillEvents.SkillUsed += recorder.Handle; + try + { + Assert.True(SkillCheck.Mobile_SkillCheckDirectLocation(from, SkillName.Mining, 1.0)); + Assert.Equal(1, recorder.Calls); + Assert.True(recorder.Success); + + Assert.False(SkillCheck.Mobile_SkillCheckDirectTarget(from, SkillName.Mining, null, -0.1)); + Assert.Equal(2, recorder.Calls); + Assert.False(recorder.Success); + + Assert.False(SkillCheck.Mobile_SkillCheckLocation(from, SkillName.Mining, 50.0, 100.0)); + Assert.Equal(3, recorder.Calls); + Assert.False(recorder.Success); + + Assert.True(SkillCheck.Mobile_SkillCheckTarget(from, SkillName.Mining, null, 0.0, 0.0)); + Assert.Equal(4, recorder.Calls); + Assert.True(recorder.Success); + } + finally + { + SkillEvents.SkillUsed -= recorder.Handle; + from.Delete(); + } + } + + [Fact] + public void CheckSkill_Direct_DoesNotRaise() + { + var from = new Mobile(); + var skill = from.Skills[SkillName.Mining]; + var recorder = new Recorder(); + + SkillEvents.SkillUsed += recorder.Handle; + try + { + SkillCheck.CheckSkill(from, skill, null, 1.0); + Assert.Equal(0, recorder.Calls); + } + finally + { + SkillEvents.SkillUsed -= recorder.Handle; + from.Delete(); + } + } + + [Fact] + public void NoSubscriber_DoesNotThrow() + { + var from = new Mobile(); + var recorder = new Recorder(); + + try + { + SkillEvents.SkillUsed += recorder.Handle; + SkillEvents.SkillUsed -= recorder.Handle; + + Assert.True(SkillCheck.Mobile_SkillCheckDirectLocation(from, SkillName.Mining, 1.0)); + Assert.Equal(0, recorder.Calls); + } + finally + { + from.Delete(); + } + } +} diff --git a/Projects/UOContent/Skills/SkillCheck.cs b/Projects/UOContent/Skills/SkillCheck.cs index 5913b0a7f..59f8ebb6a 100644 --- a/Projects/UOContent/Skills/SkillCheck.cs +++ b/Projects/UOContent/Skills/SkillCheck.cs @@ -48,6 +48,13 @@ public static class SkillCheck return false; } + var success = CheckLocation(from, skill, minSkill, maxSkill); + SkillEvents.InvokeSkillUsed(from, skill, success); + return success; + } + + private static bool CheckLocation(Mobile from, Skill skill, double minSkill, double maxSkill) + { var value = skill.Value; if (value < minSkill) @@ -76,6 +83,13 @@ public static class SkillCheck return false; } + var success = CheckDirectLocation(from, skill, chance); + SkillEvents.InvokeSkillUsed(from, skill, success); + return success; + } + + private static bool CheckDirectLocation(Mobile from, Skill skill, double chance) + { if (chance < 0.0) { return false; // Too difficult @@ -156,6 +170,13 @@ public static class SkillCheck return false; } + var success = CheckTarget(from, skill, target, minSkill, maxSkill); + SkillEvents.InvokeSkillUsed(from, skill, success); + return success; + } + + private static bool CheckTarget(Mobile from, Skill skill, object target, double minSkill, double maxSkill) + { var value = skill.Value; if (value < minSkill) @@ -182,6 +203,13 @@ public static class SkillCheck return false; } + var success = CheckDirectTarget(from, skill, target, chance); + SkillEvents.InvokeSkillUsed(from, skill, success); + return success; + } + + private static bool CheckDirectTarget(Mobile from, Skill skill, object target, double chance) + { if (chance < 0.0) { return false; // Too difficult diff --git a/Projects/UOContent/Skills/SkillEvents.cs b/Projects/UOContent/Skills/SkillEvents.cs new file mode 100644 index 000000000..dec624c45 --- /dev/null +++ b/Projects/UOContent/Skills/SkillEvents.cs @@ -0,0 +1,23 @@ +using System; +using System.Runtime.CompilerServices; + +namespace Server.Misc; + +/// +/// Skill system events. Plain C# events so other assemblies can subscribe; generated events cannot be +/// subscribed across assemblies. +/// +public static class SkillEvents +{ + /// + /// Raised once per skill attempt from the four Mobile_SkillCheck* handlers with the attempt's + /// outcome, including attempts the handler resolves without a roll (too difficult, no challenge). + /// Not raised when the mobile lacks the skill. Fires for every , including + /// creatures. Subscribers must not block or allocate. + /// + public static event Action SkillUsed; + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void InvokeSkillUsed(Mobile from, Skill skill, bool success) => + SkillUsed?.Invoke(from, skill, success); +} diff --git a/dev-docs/events.md b/dev-docs/events.md index 2fcbeda68..589860db6 100644 --- a/dev-docs/events.md +++ b/dev-docs/events.md @@ -242,6 +242,17 @@ public static void HandlePlayerLogin(PlayerMobile player) --- +## Static Content Events + +Generated events dispatch statically inside `UOContent`; content that other assemblies must observe exposes a +plain `static event` instead (shape: `Projects/UOContent/Engines/Help/HelpEvents.cs`). + +- `SkillEvents.SkillUsed` -- `Action`, raised once per skill attempt from the + four `Mobile_SkillCheck*` handlers with the attempt's outcome, including attempts resolved without a roll + (too difficult, no challenge). Not raised when the mobile lacks the skill. Fires for every `Mobile`. + +--- + ## Event Args Pooling Pattern Some EventArgs use object pooling to avoid allocation in hot paths: