Core/RestState: Check area trigger radius instead of wrong 1.0f. #15052

robinsch · 2015-07-11T17:23:17Z

Will fix #15016.

jackpoz · 2015-07-11T18:29:59Z

src/server/game/Entities/Player/Player.cpp

@@ -7424,7 +7426,7 @@ void Player::UpdateZone(uint32 newZone, uint32 newArea)
            if (GetRestType() == REST_TYPE_IN_TAVERN)        // Still inside a tavern or has recently left
            {
                // Remove rest state if we have recently left a tavern.
-                if (GetMapId() != GetInnPosMapId() || GetExactDist(GetInnPosX(), GetInnPosY(), GetInnPosZ()) > 1.0f)
+                if (GetMapId() != GetInnPosMapId() || GetExactDist(GetInnPosX(), GetInnPosY(), GetInnPosZ()) > (GetInnPosRadius() ? GetInnPosRadius() : 1.0f))


please don't try to convert a float to bool, use G3D::fuzzyEq() or similar to compare float

Rochet2 · 2015-07-11T19:03:33Z

This ignores the areatrigger box though if those are used for inns.

I would suggest this kind of fix https://gist.github.com/Rochet2/541bfecca1bfaa3b396d
It is not tested and doesnt work properly yet though due to timing with when the resting is checked upon leaving.

robinsch · 2015-07-11T19:23:06Z

Yes there are a few.
Do you want to open a PR or should I change my PR and make you the author of the commit?

Rochet2 · 2015-07-11T20:14:05Z

The code really needs fixing the exploit mentioned earlier or something to make the update tick when the player goes outside of the inn.
Currently its all depending on the client sent data and even then if the delta is removed even, you can slowly walk and not lose the resting bonus.

You can basically just walk out slowly or not send the zone update to avoid both fixes.
The codes are good, but need something to trigger them when player actually leaves the inns and whatnot.

robinsch · 2015-07-11T20:25:40Z

I think I already said it in previous PR, TC1 just calls UpdateZone on movementpackets. We could simply remove m_needsZoneUpdate, but performance wise this is not a good idea.

jackpoz · 2015-07-11T20:35:19Z

if the exploit exists in current code, it's ok to focus just on fixing the chair thing in this PR

Shauren · 2015-07-11T20:43:52Z

src/server/game/Entities/Object/Object.cpp

@@ -1193,6 +1193,35 @@ bool WorldObject::IsWithinLOSInMap(const WorldObject* obj) const
    return IsWithinLOS(ox, oy, oz);
 }

+bool WorldObject::IsWithinBox(const Position& center, float xradius, float yradius, float zradius) const


Since this method does not access any WorldObject members you can move it to Position class

robinsch · 2015-07-11T20:44:45Z

Used code from @Rochet2. Somehow if I squash commits the author is removed. 👎

Rochet2 · 2015-07-11T21:16:42Z

Well, if the current code is used, no one leaving the inn will have their rested removed.
If the current code without the additional delta (5 yard addition) is used, you can try to move really slow out of the inn and you will walk out with the rested bonus as well.

The old code worked so that almost always you are out of range with the 1.0f checking, but if it is corrected, the delays between client and server make it glitch more as you can just go out of the inn with the inn buff even when not hacking.

One of the few proper fixes that would fix the problems and the exploit would be to keep a timed event for the players with rested bonus of an inn triggered by an area trigger that would check if they are still in the inn.
But that would create additional overhead compared to how it works atm.

Rochet2 · 2015-07-11T22:05:27Z

Here is the proper edit that fixes both the exploit and the issue at hand.
But Im unsure if the timed checking is fine for you.
https://gist.github.com/Rochet2/541bfecca1bfaa3b396d
also moved the method shauren commented about to position class in this diff. (the current refactor in this PR is bad)

ghost · 2015-07-12T08:22:29Z

@robinsch : to set Rochet2 as author, checkout your at branch, then open a git bash window & run:

git commit --amend --author="Rochet2 <rochet2@post.com>"

This will open the vi editor with the author info inserted. Use the command :w to save, :q to quit.
Exit git bash. In Git Extensions, open the commit window, tick [√] Amend commit, do commit + force push.

joschiwald · 2015-07-12T11:46:04Z

src/server/game/Entities/Object/Position.cpp

+    double sinVal = std::sin(rotation);
+    double cosVal = std::cos(rotation);
+
+    float BoxDistX = GetPositionX() - GetPositionX();


it does not make sense

The refactor was bad.
Read #15052 (comment)
which contains the link to the code it should look like.
PR needs update.

OK, I see it. Inserted 31 lines too far down, should be between line 99 and 100 instead of 130-131.

OK, I see it. Inserted 31 lines too far down, should be between line 99 and 100 instead of 130-131.

I dont think the actual position matters that much .. unlike the correctness of the actual code.

Rochet2 · 2015-07-14T21:58:15Z

👍 Didnt even notice this was updated

jackpoz · 2015-08-10T12:53:33Z

is this PR ready to be merged ? both the exploits and the bugs are fixed ?

Rochet2 · 2015-08-10T13:15:36Z

is this PR ready to be merged ? both the exploits and the bugs are fixed ?

It is not!
The PR is like the code here

Here is the proper edit that fixes both the exploit and the issue at hand.
But Im unsure if the timed checking is fine for you.
https://gist.github.com/Rochet2/541bfecca1bfaa3b396d

But after a closer look its missing this vital part: https://gist.github.com/Rochet2/541bfecca1bfaa3b396d#file-innsfix-diff-L73-L83
And Im not sure how you would prefer to handle that.

I tested the code in the gist and as said it fixes both exploits. Exploits being players able to walk out slowly and the relying on client sent data. The exploits are fixed by the code in player::update function. It also fixes the original issue ofcourse.

However the PR does not resolve the issue of using client sent data for any other piece of code than inn resting.
Here is the comment I originally made about that zoneupdate exploit that uses client data:
#15041 (comment)

The exploit I mentioned comes from the fact that client data is relied on.
If the client does not send CMSG_ZONEUPDATE opcode, he can keep the resting status outside of an inn and walk around as long as he wont change the area (EG he can keep it anywhere in ewlynn fores as long as he got it from goldshire (within ewlynn)). Changing area or zone doesnt matter.

Probably should create a separate issue for that exploit as it doesnt consern the inns anymore after this PR, but I have no other known things it could affect atm.

robinsch · 2015-08-10T13:17:01Z

Exploit was also possible before so I think it's fine to merge.

jackpoz · 2015-08-10T13:25:28Z

@Rochet2 so it makes more sense to close this PR and open on with your diff

Closes TrinityCore#15016 Closes TrinityCore#15052

Closes TrinityCore#15016 Closes TrinityCore#15052 Signed-off-by: jackpoz <giacomopoz@gmail.com>

Closes #15016 Closes #15052 Signed-off-by: jackpoz <giacomopoz@gmail.com> (cherry picked from commit 12931f4) Conflicts: src/server/game/Entities/Player/Player.h src/server/game/Handlers/MiscHandler.cpp

ghost · 2015-09-26T13:00:10Z

I wonder what is wrong with the goldshire lion's inn area trigger. even with that commit it doesn't seem to add the resting state. the trigger itself is also very far under the ground so dunno if the trigger there is even triggered or so.

Rochet2 · 2015-09-26T13:12:27Z

I wonder what is wrong with the goldshire lion's inn area trigger. even with that commit it doesn't seem to add the resting state. the trigger itself is also very far under the ground so dunno if the trigger there is even triggered or so.

Can not reproduce on facb656. Have you made any edits to the areatrigger or smart scripts tables?
What TC version do you have? Can you be more specific about the conditions you are testing with? (race or character etc)
http://prntscr.com/8knuqr

ghost · 2015-09-26T14:14:44Z

nvm. forgot to change the worldserver after the compile. seems to be fine

ghost · 2015-09-26T14:36:39Z

BUT I found another issue. If you stand on the wooden parts of the Inn the rest state is there but when you walk down on the brick floor there near the cozy fire the rest state disappears

Rochet2 · 2015-09-27T07:39:27Z

If you stand on the wooden parts of the Inn the rest state is there but when you walk down on the brick floor there near the cozy fire the rest state disappears

Can not reproduce on facb656.
However I walked through the whole house and around it and since the areatrigger seems to be round and the house is a rectangle if watched from above, the rest state is active if you are outside: http://prntscr.com/8kyd00 and it is not active if you are at the furthest corner of the building: http://prntscr.com/8kydfq
Dont know how it behaved/behaves on retail though.

robinsch mentioned this pull request Jul 11, 2015

Core/MovementHandler: Only call UpdateZone when newzone is diff from oldzone #15041

Closed

Aokromes added Feedback-PatchFix Comp-Core labels Jul 11, 2015

jackpoz reviewed Jul 11, 2015
View reviewed changes

Shauren reviewed Jul 11, 2015
View reviewed changes

joschiwald reviewed Jul 12, 2015
View reviewed changes

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15016

9d32bb3

jackpoz added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Aug 10, 2015

Rochet2 added a commit to Rochet2/TrinityCore that referenced this pull request Aug 10, 2015

Core/RestState: Check area trigger radius instead of wrong 1.0f.

858fb23

Closes TrinityCore#15016 Closes TrinityCore#15052

Rochet2 mentioned this pull request Aug 10, 2015

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15249

Closed

jackpoz closed this Aug 10, 2015

jackpoz pushed a commit to jackpoz/TrinityCore that referenced this pull request Aug 10, 2015

Core/RestState: Check area trigger radius instead of 1.0f distance

12931f4

Closes TrinityCore#15016 Closes TrinityCore#15052 Signed-off-by: jackpoz <giacomopoz@gmail.com>

This was referenced Aug 11, 2015

[DEV] Changelog (TDB: 335.60 & 6.03) #15079

Closed

[DEV] 3.3.5 -> 6.x cherry-picks (TDB: 335.60 & 6.03) #15077

Closed

Aokromes removed the Feedback-FixOutdatedMissingWIP label Aug 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15052

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15052

robinsch commented Jul 11, 2015

jackpoz Jul 11, 2015

Rochet2 commented Jul 11, 2015

robinsch commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

robinsch commented Jul 11, 2015

jackpoz commented Jul 11, 2015

Shauren Jul 11, 2015

robinsch commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

ghost commented Jul 12, 2015

joschiwald Jul 12, 2015

Rochet2 Jul 12, 2015

ghost Jul 12, 2015

Rochet2 Jul 12, 2015

Rochet2 commented Jul 14, 2015

jackpoz commented Aug 10, 2015

Rochet2 commented Aug 10, 2015

robinsch commented Aug 10, 2015

jackpoz commented Aug 10, 2015

ghost commented Sep 26, 2015

Rochet2 commented Sep 26, 2015

ghost commented Sep 26, 2015

ghost commented Sep 26, 2015

Rochet2 commented Sep 27, 2015

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15052

Core/RestState: Check area trigger radius instead of wrong 1.0f. #15052

Conversation

robinsch commented Jul 11, 2015

jackpoz Jul 11, 2015

Choose a reason for hiding this comment

Rochet2 commented Jul 11, 2015

robinsch commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

robinsch commented Jul 11, 2015

jackpoz commented Jul 11, 2015

Shauren Jul 11, 2015

Choose a reason for hiding this comment

robinsch commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

Rochet2 commented Jul 11, 2015

ghost commented Jul 12, 2015

joschiwald Jul 12, 2015

Choose a reason for hiding this comment

Rochet2 Jul 12, 2015

Choose a reason for hiding this comment

ghost Jul 12, 2015

Choose a reason for hiding this comment

Rochet2 Jul 12, 2015

Choose a reason for hiding this comment

Rochet2 commented Jul 14, 2015

jackpoz commented Aug 10, 2015

Rochet2 commented Aug 10, 2015

robinsch commented Aug 10, 2015

jackpoz commented Aug 10, 2015

ghost commented Sep 26, 2015

Rochet2 commented Sep 26, 2015

ghost commented Sep 26, 2015

ghost commented Sep 26, 2015

Rochet2 commented Sep 27, 2015