Fix the inability to multicast impen/bane/brittlemail/lures on other characters

[mcreedjr]

This PR resolves the problem where players are unable to cast beneficial item spells on others due to the error: "You fail to affect <> with <>"

I believe this functionality was permitted in retail, and was permitted in ACE up until a recent PR. If I am mis-remembering and this was not permitted in retail, then I will withdraw my PR.

[ghost]

I believe this fix is incomplete. When a beneficial item spell, such as a impen or bane, is targeted on another player, it should be distributed internally to all items that the target player is wearing that are enchantable but also deduct an appropriate amount of mana, as though having cast the spell individually on each equipped piece. Just as a player targeting themselves distributes the cast item spell on all equipped items that are enchantable, yet deducts an appropriate amount of mana for all items enchanted.

[mcreedjr]

I think this PR accomplishes the first part of your statement - it applies the spell to all equipped items that are enchantable. I tested it before issuing the pull request and it functioned as expected.

The second bit about deducting an amount of mana as if the spell was cast on each piece individually... Is that only when applying item spells to other characters? The code just above it for applying item spells to self-equipped items does not deduct mana for each item the spell was cast upon, either.

[ghost]

If multiple sums of mana aren't being deducted for even self-targeted impen/banes, then that is a bug, also. When an impen or bane is cast on oneself or another player, regardless, the spell should get distributed to all clothing items equipped, sans a possible equipped shield as that should not be part of the bundled spell cast, and mana taken from the caster should be equivalent to that of having cast the spell individually on all items. ACE was functioning this way at one point.

[mcreedjr]

Interesting. I don't remember self targeted item spells cast on a character in retail deducting multiple sums of mana. Do you happen to have a reference to this on the Wiki or something similar?

I don't believe this is currently implemented in either case. I believe this is the only place where awareness could be raised that an item spell is targeting multiple pieces, so I don't believe it is implemented elsewhere.

[ghost]

It was working that way at one point.

[ghost]

Casting impen on all equipped items from one cast without removing an equivalent amount of mana would be a cheat. You would in a sense be getting free spell casts.

[ghost]

I remember it was that way in the past, as I coded it that way...

[ghost]

Never mind. CalculateManaUsage() is still totaling the sum of the mana for an equivalent multiple spell casting, when a player is targeted for an impen/bane type item spell cast. "numTargetItems = targetPlayer.EquippedObjects.Values.Count(i => (i is Clothing || i.IsShield) && i.IsEnchantable);" is the line from Creature.CalculateManaUsage(). You are correct in that the Clothing check is missing. However, it should be able to be shortened from i.WeenieType == WeenieType.Clothing to 'i is Clothing'.

[mcreedjr]

You beat me to it. I had just come to the same conclusion. If I shorten the weenie type definition as suggested will that resolve your concerns with his PR? Thank you for taking the time to review and comment.

[ghost]

Yes.

Code gets moved around a lot, and it can seemingly get lost in the shuffle. "It isn't were I left it, so it must be gone"...

[ghost]

Game-play tested proposed fix works, so should be good to go when Clothing check is changed to 'i is Clothing'

[mcreedjr]

Great. Thank you again for the review and play testing. I'll revise the PR later today as I've stepped away from a PC for a bit.

[gmriggs]

"it should be distributed internally to all items that the target player is wearing that are enchantable"

This is incorrect. There has been a ton of discussion about this in the ACE development and help channels, so please see there for reference.

The only spell that would ever go to multiple targets is a self-impen/bane. Casting item spells with other players or creatures as targets would never go to more than 1 item.

If I cast Blood Loather with another creature or player as the target, it applies to their primary weapon. If they aren't wielding a weapon in their main hand, then the spell fails to apply.

Likewise, if I try to cast impen/bane or brittlemail/lure on another creature target, it tries to redirect to their equipped shield. If it can't find this valid item to redirect to, then it gets the 'fail to affect' message after the cast (not before, like many of the other errors)

In addition, impen/bane on another creature target was massively broken before, and is already working as described if you test in-game, without any additional code changes needed. Before this PR, I could:

• cast impen/bane on another player target, buffing all of their armor (incorrect, you could not do this in retail)
• even worse, when this operation was done in ace before this pr, it was only using mana for 1 item, instead of + (ManaMod * NumTargetItems)
  ManaMod for brittlemail/lures is 0, so the data would indicate here that brittlemail/lures could never be applied to more than 1 item at a time, so this makes sense why brittlemail/lure with another player as target before this PR was a) even working and applying to all of their armor, and b) only using the equivalent mana for 1 item. but i'm not sure why impen/bane with other players as target before this PR was only using mana for 1 item instead of all of the equipped items, but it was...

[gmriggs]

incorrect. original was correct

[gmriggs]

incorrect, see above, and detailed explanation in comment

[mcreedjr]

I'm sorry. I do not regularly participate in those development discord channels. However, it is apparent I should do so if I intend to continue contributing. I will be closing this PR. Thank you to all who commented.