Core/SmartAI: AI change on charmed

[engilas]

Changes proposed:

• SmartAI calls base method UnitAI::OnCharmed to update AI on charmed

Without this change, charmed creature can't update AI.

Target branch(es): 3.3.5/master

• 3.3.5
• master

Issues addressed:

Closes #25982 (melee attack works correctly)
Partially #23582 (part 2: Enslaved demons should be able to attcak)

Tests performed:

Built and tested with warlock's 'Enslave Demon' spell, priest's 'Mind control' spell.

[jackpoz]

one thing that I wonder is if for some reason SmartAI should still stay even when charmed.
@offl do you see any issue with this PR ? is it ok if a SmartAI creature loses the SmartAI while being charmed and then regains it when the charm goes away ?

[offl]

is it ok if a SmartAI creature loses the SmartAI while being charmed and then regains it when the charm goes away ?

We have SMART_EVENT_FLAG_WHILE_CHARMED (Event can occur while player controlled)
Means we want SmartAI being active even if creature is charmed

And since people used that flag in every script for years without any reason and that flag was added by mass update without any reason, we don't have a clear list of creatures which indeed should have that flag, but that's a different story

[engilas]

It's too many logic in PetAI for commands, I think I can close this PR.

[jackpoz]

we can leave it open for a while as a why to discuss about the issue. it makes no sense to me that every single SAI creature doesn't change AI when charmed but it might take some time before we can merge this

[jackpoz]

@engilas we came up with a compromise: SmartAI::OnCharmed() should schedule an AI change if there are no events with SMART_EVENT_FLAG_WHILE_CHARMED flag (since there are quests that require SAI to trigger some script stuff when charmed).

This should fix the issue in most of the cases, except for some cases where we still need SAI.

I don't know if there is some sort of "IsThereAnyEventWithFlag(flag)" that we can use or if we need to implement it, is that something you would like to work on or should I assign it to myself ?

[engilas]

@jackpoz Yes, I can try to implement it

[engilas]

@jackpoz added check for flag SMART_EVENT_FLAG_WHILE_CHARMED

[ccrs]

SmartAI cant go, must stay, this way smart scripts can keep control on what happens.

The problem is that, as discussed here and a couple years ago, there is no implementation for PetAI-like features, like commands and such. I remember rising this myself. Nothing was finally done, left as is.

[ccrs]

@engilas we came up with a compromise: SmartAI::OnCharmed() should schedule an AI change if there are no events with SMART_EVENT_FLAG_WHILE_CHARMED flag (since there are quests that require SAI to trigger some script stuff when charmed).

This should fix the issue in most of the cases, except for some cases where we still need SAI.

I don't know if there is some sort of "IsThereAnyEventWithFlag(flag)" that we can use or if we need to implement it, is that something you would like to work on or should I assign it to myself ?

Aaaaand I didnt read that, sorry. Feels ok I guess.

[BAndysc]

@jackpoz @engilas

SmartAI::OnCharmed() should schedule an AI change if there are no events with SMART_EVENT_FLAG_WHILE_CHARMED flag (since there are quests that require SAI to trigger some script stuff when charmed).

IMHO this introduces hard to follow behaviour when writing script. A small flag in one single event can change the behaviour of the whole script! I do not like it. I can imagine someone copies an event from another script which has that flag and unintentionally changes the behaviour when charmed (even if event seems unrelated). Or the other way around: someone fix the script by changing the events and unintentionally removes the event with the flag, because it is hard to spot.

Wouldn't it be better to add action SMART_ACTION_USE_SMART_AI_WHEN_CHARMED that you would use in AIIntialize method? Thanks to this, there is no way someone will miss the fact the script has such behaviour.

And you can automatically at such action to all scripts where at least one event has the flag to mimic the behaviour. But in the future scripts, it will be clear user choice to decide

[offl]

Personally I treat it as temp solution because it's indeed not really what we need simply because we are checking if creature has event with CHARMED flag but it's not correct in first place because that flag is linked to action id and not to creature entry. And there should be no such flag linked to creature entry because we already know some of those charmed creatures with SAI should attack in melee and their scripts should not be disabled if charmed.

SAI just should not break Charmed AI logic but since it does and breaks hundreds and thousands creatures, that solution is better than nothing.

[BAndysc]

Still, if we want to temporary hack, then imho it is better to do it explicitly (new action) than implicitly (with event flag).

[offl]

What is the actual problem with making creature attack in melee and use SAI if charmed? Too hard to implement?
The thing is even if we remove SMART_EVENT_FLAG_WHILE_CHARMED from nearly all creatures(I'll remove it anyway today with or withot this PR) and fix melee attack for charmed SAI creatures, some of them will be broken because not all of them should attack in melee. However some of them will be fixed. The broken ones should be fixed manually by adding passive reactstate or their melee attack should be disabled, depends on case

This is not about flags and only about conflicts between SAI logic and Charmed logic

[ccrs]

I agree with @offl , this temporal solution feels the most suitable

• The flag is already present and nothing needs to be changed.
• When it was introduced additional work was done in form of update scripts and manual updates to adapt all existing SAI scripts
• Code modification is minimal

a new event is overkill imo

also the final goal is to introduce PetAI implementations into SAI, lets not forget that, just as @offl pointed out

[jackpoz]

I wonder if we should reset mAllEventFlags before this loop in case SAI table is reloaded at runtime

[engilas]

We don't update instance of SmartScript, instead we create new instances

TrinityCore/src/server/game/AI/SmartScripts/SmartScriptMgr.cpp

Lines 335 to 343 in bf0975f

	// creature entry / guid not found in storage, create empty event list for it and increase counters
	if (mEventMap[source_type].find(temp.entryOrGuid) == mEventMap[source_type].end())
	{
	++count;
	SmartAIEventList eventList;
	mEventMap[source_type][temp.entryOrGuid] = eventList;
	}
	// store the new event
	mEventMap[source_type][temp.entryOrGuid].push_back(temp);

I think the creatures still have the old version of SAI after reload

[jackpoz]

I honestly don't want to re-implement PetAI (and PossessedAI) in SmartAI, have 2 different implementation for the same logic.

Charming a creature is not just about "melee attacks", there's a lot more logic into it, which is why there are AI classes that do only that.

This PR is supposed to be a compromise for the time being until someone feels like untangling this mess that SmartAI is based on, with "let's reimplement everything in SmartAI implementation ignoring everything that CreatureAI already handles". I've seen this already with starting/pausing/stopping a waypoint path.

An alternative could be have SmartAI instantiate PossessedAI/PetAI and call its hooks and process the SMART_EVENT_FLAG_WHILE_CHARMED events, but it's just an idea. I'm not even sure if the scripters should be able to override PossessedAI/PetAI actions but that's another story.

[ccrs]

we're on the same page, I agree

[ghost]

Nice.

[jackpoz]

Thanks for the PR :)