fix: last projectile spell casting #2174
Conversation
This commit addresses a bug where a spell with ammunition requirements would consume the last projectile without actually casting it out. It is also intended to provide additional chore changes and move player related code out to it's own overrides. Files changed: 1. Entity.cs - Moved projectile removal to player's CastSpell override method. Also moved the player's specific handlers for spell's cooldown to player's CastSpell override method. 2. Player.cs - Removed redundant and repeated code within the UseSpell method (ie. CastSpell(..) is already called within Entity's Update method when required) and added projectile removal and cooldown handlers within the CastSpell override method.
| //Check if the caster has the right ammunition if a projectile | ||
| if (spell.SpellType == SpellType.CombatSpell && | ||
| spell.Combat.TargetType == SpellTargetType.Projectile && | ||
| spell.Combat.ProjectileId != Guid.Empty) | ||
| { | ||
| var projectileBase = spell.Combat.Projectile; | ||
| if (projectileBase != null && projectileBase.AmmoItemId != Guid.Empty) | ||
| { | ||
| TryTakeItem(projectileBase.AmmoItemId, projectileBase.AmmoRequired); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved this out to the override void CastSpell because this was removing projectiles before we could even cast them out properly, which leads to the actual bug of not being able to properly use the last projectile
| // Player is not casting a spell, cast now! | ||
| CastTime = 0; | ||
| CastSpell(Spells[SpellCastSlot].SpellId, SpellCastSlot); | ||
| CastTarget = null; | ||
| } | ||
| else | ||
| { | ||
| //Tell the client we are channeling the spell |
There was a problem hiding this comment.
Explanation for removal: duplicated logic, we were already doing this here:
Intersect-Engine/Intersect.Server.Core/Entities/Entity.cs
Lines 314 to 320 in 116dd44
| // Player cooldown handling is done elsewhere! | ||
| if (this is Player player) | ||
| { | ||
| player.UpdateCooldown(spellBase); | ||
|
|
||
| // Trigger the global cooldown, if we're allowed to. | ||
| if (!spellBase.IgnoreGlobalCooldown) | ||
| { | ||
| player.UpdateGlobalCooldown(); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| SpellCooldowns[Spells[spellSlot].SpellId] = | ||
| Timing.Global.MillisecondsUtc + spellBase.CooldownDuration; | ||
| } |
There was a problem hiding this comment.
Players have their own CastSpell override method so i moved these over
There was a problem hiding this comment.
To get rid of any type-checking (there's still a this is not Player in the changes), you should instead figure out what this logic is doing, make it a virtual Entity method, and create an overrided version in Player. Then, replace this chunk of code with a call to that method, and let inheritance take over.
| // Player cooldown handling is done elsewhere! | ||
| if (this is Player player) | ||
| { | ||
| player.UpdateCooldown(spellBase); | ||
|
|
||
| // Trigger the global cooldown, if we're allowed to. | ||
| if (!spellBase.IgnoreGlobalCooldown) | ||
| { | ||
| player.UpdateGlobalCooldown(); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| SpellCooldowns[Spells[spellSlot].SpellId] = | ||
| Timing.Global.MillisecondsUtc + spellBase.CooldownDuration; | ||
| } |
There was a problem hiding this comment.
To get rid of any type-checking (there's still a this is not Player in the changes), you should instead figure out what this logic is doing, make it a virtual Entity method, and create an overrided version in Player. Then, replace this chunk of code with a call to that method, and let inheritance take over.
As suggested by review, applies polymorphism to handle the cooldown update. We now have a virtual method in the Entity class that is overridden in the Player class. This way, we avoid type checking and let the correct method be called based on the object's type.
refactors the player logic to consume spell projectiles as the ConsumeSpellProjectile method
|
just to make sure, some extra testing with NPCs, all good: 2024-03-11-23-15-02.mp4 |
Restored quick casting functionality for spell items, resolving my mess from PR #2174. Also ensured last projectile removal for quick casted spells is working as expected.
Restored quick casting functionality for spell items, resolving my mess from PR #2174. Also ensured last projectile removal for quick casted spells is working as expected.
Fixes #1683
Which reported a bug where a spell with ammunition requirements would consume the last projectile without actually casting it out. It is also intended to provide additional chore changes and move player related code out to it's corresponding overrides.
Files changed:
Bug:
2022-12-11.12-21-06.mp4
After the fix:
2024-03-09.19-08-42.mp4