Trinitycore: Proper SPELL_EFFECT_PULL_TOWARDS_DEST implementation for players

[xvwyh]

Description:

SPELL_EFFECT_PULL_TOWARDS_DEST is implemented as a parabolic spline jump, but it should in fact be implemented as a knockback for players, which can be verified by examining sniffs.

This prevents at least the following spells from working correctly (those are the only 3 use cases of the effect in 3.3.5):
https://www.wowhead.com/spell=46230 - Mu'ru in Sunwell Plateau, apparently
https://www.wowhead.com/spell=47764 - Grand Magus Telestra in The Nexus
https://www.wowhead.com/spell=68645 - Rocket Pack on ICC Gunships encounter

Implementation:

Revert 9a1282aac6befe6b3e26deb39d066fdc04452f4a

Spell.h

         void EffectPullTowards(SpellEffIndex effIndex);
+        void EffectPullTowardsDest(SpellEffIndex effIndex);

SpellEffects.cpp

-    &Spell::EffectPullTowards,                              //145 SPELL_EFFECT_PULL_TOWARDS_DEST                      Black Hole Effect
+    &Spell::EffectPullTowardsDest,                          //145 SPELL_EFFECT_PULL_TOWARDS_DEST

Spell::EffectPullTowards

-    float speedZ = (float)(m_spellInfo->Effects[effIndex].CalcValue() / 10);
-    float speedXY = (float)(m_spellInfo->Effects[effIndex].MiscValue/10);
+    float speedXY = m_spellInfo->Effects[effIndex].MiscValue / 10.0f;
+    float speedZ = damage / 10.0f;
     Position pos;
-    if (m_spellInfo->Effects[effIndex].Effect == SPELL_EFFECT_PULL_TOWARDS_DEST)
-    {
-        if (m_targets.HasDst())
-            pos.Relocate(*destTarget);
-        else
-            return;
-    }
-    else //if (m_spellInfo->Effects[i].Effect == SPELL_EFFECT_PULL_TOWARDS)
-    {
-        pos.Relocate(m_caster);
-    }
+    pos.Relocate(m_caster);

+void Spell::EffectPullTowardsDest(SpellEffIndex effIndex)
+{
+    if (effectHandleMode != SPELL_EFFECT_HANDLE_HIT_TARGET)
+        return;
+
+    if (!unitTarget)
+        return;
+
+    if (!m_targets.HasDst())
+    {
+        LOG_ERROR_in_whateverWayYou_do_itTheseDays("Spell %u with SPELL_EFFECT_PULL_TOWARDS_DEST has no dest target", m_spellInfo->Id);
+        return;
+    }
+
+    Position const* pos = m_targets.GetDstPos();
+    float distXY = unitTarget->GetExactDist(pos); // This is a blizzlike error: this should be 2D distance according to projectile motion formulas, but Blizzard erroneously used 3D distance
+    float distZ = pos->GetPositionZ() - unitTarget->GetPositionZ();
+
+    float speedXY = m_spellInfo->Effects[effIndex].MiscValue / 10.0f;
+    float speedZ = (2 * speedXY * speedXY * distZ + Movement::gravity * distXY * distXY) / (2 * speedXY * distXY);
+
+    unitTarget->JumpTo(speedXY, speedZ, true, pos);
+}
+

(Unit::JumpTo is actually a "knockback" for players, in case you're confused by my statements. It's only a "jump" for creatures)

This will result in knockback data identical to retail servers for players - players will get knocked in such a way that they will pass through the dest target. The caveat, however, is that for creatures this isn't enough. Perhaps the parabolic jump spline is calculated incorrectly for creature knockbacks in Unit::JumpTo, or perhaps this effect requires special handling, but these changes don't fix pulls for creatures properly. You would need to somehow calculate a ballistic trajectory that goes through the desired point (dest target) and then figure out when that trajectory will intersect with the floor (which isn't a flat plane) AND not let the creature escape the navmesh with such a jump. That's the ideal scenario, but whether or not blizzard does it that way - I don't know, I didn't research creatures, I had spent way too many hours just trying to figure out the formula for players. Could be that they just project the trajectory onto the same floor plane (z) as the creature, find where they intersect, and then nudge the point along the navmesh from creature's position towards the intersection point as far as possible without leaving the navmesh.

This will also require changes to spell_igb_rocket_pack: in HandlePeriodic you would need to check if the player is missing the "falling" movement flag (after first waiting for it to be applied, as there is no guarantee the player won't send more movement updates between the spell cast and knockback acknowledgement) instead of checking if the spline is finished, as there won't be any splines used for the pull anymore.

As for Magus Telestra's case, be advised that the effect won't look nice if you're not updating player's serverside position every time a CMSG_MOVE_KNOCK_BACK_ACK packet is received. Pull is triggered every 200ms there, so if the player's position isn't getting updated often (due to CMSG_MOVE_KNOCK_BACK_ACK being sent constantly, the client won't be very eager to also send movement heartbeat updates often), the effect will look as if the player is constantly being pulled from the same place. And the pull destination also needs to be shifted up 3 yards in a spell script, but that's beyond the scope of this issue.

Branch(es):

both

TC rev. hash/commit:

32e1de39a26628dcb64bc21ad415afb2ad938925 (irrelevant, but hi, bot)

[Keader]

Open a PR?

[xvwyh]

Once again, I won't open a PR for something I can't build and test. I don't work with current TrinityCore's codebase, and I'm too lazy to set it up. My goal is just to share my knowledge; whether you choose to use it or not - it's up to you. I already spent my share of time and effort on these changes, and the least I can do now is share them with the world so that this info won't remain on just one server and rot in my brain.

[Aokromes]

if you open one PR the bot will test build.

[xvwyh]

It won't test it in-game, however. There might be a multitude of code differences and nuances between our cores, and porting would require additional work to make it work properly even if it builds without errors. I'd rather let others, who are better familiar with TC, handle that. If they would be interested, of course. And if not - well, the code is here for all to see, whoever wants it can pick it up and adapt it.

[illfated]

if someone from either the TC team or a regular user (like me) opens a PR for the code,
I presume we could test it on the TC server mentioned in https://github.com/TrinityCore/TrinityCore/issues/23200#issuecomment-485827729
(if Aokromes or other managers of the server agrees to this).
I also presume that you would not mind offering suggestions in such a PR?

[xvwyh]

I have no idea what is that TC server, but you can do absolutely anything you want with everything I post here. Yes, I will gladly help with any questions you might have regarding my posts, or private server development in general, to the best of my abilities. As long as github remembers to notify me, that is...

[illfated]

I have not verified it yet, but I presume it is a straight-forward TrinityCore 3.3.5 server built from unmodified source.

[Aokromes]

```diff
diff --git a/src/server/game/Spells/Spell.h b/src/server/game/Spells/Spell.h
index c412ef3f2a..fa41d19246 100644
--- a/src/server/game/Spells/Spell.h
+++ b/src/server/game/Spells/Spell.h
@@ -343,6 +343,7 @@ class TC_GAME_API Spell
void EffectSendTaxi(SpellEffIndex effIndex);
void EffectKnockBack(SpellEffIndex effIndex);
void EffectPullTowards(SpellEffIndex effIndex);
+ void EffectPullTowardsDest(SpellEffIndex effIndex);
void EffectDispelMechanic(SpellEffIndex effIndex);
void EffectResurrectPet(SpellEffIndex effIndex);
void EffectDestroyAllTotems(SpellEffIndex effIndex);
diff --git a/src/server/game/Spells/SpellEffects.cpp b/src/server/game/Spells/SpellEffects.cpp
index e0d54a6c10..d56d5b4dd9 100644
--- a/src/server/game/Spells/SpellEffects.cpp
+++ b/src/server/game/Spells/SpellEffects.cpp
@@ -207,7 +207,7 @@ SpellEffectHandlerFn SpellEffectHandlers[TOTAL_SPELL_EFFECTS] =
&Spell::EffectTriggerSpell, //142 SPELL_EFFECT_TRIGGER_SPELL_WITH_VALUE
&Spell::EffectUnused, //143 SPELL_EFFECT_APPLY_AREA_AURA_OWNER
&Spell::EffectKnockBack, //144 SPELL_EFFECT_KNOCK_BACK_DEST
- &Spell::EffectPullTowards, //145 SPELL_EFFECT_PULL_TOWARDS_DEST Black Hole Effect
+ &Spell::EffectPullTowardsDest, //145 SPELL_EFFECT_PULL_TOWARDS_DEST
&Spell::EffectActivateRune, //146 SPELL_EFFECT_ACTIVATE_RUNE
&Spell::EffectQuestFail, //147 SPELL_EFFECT_QUEST_FAIL quest fail
&Spell::EffectTriggerMissileSpell, //148 SPELL_EFFECT_TRIGGER_MISSILE_SPELL_WITH_VALUE
@@ -4610,23 +4610,36 @@ void Spell::EffectPullTowards(SpellEffIndex effIndex)
if (!unitTarget)
return;

• float speedXY = m_spellInfo->Effects[effIndex].MiscValue / 10.0f;
• float speedZ = damage / 10.0f;
  Position pos;
• if (m_spellInfo->Effects[effIndex].Effect == SPELL_EFFECT_PULL_TOWARDS_DEST)
• {
• if (m_targets.HasDst())
• pos.Relocate(*destTarget);
• else
• return;
• }
• else //if (m_spellInfo->Effects[i].Effect == SPELL_EFFECT_PULL_TOWARDS)
• pos.Relocate(m_caster);
  +
• unitTarget->GetMotionMaster()->MoveJump(pos, speedXY, speedZ);
  +}
  +
  +void Spell::EffectPullTowardsDest(SpellEffIndex effIndex)
  +{
• if (effectHandleMode != SPELL_EFFECT_HANDLE_HIT_TARGET)
• return;
  +
• if (!unitTarget)
• return;
  +
• if (!m_targets.HasDst())
  {
• pos.Relocate(m_caster);
• LOG_ERROR_in_whateverWayYou_do_itTheseDays("Spell %u with SPELL_EFFECT_PULL_TOWARDS_DEST has no dest target", m_spellInfo->Id);

• return;
  }

• float speedXY = float(m_spellInfo->Effects[effIndex].MiscValue) * 0.1f;

• float speedZ = unitTarget->GetDistance(pos) / speedXY * 0.5f * Movement::gravity;
• Position const* pos = m_targets.GetDstPos();
• float distXY = unitTarget->GetExactDist(pos); // This is a blizzlike error: this should be 2D distance according to projectile motion formulas, but Blizzard erroneously used 3D distance

• float distZ = pos->GetPositionZ() - unitTarget->GetPositionZ();

• unitTarget->GetMotionMaster()->MoveJump(pos, speedXY, speedZ);

• float speedXY = m_spellInfo->Effects[effIndex].MiscValue / 10.0f;
• float speedZ = (2 * speedXY * speedXY * distZ + Movement::gravity * distXY * distXY) / (2 * speedXY * distXY);
  +

• unitTarget->JumpTo(speedXY, speedZ, true, pos);
  }

  void Spell::EffectDispelMechanic(SpellEffIndex effIndex)

[Rushor]

works nice on current core!

[Faq]

Made PR