From 4e57ec237379035edcdbcdff36bc20d671d74142 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:41:27 -0700 Subject: [PATCH] fix: monster ability queries no longer have side effects (#2716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `GetMonsterAbilities()` answers three questions through one virtual: which abilities a creature has (`GetAbility`, `AOS.cs` reflect/barrier, bard fire-breath difficulty), which abilities are enabled right now, and where cooldown state must be cleaned up (`InvalidateNextAbilityTriggers` on death and delete). Every caller assumes it is a pure, stable list. About 70 creatures return a static array; only LordOaks computed it: ``` Mobile.Delete() → BaseCreature.OnDelete() → CreatureDeletedEvent → MonsterAbility.InvalidateNextAbilityTriggers() → LordOaks.GetMonsterAbilities() → CheckQueen() → new Silvani().MoveToWorld(...) ``` - Deleting a Lord Oaks nobody had fought (champion reset, `DeleteCreatures`, GM delete) spawned an orphaned Silvani where he stood. - The spell damage scalar (`AlterDamageScalarFrom`) spawned her too. - With the queen dead his list was empty, so cleanup skipped his cooldown entries. The static `Dictionary` on the ability singleton kept a reference to the deleted creature. `MonsterAbilityGroup` children's entries were never cleaned either. - Neira threw unholy bones from `AlterDamageScalarFrom`, so any caller asking for a damage scalar (a preview gump, for example) could fire the effect. ## Changes - **Per-creature cooldowns:** a lazy `Dictionary` on `BaseCreature` (`IsAbilityOnCooldown`, `SetAbilityCooldown`, `ClearAbilityCooldowns`). The static dictionaries and the query-driven cleanup are gone, and death still resets cooldowns. Tick comparisons are wraparound-safe. - **Pure contract:** `GetMonsterAbilities()` is documented as pure. The new `virtual bool CanUseAbility(MonsterAbility)` gates abilities that only work some of the time. Every `TriggerAbility*` loop and `GetAbility` check it. - **LordOaks:** always lists his abilities; `CanUseAbility` and the 10% spell shield read "queen alive". The queen spawns when he is first aggressed or first given a combatant, never on the internal map. Aggression lands before the damage it causes, so the first spell is still shielded, as before. - **Neira:** the unholy bone throw moves to `OnDamagedBySpell`. A follow-up PR makes minions such as the queen, pixies and Barracoon's rats belong to their champion spawn, so a spawn reset cleans them up. ## Testing - `LordOaksTests` covers three cases: - querying or deleting an unengaged Lord Oaks spawns nothing; - aggression spawns the queen exactly once, and the shield and abilities follow her life; - an Oaks on the internal map never spawns her. - UOContent.Tests: 1234 passed, 2 skipped. - A local constructor sweep (every Mobile built and deleted repeatedly) no longer reports LordOaks. --- .../Tests/Mobiles/LordOaksTests.cs | 98 +++++++++++++++++++ .../Mobiles/Abilities/MonsterAbility.cs | 36 +------ Projects/UOContent/Mobiles/BaseCreature.cs | 52 +++++++++- .../UOContent/Mobiles/Special/LordOaks.cs | 51 ++++++---- Projects/UOContent/Mobiles/Special/Neira.cs | 6 +- 5 files changed, 182 insertions(+), 61 deletions(-) create mode 100644 Projects/UOContent.Tests/Tests/Mobiles/LordOaksTests.cs diff --git a/Projects/UOContent.Tests/Tests/Mobiles/LordOaksTests.cs b/Projects/UOContent.Tests/Tests/Mobiles/LordOaksTests.cs new file mode 100644 index 000000000..dae82a332 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Mobiles/LordOaksTests.cs @@ -0,0 +1,98 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Server; +using Server.Mobiles; +using Xunit; + +namespace UOContent.Tests.Mobiles; + +// Lord Oaks' abilities and spell shield depend on his queen, so the queen must spawn from combat, +// never from a query such as GetMonsterAbilities or a damage calculation. +[Collection("Sequential UOContent Tests")] +public class LordOaksTests : IDisposable +{ + private readonly List _created = []; + + public void Dispose() + { + foreach (var m in World.Mobiles.Values.OfType().ToList()) + { + m.Delete(); + } + + for (var i = 0; i < _created.Count; i++) + { + _created[i].Delete(); + } + } + + private LordOaks NewOaks(Map map) + { + var oaks = new LordOaks(); + _created.Add(oaks); + oaks.MoveToWorld(new Point3D(1000, 1000, 0), map); + return oaks; + } + + private Mobile NewAttacker() + { + var attacker = new Mongbat(); + _created.Add(attacker); + return attacker; + } + + private static int SilvaniCount() => World.Mobiles.Values.Count(m => m is Silvani); + + [Fact] + public void QueryingOrDeletingAnUnengagedOaks_SpawnsNoQueen() + { + var oaks = NewOaks(Map.Felucca); + + var scalar = 1.0; + oaks.AlterDamageScalarFrom(null, ref scalar); + oaks.GetMonsterAbilities(); + oaks.Delete(); + + Assert.Equal(1.0, scalar); + Assert.Equal(0, SilvaniCount()); + } + + [Fact] + public void Aggression_SpawnsTheQueenOnce_AndShieldsOaksWhileSheLives() + { + var oaks = NewOaks(Map.Felucca); + var attacker = NewAttacker(); + + oaks.AggressiveAction(attacker, false); + oaks.AggressiveAction(attacker, false); + + var queen = World.Mobiles.Values.OfType().Single(); + + var shielded = 1.0; + oaks.AlterDamageScalarFrom(attacker, ref shielded); + Assert.Equal(0.1, shielded, 6); + Assert.True(oaks.CanUseAbility(MonsterAbilities.SummonPixiesCounter)); + + queen.Delete(); + + var exposed = 1.0; + oaks.AlterDamageScalarFrom(attacker, ref exposed); + oaks.AggressiveAction(attacker, false); + + Assert.Equal(1.0, exposed); + Assert.False(oaks.CanUseAbility(MonsterAbilities.SummonPixiesCounter)); + Assert.Equal(0, SilvaniCount()); + } + + [Fact] + public void AnOaksOnTheInternalMap_NeverSpawnsTheQueen() + { + var oaks = new LordOaks(); + _created.Add(oaks); + + oaks.AggressiveAction(NewAttacker(), false); + + Assert.Equal(0, SilvaniCount()); + } +} diff --git a/Projects/UOContent/Mobiles/Abilities/MonsterAbility.cs b/Projects/UOContent/Mobiles/Abilities/MonsterAbility.cs index 75c6fc3dc..0fcbae30c 100644 --- a/Projects/UOContent/Mobiles/Abilities/MonsterAbility.cs +++ b/Projects/UOContent/Mobiles/Abilities/MonsterAbility.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using ModernUO.CodeGeneratedEvents; namespace Server.Mobiles; @@ -9,8 +8,6 @@ namespace Server.Mobiles; /// public abstract class MonsterAbility { - private Dictionary _nextTriggerTicks; - public virtual MonsterAbilityType AbilityType => MonsterAbilityType.Generic; public virtual MonsterAbilityTrigger AbilityTrigger => MonsterAbilityTrigger.None; @@ -30,18 +27,9 @@ public abstract class MonsterAbility return false; } - if (_nextTriggerTicks?.TryGetValue(source, out var nextTrigger) == true) + if (source.IsAbilityOnCooldown(this)) { - if (nextTrigger - Core.TickCount > 0) - { - return false; - } - - _nextTriggerTicks.Remove(source); - if (_nextTriggerTicks.Count == 0) - { - _nextTriggerTicks = null; - } + return false; } var c = ChanceToTrigger; @@ -65,10 +53,7 @@ public abstract class MonsterAbility ? MinTriggerCooldown : Utility.RandomMinMax(MinTriggerCooldown, MaxTriggerCooldown); - var nextTrigger = Core.TickCount + (long)randomAmount.TotalMilliseconds; - - _nextTriggerTicks ??= new Dictionary(); - _nextTriggerTicks[source] = nextTrigger; + source.SetAbilityCooldown(this, randomAmount); } public virtual void AlterMeleeDamageFrom(BaseCreature source, Mobile target, ref int damage) @@ -100,18 +85,5 @@ public abstract class MonsterAbility } [OnEvent(nameof(CreatureEvents.CreatureDeathEvent))] - [OnEvent(nameof(CreatureEvents.CreatureDeletedEvent))] - public static void InvalidateNextAbilityTriggers(BaseCreature source) - { - var abilities = source.GetMonsterAbilities(); - if (abilities == null || abilities.Length == 0) - { - return; - } - - for (var i = 0; i < abilities.Length; i++) - { - abilities[i]._nextTriggerTicks?.Remove(source); - } - } + public static void ResetCooldownsOnDeath(BaseCreature source) => source.ClearAbilityCooldowns(); } diff --git a/Projects/UOContent/Mobiles/BaseCreature.cs b/Projects/UOContent/Mobiles/BaseCreature.cs index af619e005..d799013a5 100644 --- a/Projects/UOContent/Mobiles/BaseCreature.cs +++ b/Projects/UOContent/Mobiles/BaseCreature.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Runtime.CompilerServices; using ModernUO.Serialization; @@ -1500,10 +1500,47 @@ namespace Server.Mobiles public List MLQuests => (m_MLQuests ??= StaticMLQuester ? MLQuestSystem.FindQuestList(GetType()) : ConstructQuestList()) ?? MLQuestSystem.EmptyList; + /// + /// The abilities this creature has. Must be a pure, stable query: it is called from lookups, damage + /// calculations and cleanup, so it must not spawn, mutate state, or change with the creature's state. + /// Gate abilities that should only work some of the time with . + /// public virtual MonsterAbility[] GetMonsterAbilities() => null; + /// + /// Whether may trigger or be looked up right now. Must be a pure read. + /// + public virtual bool CanUseAbility(MonsterAbility ability) => true; + private MonsterAbilityTrigger _activeTriggers; + // Deadlines in Core.TickCount, keyed by ability. Lives on the creature so it is collected with it. + private Dictionary _abilityCooldowns; + + public bool IsAbilityOnCooldown(MonsterAbility ability) + { + if (_abilityCooldowns?.TryGetValue(ability, out var nextTrigger) != true) + { + return false; + } + + if (nextTrigger - Core.TickCount > 0) + { + return true; + } + + _abilityCooldowns.Remove(ability); + return false; + } + + public void SetAbilityCooldown(MonsterAbility ability, TimeSpan cooldown) + { + _abilityCooldowns ??= new Dictionary(); + _abilityCooldowns[ability] = Core.TickCount + (long)cooldown.TotalMilliseconds; + } + + public void ClearAbilityCooldowns() => _abilityCooldowns = null; + public virtual MonsterAbility GetAbility(MonsterAbilityType type) { var abilities = GetMonsterAbilities(); @@ -1516,6 +1553,11 @@ namespace Server.Mobiles for (var i = 0; i < abilities.Length; i++) { var ability = abilities[i]; + if (!CanUseAbility(ability)) + { + continue; + } + if (ability is MonsterAbilityGroup group) { ability = group.GetAbilityWithType(type); @@ -1579,7 +1621,7 @@ namespace Server.Mobiles for (var i = 0; i < abilities.Length; i++) { var ability = abilities[i]; - if (ability.CanTrigger(this, trigger)) + if (CanUseAbility(ability) && ability.CanTrigger(this, trigger)) { ability.Trigger(trigger, this, defender); triggered = true; @@ -1606,7 +1648,7 @@ namespace Server.Mobiles for (var i = 0; i < abilities.Length; i++) { var ability = abilities[i]; - if (ability.CanTrigger(this, trigger)) + if (CanUseAbility(ability) && ability.CanTrigger(this, trigger)) { ability.Move(this, d); } @@ -1625,7 +1667,7 @@ namespace Server.Mobiles for (var i = 0; i < abilities.Length; i++) { var ability = abilities[i]; - if (ability.CanTrigger(this, trigger)) + if (CanUseAbility(ability) && ability.CanTrigger(this, trigger)) { if ((trigger & MonsterAbilityTrigger.GiveMeleeDamage) != 0) { @@ -1666,7 +1708,7 @@ namespace Server.Mobiles for (var i = 0; i < abilities.Length; i++) { var ability = abilities[i]; - if (ability.CanTrigger(this, trigger)) + if (CanUseAbility(ability) && ability.CanTrigger(this, trigger)) { if ((trigger & MonsterAbilityTrigger.GiveSpellDamage) != 0) { diff --git a/Projects/UOContent/Mobiles/Special/LordOaks.cs b/Projects/UOContent/Mobiles/Special/LordOaks.cs index f8fb6d444..28577c90b 100644 --- a/Projects/UOContent/Mobiles/Special/LordOaks.cs +++ b/Projects/UOContent/Mobiles/Special/LordOaks.cs @@ -86,7 +86,11 @@ public partial class LordOaks : BaseChampion public override Poison PoisonImmune => Poison.Deadly; private static MonsterAbility[] _abilities = { MonsterAbilities.SummonPixiesCounter }; - public override MonsterAbility[] GetMonsterAbilities() => CheckQueen() ? _abilities : MonsterAbilities.Empty; + public override MonsterAbility[] GetMonsterAbilities() => _abilities; + + public override bool CanUseAbility(MonsterAbility ability) => IsQueenAlive; + + private bool IsQueenAlive => _queen?.Deleted == false; public override void GenerateLoot() { @@ -103,36 +107,41 @@ public partial class LordOaks : BaseChampion public override int GetDeathSound() => 0x2F7; - public bool CheckQueen() + // Aggression lands before the damage it causes, so the queen is up before the first hit is scaled. + public override void AggressiveAction(Mobile aggressor, bool criminal) { - if (Map == null) + base.AggressiveAction(aggressor, criminal); + SpawnQueen(); + } + + public override void OnCombatantChange() + { + base.OnCombatantChange(); + + if (Combatant != null) { - return false; + SpawnQueen(); + } + } + + private void SpawnQueen() + { + if (_spawnedQueen || Deleted || Map == null || Map == Map.Internal) + { + return; } - if (!_spawnedQueen) - { - Say(1042153); // Come forth my queen! + Say(1042153); // Come forth my queen! - _queen = new Silvani { Team = Team }; - _queen.MoveToWorld(Location, Map); + Queen = new Silvani { Team = Team }; + Queen.MoveToWorld(Location, Map); - _spawnedQueen = true; - return true; - } - - if (_queen?.Deleted != false) - { - _queen = null; - return false; - } - - return true; + SpawnedQueen = true; } public override void AlterDamageScalarFrom(Mobile caster, ref double scalar) { - if (CheckQueen()) + if (IsQueenAlive) { scalar *= 0.1; } diff --git a/Projects/UOContent/Mobiles/Special/Neira.cs b/Projects/UOContent/Mobiles/Special/Neira.cs index 4cbcbb5c9..b55e20d25 100644 --- a/Projects/UOContent/Mobiles/Special/Neira.cs +++ b/Projects/UOContent/Mobiles/Special/Neira.cs @@ -163,13 +163,13 @@ public partial class Neira : BaseChampion } } - public override void AlterDamageScalarFrom(Mobile caster, ref double scalar) + public override void OnDamagedBySpell(Mobile from, int damage) { - base.AlterDamageScalarFrom(caster, ref scalar); + base.OnDamagedBySpell(from, damage); if (Utility.RandomDouble() < 0.1) // 10% chance to throw an unholy bone { - AddUnholyBone(caster, 1.0); + AddUnholyBone(from, 1.0); } }