Trinitycore: [3.3.5] Gameobjects and stealth interaction

[WannabeDevTwo]

Description:

Walking over a gameobject(for example BG buffs, or hunter traps) in stealth triggers the object, so it dissapears but the player receives no effect from GO trigger. Unfortunately #23936 didn't solve this issue.

Expected behaviour:

GO should have standart behavior when player triggers them from stealth.

Steps to reproduce the problem:

1. Walk over BG buff, or hunter trap in stealth
2. GO triggers, but player receive no effect

Branch(es):

3.3.5

TC rev. hash/commit:

ce449f6b5332ab466d935de0077bccfdde716d1b

[Killyana]

What's supposed to happen in this case?

[WannabeDevTwo]

If rogue for example walks over freezing trap in stealth, rogue should get freezing trap debuff(note, that stealth will be broken in that case). But now trap triggers, and just disappear.
For BG buffs, rogue should obtain buff from GO trigger when he walks over them in stealth, but remain stealthed.

[Rushor]

I think the spells should be able to target invisible

[Rushor]

we have a check here:

bool WorldObject::IsValidAttackTarget(WorldObject const* target, SpellInfo const* bySpell /= nullptr/) const

Unit const* unit = ToUnit();
    // visibility checks (only units)
    if (unit)
    {
        // can't attack invisible
        if (!bySpell || !bySpell->HasAttribute(SPELL_ATTR6_CAN_TARGET_INVISIBLE))
        {
            if (!unit->CanSeeOrDetect(target, bySpell && bySpell->IsAffectingArea()))
                return false;
        }
    }

i guess guess gameobjects should be handled in that area aswell

or add sth like

case 3355:  // Freezing Trap Effect
            case 14308: // Freezing Trap Effect
            case 14309: // Freezing Trap Effect
            case 31932: // Freezing Trap Effect
            case 43415: // Freezing Trap Effect
            case 43448: // Freezing Trap Effect
            case 55041: // Freezing Trap Effect
            case 60210: // Freezing Arrow Effect
            case 45145: // Snape Trap Effect
            case 43485: // Snape Trap Effect
            case 13812: // Explosive Trap Effect
            case 14314: // Explosive Trap Effect
            case 14315: // Explosive Trap Effect
            case 27026: // Explosive Trap Effect
            case 43446: // Explosive Trap Effect
            case 49064: // Explosive Trap Effect
            case 49065: // Explosive Trap Effect
            case 63487: // Frost Trap Effect
            case 41086: // Frost Trap Effect
            case 71647: // Frost Trap Effect
            case 13797: // Immolation Trap Effect
            case 14298: // Immolation Trap Effect
            case 14299: // Immolation Trap Effect
            case 14300: // Immolation Trap Effect
            case 14301: // Immolation Trap Effect
            case 27024: // Immolation Trap Effect
            case 49053: // Immolation Trap Effect
            case 49054: // Immolation Trap Effect
            case 51740: // Immolation Trap Effect
                spellInfo->AttributesEx6 |= SPELL_ATTR6_CAN_TARGET_INVISIBLE;
                break;

[Rushor]

update on this: you do not even get the buff if you are not in stealth. can someone else confirm that?

[Killyana]

If you're talking about Warsong buffs, we get the auras, we fixed that months ago.

[Rushor]

@Killyana okay, can you link me the commit related to that?

[Killyana]

This one : https://github.com/TrinityCore/TrinityCore/pull/23936/files

[Jildor]

I can't reproduce it

[Killyana]

Try in Warsong with a rogue, you will activate the gobs without getting the buff

[Jildor]

I can get the buff in stealth in wsg

edit: ok, when I use stealth works, when I use vanish I can't get the buff (and same happens with hunter traps)

[Jildor]

seems related with stealth value (vanish has more stealth value than only stealth or prowl)
if you put stealth and prowl at same time happens same as vanish

[Faq]

@xvwyh has some info about this? :)

[Jildor]

ok, I do this just for test:

@@ -1716,11 +1716,11 @@ bool WorldObject::CanDetectStealthOf(WorldObject const* obj, bool checkAlert) co
         if (unit && unit->HasAuraTypeWithMiscvalue(SPELL_AURA_DETECT_STEALTH, i))
             return true;

         // Starting points
         int32 detectionValue = 30;
-
+        detectionValue += 30000;
         // Level difference: 5 point / level, starting from level 1.
         // There may be spells for this and the starting points too, but
         // not in the DBCs of the client.
         detectionValue += int32(GetLevelForTarget(obj) - 1) * 5;

and adding a big value works, so the problem is detection?

[Jildor]

maybe I'm wrong, but I think after this commit, gameobject spell cast check stealth (CanSeeOrDetect), and maybe cause this bug because traps shouldn't check it?
https://github.com/TrinityCore/TrinityCore/commit/45c5e1b9d63796d168339a44f63418f220cf2403

[Jildor]

in case of wsg buff caster is gob trap, so if gob CanSeeOrDetect target, the target get the buff
in case of hunter traps, caster is player: https://github.com/TrinityCore/TrinityCore/commit/45c5e1b9d63796d168339a44f63418f220cf2403#diff-fe5c8ebb7ad8e663836b00f39eb524d6R701
so if player CanSeeOrDetect the target, target get the spell of hunter trap. (before was summon a npc trigger https://github.com/TrinityCore/TrinityCore/commit/45c5e1b9d63796d168339a44f63418f220cf2403#diff-fe5c8ebb7ad8e663836b00f39eb524d6L2025 )

in case of !CanSeeOrDetect the bug appear, so 45c5e1b is the cause

I don't know what is the solution here, @ariel- @jackpoz ??

[xvwyh]

@Faq, sorry, the core I worked with didn't have GO casting implemented, so I would refrain from commenting on this.

Although I tried it and seems like it's bugged too, but for a different reason (?): traps only activate if the trap owner can detect the stealthed enemy, due to IsValidAttackTarget check from inside https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Grids/Notifiers/GridNotifiers.h#L906. Unlike this issue, they don't activate at all (and don't despawn) if the trap owner can't see the enemy. Which is probably a bug, but I don't recall seeing any bugreports about it, so go figure.

As for this issue, I dunno, the most straightforward way of fixing it would be to allow GO to always detect stealthed enemies. Is there ever a case where they shouldn't? Logically-speaking, objects don't have eyes, so they can't "detect" anything anyway, but a trap should behave like a trap no matter if the one who triggered it is stealthed or not. And other on-demand spell-casting GOs (quest objects, lightwells etc) also probably shouldn't miss at stealthed users. But then you have to make the same case for invisiblity too, which blizzard used for phasing purposes before the phasing even existed and still uses today, and that's a totally different can of worms. And logic also rarely coexists with WoW mechanics anyway. So I don't know.

[joshwhedon]

The main problem with the go detection is that IsValidAttackTarget doesn't only check the visibility, it uses the trap owner to check all the target validity (there is no reference to the trap itself at the moment of the check).
At first glance, it should be possible to change NearestAttackableNoTotemUnitInObjectRangeCheck to take the gameobject as parameter, and to tweak IsValidAttackTarget to handle gameobjects better (for example when looking up the affected player, need to take the gameobject case into account)

[joshwhedon]

Ok so this one was a tricky one, it was like pulling a thread which unravelled a bunch of issues. I think I managed to fix it with a minimum of impact (I hope). I prepared a patch if someone wants to use it (note that my codebase is quite modified therefore line numbers will not match). Feel free to use it if it is a satisfying solution.
https://gist.github.com/joshwhedon/d62776ae49e13b3086bc602dcbd29df0

[Jildor]

ok, @joshwhedon I try this patch and seems not fully work
When you duel with same faction player or go to FFA zone (.tele bloodarena) with two players of same faction, traps don't work anymore (with/without stealth)

and I think need add something like this to do traps can detect stealth always (if you get high stealth with vanish, trap can't detect player if you don't add this):

@@ -1704,9 +1704,14 @@ bool WorldObject::CanDetectStealthOf(WorldObject const* obj, bool checkAlert) co
     if (distance < combatReach)
         return true;

    // Only check back for units, it does not make sense for gameobjects
    if (unit && !HasInArc(float(M_PI), obj))
         return false;

+    // Traps should detect stealth always
+    if (ToGameObject() && ToGameObject()->GetGoType() == GAMEOBJECT_TYPE_TRAP)
+        return true;
+
     GameObject const* go = obj->ToGameObject();
     for (uint32 i = 0; i < TOTAL_STEALTH_TYPES; ++i)
     {

[joshwhedon]

Ha I did not think of FFA zones. Hum ...

[joshwhedon]

Oh it looks like ReputationRank WorldObject::GetReactionTo(WorldObject const* target) const does not handle gameobjects very well.

Changing the following in it should help handle gameobjects a bit better :

• Unit const* unit = ToUnit();
• Unit const* targetUnit = target->ToUnit();
• Unit const* unit = ToUnit() ? ToUnit() : selfPlayerOwner;
• Unit const* targetUnit = target->ToUnit() ? target->ToUnit() : targetPlayerOwner;

[Jildor]

yep, seems fine now, I put here the full patch changes adding ReputationRank WorldObject::GetReactionTo changes and my suggestion for traps always detect stealth

diff --git a/src/server/game/Entities/Object/Object.cpp b/src/server/game/Entities/Object/Object.cpp
index c6fe2a918d..c0a69f0fc3 100644
--- a/src/server/game/Entities/Object/Object.cpp
+++ b/src/server/game/Entities/Object/Object.cpp
@@ -1704,9 +1704,14 @@ bool WorldObject::CanDetectStealthOf(WorldObject const* obj, bool checkAlert) co
     if (distance < combatReach)
         return true;

-    if (!HasInArc(float(M_PI), obj))
+    // Only check back for units, it does not make sense for gameobjects
+    if (unit && !HasInArc(float(M_PI), obj))
         return false;

+    // Traps should detect stealth always
+    if (ToGameObject() && ToGameObject()->GetGoType() == GAMEOBJECT_TYPE_TRAP)
+        return true;
+
     GameObject const* go = obj->ToGameObject();
     for (uint32 i = 0; i < TOTAL_STEALTH_TYPES; ++i)
     {
@@ -2638,8 +2643,9 @@ ReputationRank WorldObject::GetReactionTo(WorldObject const* target) const
                 return *repRank;
     }

-    Unit const* unit = ToUnit();
-    Unit const* targetUnit = target->ToUnit();
+    Unit const* unit = ToUnit() ? ToUnit() : selfPlayerOwner;
+    Unit const* targetUnit = target->ToUnit() ? target->ToUnit() : targetPlayerOwner;
+
     if (unit && unit->HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_PLAYER_CONTROLLED))
     {
         if (targetUnit && targetUnit->HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_PLAYER_CONTROLLED))
@@ -2899,7 +2905,7 @@ bool WorldObject::IsValidAttackTarget(WorldObject const* target, SpellInfo const
     if (IsFriendlyTo(target) || target->IsFriendlyTo(this))
         return false;

-    Player const* playerAffectingAttacker = ToUnit() && HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_PLAYER_CONTROLLED) ? GetAffectingPlayer() : nullptr;
+    Player const* playerAffectingAttacker = ToUnit() && HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_PLAYER_CONTROLLED) ? GetAffectingPlayer() : ToGameObject() ? GetAffectingPlayer() : nullptr;
     Player const* playerAffectingTarget = target->ToUnit() && target->HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_PLAYER_CONTROLLED) ? target->GetAffectingPlayer() : nullptr;

     // Not all neutral creatures can be attacked (even some unfriendly faction does not react aggresive to you, like Sporaggar)
@@ -2940,14 +2946,14 @@ bool WorldObject::IsValidAttackTarget(WorldObject const* target, SpellInfo const
     // additional checks - only PvP case
     if (playerAffectingAttacker && playerAffectingTarget)
     {
-        if (unitTarget->IsPvP())
+        if (playerAffectingTarget->IsPvP())
             return true;

-        if (unit->IsFFAPvP() && unitTarget->IsFFAPvP())
+        if (playerAffectingAttacker->IsFFAPvP() && playerAffectingTarget->IsFFAPvP())
             return true;

-        return unit->HasByteFlag(UNIT_FIELD_BYTES_2, UNIT_BYTES_2_OFFSET_PVP_FLAG, UNIT_BYTE2_FLAG_UNK1) ||
-            unitTarget->HasByteFlag(UNIT_FIELD_BYTES_2, UNIT_BYTES_2_OFFSET_PVP_FLAG, UNIT_BYTE2_FLAG_UNK1);
+        return playerAffectingAttacker->HasByteFlag(UNIT_FIELD_BYTES_2, UNIT_BYTES_2_OFFSET_PVP_FLAG, UNIT_BYTE2_FLAG_UNK1) ||
+            playerAffectingTarget->HasByteFlag(UNIT_FIELD_BYTES_2, UNIT_BYTES_2_OFFSET_PVP_FLAG, UNIT_BYTE2_FLAG_UNK1);
     }

     return true;
diff --git a/src/server/game/Grids/Notifiers/GridNotifiers.h b/src/server/game/Grids/Notifiers/GridNotifiers.h
index 31489d99aa..968984eb1f 100644
--- a/src/server/game/Grids/Notifiers/GridNotifiers.h
+++ b/src/server/game/Grids/Notifiers/GridNotifiers.h
@@ -903,7 +903,7 @@ namespace Trinity
                 if (!u->isTargetableForAttack(false))
                     return false;

-                if (!i_obj->IsWithinDistInMap(u, i_range) || !i_funit->IsValidAttackTarget(u))
+                if (!i_obj->IsWithinDistInMap(u, i_range) || !i_obj->IsValidAttackTarget(u))
                     return false;

                 i_range = i_obj->GetDistance(*u);
diff --git a/src/server/game/Spells/Spell.cpp b/src/server/game/Spells/Spell.cpp
index 2daa41cee5..9b626b6ab4 100644
--- a/src/server/game/Spells/Spell.cpp
+++ b/src/server/game/Spells/Spell.cpp
@@ -5212,7 +5212,7 @@ SpellCastResult Spell::CheckCast(bool strict, uint32* param1 /*= nullptr*/, uint
     {
         // Check explicit target for m_originalCaster - todo: get rid of such workarounds
         WorldObject* caster = m_caster;
-        if (m_originalCaster)
+        if (m_originalCaster && !caster->ToGameObject())
             caster = m_originalCaster;

         SpellCastResult castResult = m_spellInfo->CheckExplicitTarget(caster, m_targets.GetObjectTarget(), m_targets.GetItemTarget());

[joshwhedon]

Probably need someone competent to review it, because the changes I prepare, while they make sense if we want to treat gameobjects more generally, might not be the right way to handle things :D .

[Jildor]

yep, summon @jackpoz ;)

[jackpoz]

what do you want to summon me for ?

[Faq]

Please make a PR, then wait for comments, but my suggestion is to add more comments in code like was done in start with // Only check back for units, it does not make sense for gameobjects

[Jildor]

well jackpoz, you make this: https://github.com/TrinityCore/TrinityCore/pull/23936 that is more less related with this another issue (in the two issues the problem is the same, gob cast rework: https://github.com/TrinityCore/TrinityCore/commit/45c5e1b9d63796d168339a44f63418f220cf2403)
so maybe you can review if the fix go in the right direction or not.

But, maybe with PR is better, please @joshwhedon could you make a PR with fix?

[jackpoz]

I can't add any line comment to a patch posted in a message so please open a PR so we can review it. You can open it as Draft if you think it's not ready.

[jackpoz]

@Jildor can you open the PR please ?

[Jildor]

Shouldn't @joshwhedon do the PR?
he did the fix: https://github.com/TrinityCore/TrinityCore/issues/24224#issuecomment-605587498
I tested and made an addition to that fix, but @joshwhedon he investigated thoroughly, I think he should be do the job, in case he needs to make changes, to make it clearer.

[jackpoz]

see you in 1 year when no PR will have been opened yet

[joshwhedon]

I will be honest ... I just upgraded my server to the latest version of TC after 2 years, things are basically worse now :D, so I am a bit busy trying to fix a lot of stuff. I don't really have time to prepare a PR for this. So feel free to use it and make a PR for it if it does the trick for you.

[jackpoz]

https://github.com/TrinityCore/TrinityCore/commit/862097a63351e957fd4aa6cb82fe7d9edf4d3926