Core/Movement: Improve player movement #18771

Open
wants to merge 79 commits into
from

Projects

None yet

6 participants

@chaodhib
Contributor
chaodhib commented Jan 8, 2017 edited

Changes proposed:

Goal: Significantly improve player movement smoothness and reduce jerkiness

  • Add clean support of speed changes to all units
  • Add clean support of collision height changes to units controlled by players
  • Add clean support for knock backs to units controlled by players
  • Add clean support of the toggle-able movement flags (root, hover, gravity disabled, water walk, feather fall) to all units. Please do not review this part yet. It's not finished.
  • Add clean support of special movement flags (can fly, can transition between fly and swim) to units controlled by players. Please do not review this part yet. It's not finished.
  • Add clean support of teleport to all units. Please do not review this part yet. It's not finished.

The biggest change is the remodeling of the way movement changes (change of speed, movement flag, etc..) are applied on units controlled by players. Let's take the example of a change of speed:
Before: when SetSpeed(MOVE_RUN, 10) was called on any unit, the change was active as soon as the method returned.
Now: when SetSpeed(MOVE_RUN, 10) is called on an unit moved by a player, first the server sends a packet to the player's client to inform it of the changes. The client then replies with a confirmation. Only when the confirmation is received by the server that the change is actually applied, and then propagated to the client of the other players. The reason why this change was needed can be found here.

More information on this can be found in the header of this file: https://github.com/chaodhib/TrinityCore/blob/c792f35f0e6492d933c3aabc2cd20ebc42c38aad/src/server/game/Movement/MovementPacketSender.h

These changes take into account player taking possession of another player, of a creature or of a vehicle.
These changes also include an implementation of the movement counter and a queue to keep track of pending player changes (this change is also needed for the implementation of solid anti speed hack detection mechanism).

Videos:
Before: https://youtu.be/KnsQ_Zqz0kQ
After: https://youtu.be/MwpWViAxhbs

Target branch(es): 3.3.5

Issues addressed: Closes at least #18582

Tests performed: Tested IG.

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

  • When a player starts the game with a buff (or debuff) affecting his speed (example: mount), the server sends the update speed packet to the client before it even send the SMSG_COMPRESSED_UPDATE_OBJECT packet that will initialize the player. This is too soon and the client does not reply to the "update speed packet", which requires an ack. Next time a movement packet which requires an ack is sent to the client (change of speed for example), the client is kicked from the server on the ground that it should have replied to the first packet first. Not sure how to handle this. This is outside of the scope of this PR. It's a bug in the core on how players are initialize in the world. I need help on this.
  • The hunter's disengage spell 781 invoke another "disengage" spell 56446 which is wrong. this resulst in 2 KNOCK_BACK packets sent. According to sniffs, the first spell should not trigger another spell. I fixed it in f3ad754 but it shouldn't really be part of this PR.
  • A timer on the pending movement changes should be implemented. If the client takes too much time to confirm a movement change, disciplinary measures (log, kick and/or ban depending on choice of admin) should be taken.

This PR requires a CMake re-run.

chaodhib added some commits Dec 25, 2016
@chaodhib chaodhib Ensure that SMSG_MOVE_SET_COLLISION_HGT is only sent from one place f1c2f74
@chaodhib chaodhib move the code used to send MSG_MOVE_TELEPORT_ACK packets to Mouvement…
…Sender
de82b2e
@chaodhib chaodhib Gonna close my eyes and prevent I never saw the implementation of Uni…
…t::SendTeleportPacket()
f0e2321
@chaodhib chaodhib Rename MovementSender to MovementPacketSender. b185516
@chaodhib chaodhib Remove unnecessary if e3b5394
@chaodhib chaodhib Ensure that all speed change packet are sent from the same 3 methods 9b1a1ea
@chaodhib chaodhib clean up 2b9e790
@chaodhib chaodhib I'm stupid. Should have done this sooner. 37a2e38
@chaodhib chaodhib Move Player::SetMovement to MovementPacketSender 1328be1
@chaodhib chaodhib Move the movement counter to Unit from Player d2ff8d8
@chaodhib chaodhib Add const method IsMovedByPlayer to Unit d5b6263
@chaodhib chaodhib Gather more movement packet transfer code to MovementPacketSender 7f23798
@chaodhib chaodhib fix mistake ef2e311
@chaodhib chaodhib Gather knockback code 7b5272a
@chaodhib chaodhib Gather hover code 6d09cb6
@chaodhib chaodhib Gather can fly code 7dedab4
@chaodhib chaodhib gather feather fall code d46e278
@chaodhib chaodhib gather disable gravity code 311b19e
@chaodhib chaodhib fix compilation 2941b4a
@chaodhib chaodhib Add MOVEMENTFLAG2_CAN_TRANSITION_BETWEEN_SWIM_AND_FLY to enum Movemen…
…tFlags2
75f7999
@chaodhib chaodhib - removed PlayerMovementType
- Gathered the MovementPacketSender::Send movement flag
a83dd97
@chaodhib chaodhib remove redundant field force_move_type 3320dd3
@chaodhib chaodhib first try at going full circle with the speed change of a player-move…
…d unit
49a75fb
@chaodhib chaodhib moved handler methods that belonged more in MovementHandler.cpp 88553fa
@chaodhib chaodhib clean up ad09f07
@chaodhib chaodhib small fix 6d6de75
@chaodhib chaodhib Add 2 methods to serialize/unserialize MovementInfo structures 1d079bf
@chaodhib chaodhib Delete ReadMovementInfo & WriteMovementInfo and replace them with the…
… 2 new methods on MovementInfo.
c7242f2
@chaodhib chaodhib Introduce GetMovementInfo() and UpdateMovementInfo() in Unit to repla…
…ce Unit::BuildMovementPacket
80be131
@chaodhib chaodhib Minor changes to MovementInfo serialize/unserialize methods 0e47adb
@chaodhib chaodhib Moved Movement Flag accessor methods from Unit To GameObject c7bd61d
@chaodhib chaodhib Add SetFallTime() in GameObject 07fbd5d
@chaodhib chaodhib Move up GetMovementInfo(), UpdateMovementInfo() and ValidateNewMoveme…
…ntInfo from Unit to WorldObject
b8a9907
@chaodhib chaodhib Deleting Unit::BuildMovementPacket 9fdd378
@chaodhib chaodhib Minor change 5e7e0ce
@chaodhib chaodhib Adapt the code to the new way of accessing the movementInfo 850d422
@chaodhib chaodhib minor clean up MovementHandler 7a393c9
@chaodhib chaodhib partial revert of b8a9907
UpdateMovementInfo does not need to be in WorldObject anymore.
Also introduce Unit::ApplyChangesOfMovementInfo which is called by Unit::UpdateMovementInfo
da8b694
@chaodhib chaodhib fix malformed packets 781b878
@chaodhib chaodhib clean up 3a6d20b
@chaodhib chaodhib adapt to latest changes b597efe
@chaodhib chaodhib minor change a5fe457
@chaodhib chaodhib a lot of desync between m_movementInfo and the movement/position stat…
…e of unit has been causing a tons of bug. gonna get ride of m_movementInfo
523a87f
@chaodhib chaodhib minor change 2e26161
@chaodhib chaodhib WorldObject::GetMovementInfo() now returns a new generated on fly Mov…
…ementInfo based on the state of the unit. Basically a roll back of a few commits ago.
fd6e351
@chaodhib chaodhib add a second parameter to WriteContentIntoPacket & FillContentFromPac…
…ket in MovementInfo for more convenient use
b42d5c7
@chaodhib chaodhib reducing redondancy between WorldObject::GetMovementInfo and Unit::Ge…
…tMovementInfo
bd4b207
@chaodhib chaodhib Remove unnecessary override of the method WorldObject::GetMovementInfo 496b39d
@chaodhib chaodhib Polish 3952c58
@chaodhib chaodhib comment 0f61593
@chaodhib chaodhib add correct logger name in MovementPacketSender logging methods d1f5189
@chaodhib chaodhib Rename a few methods. Fix bad copy pasta of logger messages 4a6f20a
@chaodhib chaodhib Add comment bcc7401
@chaodhib chaodhib More work on the MovementPacketSender bd2dbd9
@chaodhib chaodhib Add movement counter and 2 movement timestamps 031d7f2
@chaodhib chaodhib clean up 919647c
@chaodhib chaodhib moar clean up eb5bd86
@chaodhib chaodhib m-o-a-r 6943301
@chaodhib chaodhib Add pending changes queue 11ace65
@chaodhib chaodhib make use of the pending change queue in the speed change ack handler e2d2905
@chaodhib chaodhib clean up, renaming & improvements + temporary log methods 11cf6f6
@chaodhib chaodhib commenting a line that is part of something that should be reworked i…
…n the future
6a2d02a
@chaodhib chaodhib improve Unit::UpdateMovementInfo so it does redundant tasks that most…
… movement handlers should do
9a41ee4
@chaodhib chaodhib remove unused method e22a930
@chaodhib chaodhib some clean up 736d503
@chaodhib chaodhib Acknowledgement movement packets needs to be handled in the order the…
…y are received
e677c3d
@chaodhib chaodhib probably better to handle all movement packet in a non parallele way …
…to avoid jitters and small back step in the time lines.
8ffacaa
@chaodhib chaodhib Implement collision height handling on players 29920d2
@chaodhib chaodhib fix time 5407b5b
@chaodhib chaodhib implement knock back 23b7c62
@chaodhib chaodhib fix hunter disengage. SPELL_EFFECT_LEAP_BACK was called twice. this c…
…ommit should be reviewed and potentially rolled back if needed. Should not really be part of this PR.
f3ad754
@chaodhib chaodhib fix bug in knock back handling b6d8c3f
@chaodhib chaodhib add logger method b68ca7b
@chaodhib chaodhib clean up e48fc36
@chaodhib chaodhib more clean up 097027b
@chaodhib chaodhib forgot this part in b6d8c3f 765df3b
@chaodhib chaodhib oups 37aebfa
@chaodhib chaodhib various clean ups
c792f35
@@ -0,0 +1 @@
+DELETE FROM `world`.`spell_linked_spell` WHERE `spell_trigger`='781';
@Kittnz
Kittnz Jan 8, 2017 Contributor

world.

@Aokromes Aokromes Update XXXX_XX_XX_XX_world_335.sql
97778c0
@chaodhib
Contributor
chaodhib commented Jan 8, 2017

thank you @Aokromes

@Shauren
Member
Shauren commented Jan 8, 2017 edited

Ref point 1 - the speed packet should not be sent in a separate packet requiring ack, it is sent in SMSG_UPDATE_OBJECT (and so should be effective immediately)

disclaimer: also I did not read the code, only your description

@chaodhib
Contributor
chaodhib commented Jan 8, 2017 edited

@Shauren This would be logical yeah but that's not how it works in retail. Here are the relevant part of a sniff on this: here

In it, we can clearly see that the player created through the SMSG_UPDATE_OBJECT packet (here it's actually a SMSG_COMPRESSED_UPDATE_OBJECT) has a normal speed. After that packet has arrived SMSG_MOVE_SET_COLLISION_HGT and SMSG_FORCE_RUN_SPEED_CHANGE are sent, which set the speed of the player (and the height since mounted players have bigger height).

@Warpten
Member
Warpten commented Jan 9, 2017

Issue one: move update speed calls to after object update creation, aka SendInitialPacketsAfterAddToMap?

@chaodhib
Contributor
chaodhib commented Jan 9, 2017 edited

Here is the call stack leading to the sending of the update speed packet. In it, we can see that the update speed packet is sent during aura initialization from DB.

call stack

@Takenbacon
Contributor

@chaodhib Why not ignore UpdateSpeed calls if not in world? The client will receive the correct speed anyways in SMSG_UPDATE_OBJECT

@chaodhib
Contributor

Because that's not how it works in retail (like I showed by the sniff linked above). Also, to the extent possible, I would like that the 'pending changes tracking' code does not have to deal with this stuff.

@Kittnz Kittnz added the Comp-Core label Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment