Fix Imp Fire Shield owner targeting while grouped.

[mserajnik]

🍰 Pullrequest

Currently, PetAI::UpdateAllies() skips the owner (= the summoning Warlock in this case) in the grouped-member loop here:

core/src/game/AI/PetAI.cpp

Lines 382 to 383 in 09a9a84

	if (target->GetObjectGuid() == owner->GetObjectGuid())
	continue;

That would be fine if the owner were added unconditionally elsewhere. However, in the current code the owner is only inserted in the non-grouped branch here:

core/src/game/AI/PetAI.cpp

Lines 388 to 389 in 09a9a84

	else //remove group
	m_AllySet.insert(owner->GetObjectGuid());

That makes the Imp stop considering its summoning Warlock as a valid friendly autocast target while grouped.

In addition, the cached m_AllySet can also stay stale after leaving a 2-player party, because a 2-entry set can still be {pet, former party member} (since the owner was never added in the grouped branch) versus {pet, owner} after leaving the party, and the size-based m_AllySet.size() == 2 && !group check will incorrectly still pass.

This change always inserts the owner before processing grouped allies. This fixes both the grouped case and the stale cache after leaving a 2-player party, because even with a 2-player party the size of m_AllySet will be 3 ({pet, owner, party member}) and thus not hit the early return anymore.

Note that I could not reproduce the GM visibility issue around Fire Shield that I first reported in #2916 anymore, so I probably initially misinterpreted it while simultaneously hitting this party bug.

Proof

• None

Issues

• Fixes 🐞 [Bug] Imp doesn't seem to cast Fire Shield on Warlock that summoned him #2916

How2Test

• Create a Warlock character using a GM account
• .levelup 13 (need to be level 14 to learn Fire Shield)
• .learn all_myclass
• .additem 16326 1
• Cast Summon Imp
• Learn Fire Shield by right-click the Grimoire of Fire Shield (Rank 1) in the inventory
• Enable Fire Shield autocasting by right clicking on it in the pet action bar
• .partybot add priest
• Keep the party bot out of combat and attack a mob
• Observe that the Imp will still cast Fire Shield on you, despite being in a party (this is not the case without this fix)
• Leave the party, wait at least 10 seconds, retest without relogging, and confirm you can still receive Fire Shield (this also does not work without relogging without this fix)

Todo / Checklist

• Fire Shield currently also autocasts when not attacked in melee (as soon as you get aggro) because its conditional in Spell::CanAutoCast() uses GetAttackerForHelper(). I am not entirely sure, but I think this is not fully blizz-like. I didn't touch this in this PR though.

[ratkosrb]

If the owner is charmed it will not be a friendly target.

[mserajnik]

Good point, I forgot about that.

This makes the set size checks questionable because a lot can change within 10 seconds in regards to party composition and party members getting charmed.

I will have a look at this later and try to come up with a good solution.

[mserajnik]

Had a few minutes and checked how CMaNGOS does it and it looks like they also add the owner unconditionally: https://github.com/cmangos/mangos-classic/blob/db3c5cba390b8f02c709675780bc086dc88fec37/src/game/AI/BaseAI/PetAI.cpp#L429

But maybe they check if the potential ally is charmed later; will investigate once I am home, if they have a good solution it may be worth it aligning our handling here.

[mserajnik]

I looked into this:

Unless I misunderstood something when reading the code, with this PR we are essentially matching current CMaNGOS behavior: the owner is added unconditionally to m_AllySet and a charmed owner or party members are not filtered there. Instead, the charm state is validated later at cast time. I added a comment in UpdateAllies() to make that explicit.

In VMaNGOS, friendly-target pet casts go through Spell::CheckPetCast(), which rejects hostile targets via IsHostileTo(), so a charmed target is filtered there rather than when adding them to m_AllySet. CMaNGOS handles this in the same way conceptually as well, but through CanAssistSpell() / CanAssist() instead.

If we wanted to, we could additionally rebuild m_AllySet earlier when a charmed party member is detected and explicitly exclude charmed targets from the set, but I am not sure if that is worth adding here; since m_AllySet is only rebuilt every 10 seconds, that would mostly just invert the stale cache problem: a temporarily charmed target would stay excluded until the next set rebuild even after the charm ends. And since in that time frame also other charms can happen, we can not blindly rely on the set being correct anyway.

Unrelated, but I also noticed that CMaNGOS likely has a bug in the GetMembersCount() + 2 check, because GetMembersCount() already includes the owner there as well, just like in VMaNGOS (so it should probably be + 1 instead there as well).