From 10fb74b8270d09b6bff52259205deeab1775f393 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Mon, 8 Jun 2026 23:42:28 -0700 Subject: [PATCH] fix(necromancy): correct Blood Oath duration, reflection, and expiry timing (#1690) (#2480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1690. Addresses Blood Oath holistically — three bugs found while researching the spell against RunUO, ServUO, the UODemise/uo.com guides, and the archived UOGuide page. ## Bugs fixed ### 1. Expiry timing (the filed issue) The `ExpireTimer` polled every 1s, so expiry and death/delete cleanup lagged up to ~1s. Replaced with a **single-shot** timer plus centralized `[OnEvent]` handlers on `PlayerDeathEvent`/`PlayerDeletedEvent`/`CreatureDeathEvent`/`CreatureDeletedEvent` — the oath now breaks immediately on death/delete of either party. ### 2. Duration formula Used `/80` (the bugged in-game tooltip value) instead of the real OSI formula `((SpiritSpeak - Resist) / 8) + 8`. Confirmed by RunUO, ServUO, the emulator guides, and the code's own fixed-point comment. At GM Spirit Speak this changes duration from ~9.5s to 23s and makes Spirit Speak actually affect duration. ### 3. Damage reflection (`BaseCreature.Damage` vs `PlayerMobile.Damage`) `BaseCreature.Damage` diverged: it attributed the reflected hit to the attacker itself (`from.Damage(amount, from)`) instead of the caster, reflected the bonused (not original) amount, used `×1.1` vs `×1.2`, lacked the caster-survival guard, and had no Publish 48 resist mitigation. Unified both paths: reflect the **original** damage attributed to the **caster** at `×1.2`. Publish 48 resist mitigation now applies only to creature casters and is gated behind `Core.SA`. ## Internals - Collapsed the parallel `_oathTable` into a single `_table` keyed by both participants → shared timer, so `RemoveCurse` resolves from either side (required by the event handlers). - Extracted `GetDurationSeconds` and `ComputeReflectedDamage` as testable statics. ## Tests 13 new tests (duration formula, reflection mitigation, oath lifecycle, end-to-end event-driven removal). Full suite: **436/436 pass**. --- .../Spells/Necromancy/BloodOathSpellTests.cs | 140 ++++++++++++++++++ Projects/UOContent/Mobiles/BaseCreature.cs | 20 ++- Projects/UOContent/Mobiles/PlayerMobile.cs | 11 +- .../Spells/Necromancy/BloodOathSpell.cs | 108 ++++++++------ 4 files changed, 223 insertions(+), 56 deletions(-) create mode 100644 Projects/UOContent.Tests/Tests/Spells/Necromancy/BloodOathSpellTests.cs diff --git a/Projects/UOContent.Tests/Tests/Spells/Necromancy/BloodOathSpellTests.cs b/Projects/UOContent.Tests/Tests/Spells/Necromancy/BloodOathSpellTests.cs new file mode 100644 index 000000000..0264a282e --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Spells/Necromancy/BloodOathSpellTests.cs @@ -0,0 +1,140 @@ +using System; +using Server.Mobiles; +using Server.Spells.Necromancy; +using Xunit; + +namespace Server.Tests.Spells.Necromancy; + +[Collection("Sequential UOContent Tests")] +public class BloodOathSpellTests +{ + private static Mobile NewMobile() + { + var m = new Mobile(World.NewMobile); + m.DefaultMobileInit(); + return m; + } + + // Issue #1690: the real OSI duration is ((SpiritSpeak - Resist) / 8) + 8 seconds. + // ModernUO previously used /80 (matching the bugged in-game tooltip), which made + // Spirit Speak almost irrelevant to duration. RunUO/ServUO and the code's own + // fixed-point comment both use /8. + [Theory] + [InlineData(120.0, 0.0, 23.0)] // GM Spirit Speak, no resist + [InlineData(120.0, 120.0, 8.0)] // equal skills -> baseline 8s + [InlineData(100.0, 20.0, 18.0)] // (100-20)/8 + 8 + [InlineData(20.0, 0.0, 10.5)] // minimum skill + public void GetDurationSeconds_UsesDivideByEight(double ss, double resist, double expected) + { + Assert.Equal(expected, BloodOathSpell.GetDurationSeconds(ss, resist), 3); + } + + // Player caster: Publish 48 resist mitigation does NOT apply; the attacker takes the + // full reflected (original, un-bonused) damage. + [Fact] + public void ComputeReflectedDamage_NoMitigation_ReturnsOriginal() + { + Assert.Equal(40, BloodOathSpell.ComputeReflectedDamage(40, 120.0, applyResistMitigation: false)); + } + + // Creature caster (Publish 48 / SA+): ((Resist * 10) / 20) + 10 = % of reflected damage resisted. + [Theory] + [InlineData(40, 0.0, 36)] // 10% resisted -> 40 * 0.90 + [InlineData(40, 100.0, 16)] // 60% resisted -> 40 * 0.40 + [InlineData(40, 120.0, 12)] // 70% resisted -> 40 * 0.30 + public void ComputeReflectedDamage_WithMitigation_ReducesByResist(int dmg, double resist, int expected) + { + Assert.Equal(expected, BloodOathSpell.ComputeReflectedDamage(dmg, resist, applyResistMitigation: true)); + } + + // The cursed target reflects to the caster; the caster attacking other mobiles must not reflect. + [Fact] + public void RegisterOath_BindsTargetToCaster_NotCasterToSelf() + { + var caster = NewMobile(); + var target = NewMobile(); + + BloodOathSpell.RegisterOath(caster, target, TimeSpan.FromMinutes(5)); + + Assert.Equal(caster, BloodOathSpell.GetBloodOath(target)); + Assert.Null(BloodOathSpell.GetBloodOath(caster)); + + BloodOathSpell.RemoveCurse(target); + caster.Delete(); + target.Delete(); + } + + // Death/delete of either party removes the oath, so RemoveCurse must resolve from either + // the caster or the target key (the death hooks call it with `this`). + [Fact] + public void RemoveCurse_ByCaster_ClearsBothEntries() + { + var caster = NewMobile(); + var target = NewMobile(); + BloodOathSpell.RegisterOath(caster, target, TimeSpan.FromMinutes(5)); + + Assert.True(BloodOathSpell.RemoveCurse(caster)); + + Assert.Null(BloodOathSpell.GetBloodOath(target)); + Assert.False(BloodOathSpell.RemoveCurse(target)); // already removed + + caster.Delete(); + target.Delete(); + } + + [Fact] + public void RemoveCurse_NotCursed_ReturnsFalse() + { + var m = NewMobile(); + Assert.False(BloodOathSpell.RemoveCurse(m)); + m.Delete(); + } + + // Issue #1690: death/delete of either party must break the oath immediately. The oath is wired + // to the central PlayerMobile/BaseCreature death+delete events instead of a polling timer. + [Fact] + public void PlayerDeletedEvent_BreaksOath() + { + var caster = new PlayerMobile(World.NewMobile); + caster.DefaultMobileInit(); + var target = new PlayerMobile(World.NewMobile); + target.DefaultMobileInit(); + + BloodOathSpell.RegisterOath(caster, target, TimeSpan.FromMinutes(5)); + + PlayerMobile.PlayerDeletedEvent(caster); // central handler breaks the oath from the caster side + + Assert.Null(BloodOathSpell.GetBloodOath(target)); + Assert.False(BloodOathSpell.RemoveCurse(target)); + + caster.Delete(); + target.Delete(); + } + + [Fact] + public void CreatureDeletedEvent_BreaksOath() + { + var caster = new PlayerMobile(World.NewMobile); + caster.DefaultMobileInit(); + var target = new TestCreature(World.NewMobile); + target.DefaultMobileInit(); + + BloodOathSpell.RegisterOath(caster, target, TimeSpan.FromMinutes(5)); + + BaseCreature.CreatureDeletedEvent(target); // central handler breaks the oath from the target side + + Assert.Null(BloodOathSpell.GetBloodOath(target)); + Assert.False(BloodOathSpell.RemoveCurse(caster)); + + caster.Delete(); + target.Delete(); + } + + private class TestCreature : BaseCreature + { + // Serial ctor skips AI/speed-table setup, which the test fixture does not configure. + public TestCreature(Serial serial) : base(serial) + { + } + } +} diff --git a/Projects/UOContent/Mobiles/BaseCreature.cs b/Projects/UOContent/Mobiles/BaseCreature.cs index 75f516afc..86f8e4848 100644 --- a/Projects/UOContent/Mobiles/BaseCreature.cs +++ b/Projects/UOContent/Mobiles/BaseCreature.cs @@ -1479,6 +1479,10 @@ namespace Server.Mobiles { var oldHits = Hits; + // Blood oath reflects the original damage the attacker dealt, before other modifiers. + var hasBloodOath = from != null && BloodOathSpell.GetBloodOath(from) == this; + var reflectedDamage = hasBloodOath ? amount : 0; + if (Core.AOS && !Summoned && Controlled && Utility.RandomDouble() < 0.2) { amount = (int)(amount * BonusPetDamageScalar); @@ -1489,14 +1493,24 @@ namespace Server.Mobiles amount = (int)(amount * 1.25); } - if (from != null && BloodOathSpell.GetBloodOath(from) == this) + if (hasBloodOath) { - amount = (int)(amount * 1.1); - from.Damage(amount, from); + amount = (int)(amount * 1.2); } base.Damage(amount, from, informMount); + // If the blood oath caster will die then damage is not reflected back to the attacker. + if (hasBloodOath && Alive && !Deleted && !IsDeadBondedPet) + { + // Reflect the original damage back to the attacker, attributed to the caster. + // The caster is a creature, so the Publish 48 (SA+) resist mitigation applies. + from.Damage( + BloodOathSpell.ComputeReflectedDamage(reflectedDamage, from.Skills.MagicResist.Value, Core.SA), + this + ); + } + if (SubdueBeforeTame && !Controlled && oldHits > HitsMax / 10 && Hits <= HitsMax / 10) { // * The creature has been beaten into subjugation! * diff --git a/Projects/UOContent/Mobiles/PlayerMobile.cs b/Projects/UOContent/Mobiles/PlayerMobile.cs index d122b7acc..1eb9f45c5 100644 --- a/Projects/UOContent/Mobiles/PlayerMobile.cs +++ b/Projects/UOContent/Mobiles/PlayerMobile.cs @@ -2824,13 +2824,10 @@ namespace Server.Mobiles // If the blood oath caster will die then damage is not reflected back to the attacker if (hasBloodOath && Alive && !Deleted && !IsDeadBondedPet) { - // In some expansions resisting spells reduces reflect dmg from monster blood oath - var resistReflectedDamage = !from.Player && Core.ML && !Core.HS - ? (from.Skills.MagicResist.Value * 0.5 + 10) / 100 - : 0; - - // Reflect damage to the attacker - from.Damage((int)(amount * (1.0 - resistReflectedDamage)), this); + // Reflect the attacker's original damage back to them, attributed to the caster. + // The caster is a player, so the Publish 48 resist mitigation does not apply + // (it only reduces reflected damage from creature casters). + from.Damage(BloodOathSpell.ComputeReflectedDamage(amount, 0, applyResistMitigation: false), this); } } diff --git a/Projects/UOContent/Spells/Necromancy/BloodOathSpell.cs b/Projects/UOContent/Spells/Necromancy/BloodOathSpell.cs index 847b3dbd7..c0f3c3ed0 100644 --- a/Projects/UOContent/Spells/Necromancy/BloodOathSpell.cs +++ b/Projects/UOContent/Spells/Necromancy/BloodOathSpell.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using ModernUO.CodeGeneratedEvents; using Server.Engines.BuffIcons; using Server.Mobiles; using Server.Targeting; @@ -16,7 +17,8 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell Reagent.DaemonBlood ); - private static readonly Dictionary _oathTable = new(); + // Keyed by BOTH participants (caster and target) -> shared timer, so the oath resolves and + // removes from either side. Required so the death/delete events can break it from either mobile. private static readonly Dictionary _table = new(); public BloodOathSpell(Mobile caster, Item scroll = null) : base(caster, scroll, _info) @@ -39,11 +41,11 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell { Caster.SendLocalizedMessage(1060508); // You can't curse that. } - else if (_oathTable.ContainsKey(Caster)) + else if (_table.ContainsKey(Caster)) { Caster.SendLocalizedMessage(1061607); // You are already bonded in a Blood Oath. } - else if (_oathTable.ContainsKey(m)) + else if (_table.ContainsKey(m)) { if (m.Player) { @@ -60,17 +62,12 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell /* Temporarily creates a dark pact between the caster and the target. * Any damage dealt by the target to the caster is increased, but the target receives the same amount of damage. - * The effect lasts for ((Spirit Speak skill level - target's Resist Magic skill level) / 80 ) + 8 seconds. + * The effect lasts for ((Spirit Speak skill level - target's Resist Magic skill level) / 8) + 8 seconds. * - * NOTE: The above algorithm must be fixed point, it should be: - * ((ss-rm)/8)+8 + * NOTE: The in-game tooltip (and UOGuide) display /80 due to a fixed-point bug. + * The actual OSI formula is /8, matching RunUO/ServUO. */ - RemoveCurse(m); - - _oathTable[Caster] = Caster; - _oathTable[m] = Caster; - m.Spell?.OnCasterHurt(); Caster.PlaySound(0x175); @@ -81,16 +78,11 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell m.FixedParticles(0x375A, 1, 17, 9919, 33, 7, EffectLayer.Waist); m.FixedParticles(0x3728, 1, 13, 9502, 33, 7, (EffectLayer)255); - var duration = TimeSpan.FromSeconds((GetDamageSkill(Caster) - GetResistSkill(m)) / 80 + 8); + var duration = TimeSpan.FromSeconds(GetDurationSeconds(GetDamageSkill(Caster), GetResistSkill(m))); m.CheckSkill(SkillName.MagicResist, 0.0, 120.0); // Skill check for gain - var timer = new ExpireTimer(Caster, m, duration); - timer.Start(); + RegisterOath(Caster, m, duration); - (Caster as PlayerMobile)?.AddBuff(new BuffInfo(BuffIcon.BloodOathCaster, 1075659, duration, m.Name)); - (m as PlayerMobile)?.AddBuff(new BuffInfo(BuffIcon.BloodOathCurse, 1075661, duration, Caster.Name)); - - _table[m] = timer; HarmfulSpell(m); } } @@ -100,25 +92,51 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell Caster.Target = new SpellTarget(this, TargetFlags.Harmful); } - public static bool RemoveCurse(Mobile target) + // ((Spirit Speak - Resisting Spells) / 8) + 8 seconds. + internal static double GetDurationSeconds(double damageSkill, double resistSkill) => + (damageSkill - resistSkill) / 8 + 8; + + // The attacker takes the original (un-bonused) damage reflected back. Publish 48 lets the + // attacker's Resisting Spells reduce the reflected damage, but only against creature casters. + internal static int ComputeReflectedDamage(int originalDamage, double attackerMagicResist, bool applyResistMitigation) { - if (!_table.Remove(target, out var timer)) + if (!applyResistMitigation) + { + return originalDamage; + } + + // ((Resisting Spells * 10) / 20) + 10 = percentage of damage resisted + var resisted = (attackerMagicResist * 0.5 + 10) / 100; + return (int)(originalDamage * (1.0 - resisted)); + } + + internal static void RegisterOath(Mobile caster, Mobile target, TimeSpan duration) + { + var timer = new ExpireTimer(caster, target, duration); + _table[caster] = timer; + _table[target] = timer; + timer.Start(); + + (caster as PlayerMobile)?.AddBuff(new BuffInfo(BuffIcon.BloodOathCaster, 1075659, duration, target.Name)); + (target as PlayerMobile)?.AddBuff(new BuffInfo(BuffIcon.BloodOathCurse, 1075661, duration, caster.Name)); + } + + public static bool RemoveCurse(Mobile m) + { + if (m == null || !_table.TryGetValue(m, out var timer)) { return false; } var caster = timer.Caster; - if (_oathTable.Remove(caster)) - { - caster.SendLocalizedMessage(1061620); // Your Blood Oath has been broken. - } - - if (_oathTable.Remove(target)) - { - target.SendLocalizedMessage(1061620); // Your Blood Oath has been broken. - } + var target = timer.Target; timer.Stop(); + _table.Remove(caster); + _table.Remove(target); + + caster.SendLocalizedMessage(1061620); // Your Blood Oath has been broken. + target.SendLocalizedMessage(1061620); // Your Blood Oath has been broken. (caster as PlayerMobile)?.RemoveBuff(BuffIcon.BloodOathCaster); (target as PlayerMobile)?.RemoveBuff(BuffIcon.BloodOathCurse); @@ -127,31 +145,29 @@ public class BloodOathSpell : NecromancerSpell, ITargetingSpell } public static Mobile GetBloodOath(Mobile m) => - m == null || _oathTable.TryGetValue(m, out var oath) && oath == m ? null : oath; + m != null && _table.TryGetValue(m, out var timer) && timer.Target == m ? timer.Caster : null; + + // Death or deletion of either participant breaks the oath immediately. RemoveCurse resolves the + // shared timer from either the caster or the target key, so a single call per mobile is enough. + [OnEvent(nameof(PlayerMobile.PlayerDeathEvent))] + [OnEvent(nameof(PlayerMobile.PlayerDeletedEvent))] + [OnEvent(nameof(BaseCreature.CreatureDeathEvent))] + [OnEvent(nameof(BaseCreature.CreatureDeletedEvent))] + public static void OnCurseEnds(Mobile m) => RemoveCurse(m); private class ExpireTimer : Timer { - private readonly Mobile _target; - private readonly DateTime _end; - public Mobile Caster { get; } + public Mobile Target { get; } - public ExpireTimer(Mobile caster, Mobile target, TimeSpan delay) : base( - TimeSpan.FromSeconds(1.0), - TimeSpan.FromSeconds(1.0) - ) + // Single-shot: fire once when the oath expires. Death or deletion of either party is + // handled separately by the OnCurseEnds event handler. + public ExpireTimer(Mobile caster, Mobile target, TimeSpan delay) : base(delay) { Caster = caster; - _target = target; - _end = Core.Now + delay; + Target = target; } - protected override void OnTick() - { - if (Caster.Deleted || _target.Deleted || !Caster.Alive || !_target.Alive || Core.Now >= _end) - { - RemoveCurse(_target); - } - } + protected override void OnTick() => RemoveCurse(Target); } }