Core/Player: increase ActivateTaxiPathTo() distance check #18774

Closed
wants to merge 4 commits into
from

Projects

None yet
@Staleness89
Staleness89 commented Jan 9, 2017 edited

Changes proposed:
Remove the distance check from ActiveTaxiPathTo()
Should Fix the failure to activate taxi paths for at least one quest (#18615)
Not sure if this is better than increasing the distance. Feedback please?

Target branch(es): 3.3.5/master

  • 3.3.5
  • master

Issues addressed: Closes # (insert issue tracker number)
#15398
#18615

Tests performed: (Does it build, tested in-game, etc.)
None. Feedback please?

Known issues and TODO list: (add/remove lines as needed)
None

@Staleness89 Staleness89 Remove distance check from ActivateTaxiPathTo()
2dfbb9f
@Staleness89 Staleness89 changed the title from (Core/Taxis) Remove or increase ActivateTaxiPathTo() distance check to (Core/Player) Remove or increase ActivateTaxiPathTo() distance check Jan 9, 2017
@ForesterDev
Contributor

I think, this changes are completely incorrect. Before this changes, error returns when some checks are fail, now error returns always, because you remove this checks.

@Staleness89

@ForesterDev Yes I was very tired at that moment, I can fix that.

@Staleness89 Staleness89 Remove the whole thing....
8a8be02
@jackpoz
Member
jackpoz commented Jan 9, 2017

Tests performed: (Does it build, tested in-game, etc.)
None. Feedback please?

What do you mean with "none" ? No tests at all ?

@Staleness89

No tests at all, obviously as I did it wrong the first time. It's a pretty straight forward change, the question is does it break anything else related, and I can't think of anything. I can also change it to increase the distance rather than remove the check completely.

@Aokromes
Member
Aokromes commented Jan 9, 2017

for sure the check is to stop hacks.

@Rushor
Contributor
Rushor commented Jan 9, 2017 edited

4 * INTERACTION_DISTANCE would not be enough here?

@Staleness89

@Aokromes Yes I thought the check might be an anti-hack. I can test at least one spot affected by this and find out if 4 * INTERACTION_DISTANCE is enough for it.

@Shauren
Member
Shauren commented Jan 9, 2017

And now it will always fail with flight masters (npc != nullptr)

@Staleness89
Staleness89 commented Jan 9, 2017 edited

edit: 4 * INTERACTION_DISTANCE is almost enough but maybe someone else could still find a different way to fix this. Closing for now, if no one else does anything I'll try again later after testing.

@Staleness89 Staleness89 closed this Jan 9, 2017
@Staleness89 Staleness89 reopened this Jan 9, 2017
@Staleness89 Staleness89 closed this Jan 9, 2017
@Staleness89 Staleness89 6 * INTERACTION_DISTANCE
62a662a
@Staleness89 Staleness89 reopened this Jan 9, 2017
@Aokromes
Member
Aokromes commented Jan 9, 2017

Meh i checked on 4.3.4 db and it got diff coordinates, then compared with mangos, we have same coords, checked sniff, bad luck Steward of Time is on proper coords.

@Staleness89

Tested with 6 * INTERACTION_DISTANCE, fixes #18615.

@Treeston
Member
Treeston commented Jan 9, 2017

feels like hack

{
GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
return false;
}
}
+
@jackpoz
jackpoz Jan 9, 2017 Member

it is good habit to review all changes pushed, trying to minimize the codestyle changes

@jackpoz
Member
jackpoz commented Jan 9, 2017

please include the original commit that set the value to 2 in your PR

@@ -21113,15 +21113,16 @@ bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc
return false;
}
- // check node starting pos data set case if provided
+ // check node starting pos data set case if provided
@tkrokli
tkrokli Jan 9, 2017 Member

Please remove this change, it is unnecessary. TC source does not use TABs.
GitHub will use 1 TAB = 8 spaces in preview. Correct indent is 4 spaces.

@tkrokli tkrokli Remove unneeded whitespace changes
Keeping only the relevant change and removing added (unneeded) whitespace.
ac5a733
@tkrokli tkrokli changed the title from (Core/Player) Remove or increase ActivateTaxiPathTo() distance check to Core/Player: increase ActivateTaxiPathTo() distance check Jan 9, 2017
@Treeston
Member
Treeston commented Jan 9, 2017

Why do we even check distance from taxi node here instead of distance from npc (if present)?

@Staleness89

@jackpoz - 2 was the original value
@Treeston how would you find the npc's position?

@Treeston
Member

npc is passed to ActivateTaxiPathTo

@Staleness89
Staleness89 commented Jan 10, 2017 edited

I tried this:

if (node->map_id != GetMapId() || !IsInDist(npc->GetPositionX(), npc->GetPositionY(), npc->GetPositionZ(), 2 * INTERACTION_DISTANCE))

which causes a server crash. Any idea on how to fix that?

@Treeston
Member

npc can be null

@Staleness89
Staleness89 commented Jan 11, 2017 edited

@Treeston I need a new hint.

@tkrokli
Member
tkrokli commented Jan 12, 2017

if you are doing this within

bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc /*= nullptr*/, uint32 spellid /*= 0*/)

, you check that npc is not null, either if (npc) or if (!npc) \ return false; and go from there.

@Treeston
Member

npc == null should still be handled (same check as current).

Please don't give advice if you have no idea what you are talking about.

@Eliminationzx
Contributor
Eliminationzx commented Jan 12, 2017 edited

I guess it should be like this

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 65e7ec8..cf6e6ad 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -21114,16 +21114,19 @@ bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc
     }
 
     // check node starting pos data set case if provided
-    if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
+    if (!npc)
     {
-        if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+        if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
         {
-            GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
-            return false;
+            if (node->map_id != GetMapId() || !IsWithinDist3d(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+            {
+                GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+                return false;
+            }
         }
     }
     // node must have pos if taxi master case (npc != NULL)
-    else if (npc)
+    else
@Staleness89

@Eliminationzx That fixed the crash, but now comes with the out of range error again (Still testing on Steward of Time) I believe it's because the npc doesn't actually start the taxi, the SAI involved makes the player self-cast the taxi start spell, so there's really no NPC involved in that calculation.

@Eliminationzx
Contributor

@Staleness89, post updated.

@Staleness89

@Eliminationzx still doesn't seem to work.

@ariel-
Member
ariel- commented Jan 19, 2017

it was working for years, and i don't remember any commit moving it, also another mob on dragonbright have the same issue.

Would be better to actually debug the root cause for the problem instead of touching code that isn't related to the problem being there in first place.

@ariel-
Member
ariel- commented Jan 21, 2017 edited

I retract. Maximum interaction distance tolerance is what caused the issue in the first place:

Commit 8758448 changed maximum distance from sqrt((2 * INTERACTION_DISTANCE)³) ie 31.62 yds approx to (2 * INTERACTION_DISTANCE) ie 10 yds

@ariel- ariel- reopened this Jan 21, 2017
@joschiwald joschiwald was assigned by ariel- Jan 21, 2017
@Treeston
Member
Treeston commented Jan 21, 2017 edited

8758448 was an intentional change. Old limit was practically non-existant, and open to hacks.

Simply increasing the antihack check distance is not the correct fix here.

@Treeston Treeston closed this Jan 21, 2017
@Staleness89

so.. where should it be fixed if not here?

@ariel-
Member
ariel- commented Jan 21, 2017

I know, but it's too restrictive

@Treeston
Member

It isn't. The issue here (as discussed in #15398) is that the core compares against the wrong position.

@ariel-
Member
ariel- commented Jan 21, 2017

I miscalculated, squared distances compared against squared constant, so max distance was sqrt(1000) instead 1000 ie 31.62 yds. Updated my previous comment

Still a typo, though.

@Treeston
Member
Treeston commented Jan 21, 2017 edited

No. "Proper" distance check based on intent of old code would be sqrt(2^2+2^2+2^2), or 3.46, times INTERACTION_DISTANCE, if you really cared.

Realistically you'll never be off in more than two directions so it'd be more of 2.82*INTERACTION_DISTANCE.

@ariel-
Member
ariel- commented Jan 21, 2017 edited

INTERACTION_DISTANCE is 5 yds, those are multiplications, (2*5)*(2*5)*(2*5) = 10*10*10 = 1000. Compared squared distance against a squared distance ie sqrt(1000).

I don't know how much leeway allows the client, if any.

@Eliminationzx
Contributor
Eliminationzx commented Jan 21, 2017 edited

Can we do something like this?

diff --git a/src/server/game/Entities/Player/Player.cpp b/src/server/game/Entities/Player/Player.cpp
index 65e7ec8..394502f 100644
--- a/src/server/game/Entities/Player/Player.cpp
+++ b/src/server/game/Entities/Player/Player.cpp
@@ -21114,20 +21114,23 @@ bool Player::ActivateTaxiPathTo(std::vector<uint32> const& nodes, Creature* npc
     }
 
     // check node starting pos data set case if provided
-    if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
+    if (!spellid)
     {
-        if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+        if (node->x != 0.0f || node->y != 0.0f || node->z != 0.0f)
         {
-            GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+            if (node->map_id != GetMapId() || !IsInDist(node->x, node->y, node->z, 2 * INTERACTION_DISTANCE))
+            {
+                GetSession()->SendActivateTaxiReply(ERR_TAXITOOFARAWAY);
+                return false;
+            }
+        }
+        // node must have pos if taxi master case (npc != NULL)
+        else if (npc)
+        {
+            GetSession()->SendActivateTaxiReply(ERR_TAXIUNSPECIFIEDSERVERERROR);
             return false;
         }
     }
-    // node must have pos if taxi master case (npc != NULL)
-    else if (npc)
-    {
-        GetSession()->SendActivateTaxiReply(ERR_TAXIUNSPECIFIEDSERVERERROR);
-        return false;
-    }
@Staleness89

There is no way to associate the caster with the npc triggering the event, as it's the same player. Should it be better to actually drop the check entirely if activating taxi through a spell?

@Staleness89 Staleness89 deleted the Staleness89:taxi_distance_check branch Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment