Combat system allows entering in combat with target, but threat manager rejects it

[joshwhedon]

Description:

This one will be a bit difficult to explain so bear with me. Often, to initiate combat between two creatures in script, we use the AttackStart method. However, it seems that in some cases, AttackStart allows starting the combat, but the threat manager sets the target as offline due to invalid data (in this case, factions which cannot attack each other). This results in a creature chasing an other, but never actually attacking it.
I think that this faction / valid attack target check should happen before the threat manager does it, because we should never allow to start attacking a target we cannot really fight.

Current behaviour:

NPC chase each other endlessly without actually fighting each other.

Expected behaviour:

NPC should either not chase each other, or actually fight each other.

Steps to reproduce the problem:

Do .go creature id 26170

Take the quest Last Rites

Start the event

Notice Thassarian chasses Valanar but never really attacks

https://imgur.com/a/eQ9C8S9

Branch(es):

3.3.5

TC rev. hash/commit:

9407f9b

Operating system:
Linux and Windows, does not matter

[jackpoz]

There are some helper methods that can/should be used when trying to attack something:

• WorldObject::IsValidAttackTarget()
• Creature::CanCreatureAttack()

Not sure which one is the standard TC one, it seems WorldObject::IsValidAttackTarget() is used by quite a few scripts, especially in MoveInLineOfSight() hook

[Jildor]

In the example case about quest Last Rites, npc thassarian is forced to attack valanaar:
https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/scripts/Northrend/zone_borean_tundra.cpp#L867-L871

thassarian do attackstart, and executes:
https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Entities/Unit/Unit.cpp#L5470
and seems unit:attack doesn't check if faction is valid or something like this so thassarian do EngageWithTarget and set offlinethreat or something
so, thassarian faction is 1892 and valanaar faction is 14, isn't valid for attack and can't do DoMeleeAttackifReady.

[joshwhedon]

Exactly Jildor. To me, Unit::Attack is missing checks, because it is meant to return true or false when the attack is valid. Now, it returns true in some cases where the attack is not valid. So I guess some checks were moved around.

[Rushor]

@Jildor do you know another example in scripts where the issue happens (like https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/scripts/Northrend/zone_borean_tundra.cpp#L867-L871)

[joshwhedon]

I think we might be able to replace the evade check in Unit::Attack with CanCreatureAttack instead (canCreatureAttack does the evade check too).

Creature* creature = ToCreature();
// creatures cannot attack invalid targets
if (creature && !creature->CanCreatureAttack(victim))
return false;
I don't think we should use IsValidAttackTarget because it might cause issues with neutral units

[jackpoz]

tip for people linking github lines: get a permalink that points to a specific commit or your link will be useless next time the file gets updated

[Treeston]

The AttackStart hook describes the behavior that should be exhibited when attacking a certain unit. It should not be directly invoked.

Use Unit::EngageWithTarget.

[jackpoz]

does the issue still happen in thassarian ? should the faction be changed to allow combat (or anyone else would like to fix that script ? ) ?

[jackpoz]

does the issue still happen in thassarian ? should the faction be changed to allow combat (or anyone else would like to fix that script ? ) ?

giving another chance to my questions

[offl]

NPC info output at that moment

Name: Prince Valanar
SpawnID: 0, current GUID low: 1239.
Entry: 28189.
Faction: 14.
NPC Flags: 0.
DisplayID: 25514 (Native: 25514).
Compatibility Mode: 0
Level: 72.
EquipmentId: 0 (Original: 0).
Health (base): 64170. (max): 64170. (current): 64170.
Movement type: Ground: Run, Swim: true, Flight: None, Chase: Run, Random: Walk, InteractionPauseTimer: 180000
Unit Flags: 557056
* UNIT_FLAG_SWIMMING (0x8000)
* UNIT_FLAG_IN_COMBAT (0x80000)
Unit Flags 2: 2048.
Dynamic Flags: 0.
Faction Template: 14.
SpawnTime: Full:5m Remaining:0s
Loot: 0 Pickpocket: 0 Skinning: 0
InstanceID: 0
Phasemask: 1
Armor: 7318
Position: 3726.474121 3562.319824 477.440857.
AIName:  ScriptName: 
React state: AGGRESSIVE
Active AI: struct npc_counselor_talbot::npc_counselor_talbotAI
Flags Extra: 0
MechanicImmuneMask: 0

Name: Thassarian
SpawnID: 101303, current GUID low: 256.
Entry: 26170.
Faction: 1892.
NPC Flags: 0.
DisplayID: 23649 (Native: 23649).
Spawn group: Dynamic Scaling (Escort NPCs) (ID: 3, Flags: 25, Active: 1)
Compatibility Mode: 0
Level: 72.
EquipmentId: 1 (Original: 1).
Health (base): 67270. (max): 67270. (current): 67270.
Movement type: Ground: Run, Swim: true, Flight: None, Chase: Run, Random: Walk, InteractionPauseTimer: 180000
Unit Flags: 524288
* UNIT_FLAG_IN_COMBAT (0x80000)
Unit Flags 2: 2048.
Dynamic Flags: 0.
Faction Template: 1892.
SpawnTime: Full:5m Remaining:0s
Loot: 0 Pickpocket: 0 Skinning: 0
InstanceID: 0
Phasemask: 1
Armor: 7318
Position: 3729.666260 3564.607178 477.440857.
AIName:  ScriptName: npc_thassarian
React state: AGGRESSIVE
Active AI: struct npc_thassarian::npc_thassarianAI
Flags Extra: 64
* CREATURE_FLAG_EXTRA_NO_XP (0x40)
MechanicImmuneMask: 0