Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core/Movement: Improve player movement #18771

Closed
wants to merge 1 commit into from

Conversation

chaodhib
Copy link
Member

@chaodhib chaodhib commented Jan 8, 2017

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.
  • Add clean support of special movement flags (can fly, can transition between fly and swim) to units controlled by players.
  • Add clean support of teleport to all units.

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 with how players are initialized in the world. I need help on this. EDIT: fixed in 722f581.
  • 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. EDIT: Done in 12f6027.
  • Bug hunt:

This PR requires a CMake re-run.

@@ -0,0 +1 @@
DELETE FROM `world`.`spell_linked_spell` WHERE `spell_trigger`='781';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

world.

@chaodhib
Copy link
Member Author

chaodhib commented Jan 8, 2017

thank you @Aokromes

@Shauren
Copy link
Member

Shauren commented Jan 8, 2017

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
Copy link
Member Author

chaodhib commented Jan 8, 2017

@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
Copy link
Member

Warpten commented Jan 9, 2017

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

@chaodhib
Copy link
Member Author

chaodhib commented Jan 9, 2017

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
Copy link
Contributor

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

@chaodhib
Copy link
Member Author

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.

@@ -26628,4 +26560,4 @@ uint32 Player::DoRandomRoll(uint32 minimum, uint32 maximum)
SendDirectMessage(&data);

return roll;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline.


UpdatePosition(movementInfo.pos);
m_movementInfo = movementInfo;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, newline is still missing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done now

@Eliminationzx
Copy link
Contributor

Eliminationzx commented Jan 28, 2017

More strange behaviors:
After teleporting on bg or from bg players sometimes has visual bug(lagg).

@Aokromes
Copy link
Member

the 2nd point is present on current versions.

@Eliminationzx
Copy link
Contributor

Any news?

@chaodhib
Copy link
Member Author

chaodhib commented Feb 12, 2017

I'm working on other stuff. I'll get back to this later (I know. I'm starting too many things at once. I have like 5+ open PRs :x)

I would like to know the interest for this PR though. And if the interest is high enough, I will change my priorities.

@Eliminationzx
Copy link
Contributor

Eliminationzx commented Feb 12, 2017

@chaodhib , can you fix updatespeed behavior? its so annoying...
Also, I've found a new behavior - sometimes knockback effects has wrong trajectory (only for players).
Try to use Blast Wave when targets will be in 1.0 - 5.0 distance.

@chaodhib chaodhib force-pushed the pm_rework branch 2 times, most recently from 0212288 to cb9fbd8 Compare February 17, 2017 20:01
@chaodhib
Copy link
Member Author

chaodhib commented Feb 17, 2017

@Eliminationzx I fixed the issue regarding the knockback. Thanks for the feedback.

Could you give more detail regarding the BG thing?

@Eliminationzx
Copy link
Contributor

Eliminationzx commented Feb 18, 2017

@chaodhib , more detail bg behavior:

  1. Go into bg with your group (for example).
  2. Leave bg with your group.
  3. Look at your group member, you can see how he's teleporting while moving(visual bug), but for him movement looks fine.

I don't know, probably it's random thing.

@Aokromes
Copy link
Member

like i said that teleport thing exists even without this pr.

@chaodhib
Copy link
Member Author

It's potentially something that this PR will solve.

A video showing the bug would help me.

@@ -26667,7 +26599,6 @@ uint32 Player::DoRandomRoll(uint32 minimum, uint32 maximum)

return roll;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo this :P (i fear to edit this with github) xd

@Eliminationzx
Copy link
Contributor

Eliminationzx commented Feb 27, 2017

@chaodhib , could you fix issue about disconnect after login with mount and other increase speed auras? I'll make a video about visual bug later.

@chaodhib
Copy link
Member Author

@Aokromes @tkrokli Thank you.

@Eliminationzx I talked about this issue in the PR's description (first point in the todo list). I'm not sure really how to solve it at the moment. If a call to AddToWorld() returns false before the UPDATE packet initializing the player is sent, I guess it won't be too difficult. Only drawback is that from my (limited) initial investigation, this isn't how retail servers work (cf above).

@ghost
Copy link

ghost commented Feb 28, 2017

Why Travis CI is not working at the moment: https://www.traviscistatus.com/incidents/hmwq9yy5dh9d

@thestrokes1
Copy link

Ok So Did the Scatter problem was solved? any link?

@sirikfoll
Copy link
Contributor

@chaodhib Do you know how to handle problem discussed in #21423? It's about being stunned or rooted while during a movespline, seems to fit here ^^

@tripleslash
Copy link

tripleslash commented Mar 22, 2019

Quoting some comment I posted on another issue that could be relevant here. The question was how roots are handled while the client is affected by a spline. Linking it here because it might be relevant for this PR.

The way this is handled in the client is:

Let's assume the client is currently falling or in a spline. However the sequency basically also applies in the same way for the other situations

  1. Server sends SMSG_FORCE_MOVE_ROOT to the client
  2. Client responds immediately with CMSG_FORCE_MOVE_ROOT_ACK with MOVEMENTFLAG_PENDING_ROOT set, but not MOVEMENTFLAG_ROOT set (This is only the case if the client is not falling or in a spline currently - otherwise he will instantly set MOVEMENTFLAG_ROOT)
  3. Endpoint of spline is reached with CMSG_MOVE_SPLINE_DONE, the flag will still be MOVEMENTFLAG_PENDING_ROOT but not MOVEMENTFLAG_ROOT
  4. CMSG_MOVE_SPLINE_DONE will always have MOVEMENTFLAG_FALLING set, because its impossible to stop a spline without being in falling state
  5. Fall is ended by CMSG_MOVE_FALL_LAND in CGUnit_C::OnCollideFallLandNotify, which internally calls CGUnit_C::UpgradePendingRoot
  6. After the fall is stopped (MOVEMENTFLAG_FALLING was removed) the server should verify that MOVEMENTFLAG_ROOT is set, and MOVEMENTFLAG_PENDING_ROOT was removed. If this is not true the client is cheating.

Fall can also be stopped by CMSG_MOVE_SET_FLY and CMSG_MOVE_START_SWIM, same checks should apply as in CMSG_MOVE_FALL_LAND.

@sirikfoll
Copy link
Contributor

@tripleslash Should it work the same way for Death Grip and Charge spells? I got a different packet sequence and no Pending flags for those (I'm using this PR)

With StopMoving() call in SetRooted()
ServerToClient: SMSG_ON_MONSTER_MOVE (0x00DD) Length: 52 ConnIdx: 0 Time: 03/23/2019 23:23:45.078 Number: 824
GUID: Full: 0x00000001 Type: Player Low: 1 Name: 
Toggle AnimTierInTrans: false
Position: X: -9448.55 Y: 68.236 Z: 56.3225
Move Ticks: 169
Spline Type: 0 (Normal)
Spline Flags: 6144 (Trajectory, WalkMode)
Move Time: 1501
Vertical Speed: 20.70732
Async-time in ms: 0
Waypoints: 1
Waypoint Endpoint: X: -9466.299 Y: 66.07617 Z: 56.04485

ServerToClient: SMSG_ON_MONSTER_MOVE (0x00DD) Length: 20 ConnIdx: 0 Time: 03/23/2019 23:23:45.484 Number: 829
GUID: Full: 0x00000001 Type: Player Low: 1 Name: 
Toggle AnimTierInTrans: false
Position: X: -9452.133 Y: 67.8 Z: 60.02477
Move Ticks: 170
Spline Type: 1 (Stop)

ServerToClient: SMSG_FORCE_MOVE_ROOT (0x00E8) Length: 6 ConnIdx: 0 Time: 03/23/2019 23:23:45.484 Number: 831
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Counter: 2

ServerToClient: SMSG_AURA_UPDATE (0x0496) Length: 18 ConnIdx: 0 Time: 03/23/2019 23:23:45.484 Number: 833
GUID: Full: 0x00000001 Type: Player Low: 1 Name: 
[0] Slot: 1
[0] Spell ID: 10308 (Hammer of Justice)
[0] Flags: 169 (EffectIndex0, NotCaster, Duration, Negative)
[0] Level: 80
[0] Charges: 0
[0] Max Duration: 6000
[0] Duration: 5898

ClientToServer: CMSG_FORCE_MOVE_ROOT_ACK (0x00E9) Length: 36 ConnIdx: 0 Time: 03/23/2019 23:23:45.484 Number: 835
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Counter: 2
Movement Flags: 134219776 (Root, SplineEnabled)
Extra Movement Flags: 128 (IsJumpSplineInAir)
Time: 184023733
Position: X: -9452.523 Y: 67.75249 Z: 60.82457
Orientation: 0.1210893
Fall Time: 35

ClientToServer: CMSG_MOVE_SPLINE_DONE (0x02C9) Length: 36 ConnIdx: 0 Time: 03/23/2019 23:23:45.484 Number: 836
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Flags: 2048 (Root)
Extra Movement Flags: 0 (None)
Time: 184023733
Position: X: -9452.523 Y: 67.75249 Z: 60.82457
Orientation: 0.1210893
Fall Time: 35
Movement Counter: 170

ServerToClient: SMSG_FORCE_MOVE_UNROOT (0x00EA) Length: 6 ConnIdx: 0 Time: 03/23/2019 23:23:51.484 Number: 869
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Counter: 3

ClientToServer: CMSG_FORCE_MOVE_UNROOT_ACK (0x00EB) Length: 52 ConnIdx: 0 Time: 03/23/2019 23:23:51.484 Number: 871
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Counter: 3
Movement Flags: 4096 (Falling)
Extra Movement Flags: 0 (None)
Time: 184029733
Position: X: -9452.523 Y: 67.75249 Z: 60.82457
Orientation: 0.1210893
Fall Time: 0
Fall Velocity: 0
Fall Sin Angle: 0.9926776
Fall Cos Angle: 0.1207936
Fall Speed: 0

ClientToServer: MSG_MOVE_HEARTBEAT (0x00EE) Length: 48 ConnIdx: 0 Time: 03/23/2019 23:23:51.984 Number: 873
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Flags: 12288 (Falling, FallingFar)
Extra Movement Flags: 0 (None)
Time: 184030233
Position: X: -9452.523 Y: 67.75249 Z: 58.41319
Orientation: 0.1210893
Fall Time: 500
Fall Velocity: 0
Fall Sin Angle: 0.9926776
Fall Cos Angle: 0.1207936
Fall Speed: 0

ClientToServer: MSG_MOVE_FALL_LAND (0x00C9) Length: 32 ConnIdx: 0 Time: 03/23/2019 23:23:52.187 Number: 874
Guid: Full: 0x00000001 Type: Player Low: 1 Name: 
Movement Flags: 0 (None)
Extra Movement Flags: 0 (None)
Time: 184030419
Position: X: -9452.523 Y: 67.75249 Z: 56.28094
Orientation: 0.1210893
Fall Time: 686
Without StopMoving() call in SetRooted()
ServerToClient: SMSG_ON_MONSTER_MOVE (0x00DD) Length: 52 ConnIdx: 0 Time: 03/23/2019 22:20:05.313 Number: 21
GUID: Full: 0x00000010 Type: Player Low: 16
Toggle AnimTierInTrans: false
Position: X: 62.67128 Y: 35.83758 Z: -144.7086
Move Ticks: 75
Spline Type: 0 (Normal)
Spline Flags: 6144 (Trajectory, WalkMode)
Move Time: 1501
Vertical Speed: 20.70732
Async-time in ms: 0
Waypoints: 1
Waypoint Endpoint: X: 52.42773 Y: 15.53711 Z: -144.7086

ServerToClient: SMSG_FORCE_MOVE_ROOT (0x00E8) Length: 6 ConnIdx: 0 Time: 03/23/2019 22:20:05.797 Number: 27
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Counter: 2

ServerToClient: SMSG_AURA_UPDATE (0x0496) Length: 18 ConnIdx: 0 Time: 03/23/2019 22:20:05.797 Number: 29
GUID: Full: 0x00000010 Type: Player Low: 16
[0] Slot: 2
[0] Spell ID: 10308 (Hammer of Justice)
[0] Flags: 169 (EffectIndex0, NotCaster, Duration, Negative)
[0] Level: 80
[0] Charges: 0
[0] Max Duration: 6000
[0] Duration: 5899

ClientToServer: CMSG_FORCE_MOVE_ROOT_ACK (0x00E9) Length: 36 ConnIdx: 0 Time: 03/23/2019 22:20:05.797 Number: 31
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Counter: 2
Movement Flags: 134219776 (Root, SplineEnabled)
Extra Movement Flags: 128 (IsJumpSplineInAir)
Time: 180204210
Position: X: 52.42773 Y: 15.53711 Z: -144.7086
Orientation: 4.245071
Fall Time: 0

ClientToServer: CMSG_MOVE_SPLINE_DONE (0x02C9) Length: 36 ConnIdx: 0 Time: 03/23/2019 22:20:05.797 Number: 32
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Flags: 2048 (Root)
Extra Movement Flags: 0 (None)
Time: 180204210
Position: X: 52.42773 Y: 15.53711 Z: -144.7086
Orientation: 4.245071
Fall Time: 0
Movement Counter: 75

ServerToClient: SMSG_FORCE_MOVE_UNROOT (0x00EA) Length: 6 ConnIdx: 0 Time: 03/23/2019 22:20:11.797 Number: 46
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Counter: 3

ClientToServer: CMSG_FORCE_MOVE_UNROOT_ACK (0x00EB) Length: 52 ConnIdx: 0 Time: 03/23/2019 22:20:11.797 Number: 48
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Counter: 3
Movement Flags: 4096 (Falling)
Extra Movement Flags: 0 (None)
Time: 180210197
Position: X: 52.42773 Y: 15.53711 Z: -144.7086
Orientation: 4.245071
Fall Time: 0
Fall Velocity: 0
Fall Sin Angle: -0.4504931
Fall Cos Angle: -0.8927799
Fall Speed: 0

ClientToServer: MSG_MOVE_FALL_LAND (0x00C9) Length: 32 ConnIdx: 0 Time: 03/23/2019 22:20:11.797 Number: 49
Guid: Full: 0x00000010 Type: Player Low: 16
Movement Flags: 0 (None)
Extra Movement Flags: 0 (None)
Time: 180210197
Position: X: 52.42773 Y: 15.53711 Z: -144.7086
Orientation: 4.245071
Fall Time: 0

@tanatosX
Copy link

tanatosX commented Apr 9, 2019

You are not quite right about the roots. I'll prove it now. Before you implement these systems, you must implement server-client time synchronization and after 13 years, finally turn the trinityCore from a single-player into a multi-player. Now it's only emulate client-server interaction. Maybe some devs don't really want to share secrets, and properly implement it in current trinityCore :) Or it's really very diffcult.

Also WoW movement/splines/transport used fucking doubly linked lists and very hardly to debug it.
The data below is relevant for the client 3.3.5 (12340).
So

  1. Server sends SMSG_FORCE_MOVE_ROOT to the client. Client creates new move event PMOVE_ROOT with current clientTime and added to local player moveQueue. (pastebin 1)

  2. Simplified, IdleMovementUpdate (global update cycle all movement on client) -> CMovement_C::MoveUnits(int currentTime, int lastUpdateTime) -> ExecuteMovement (these functions have big bunch of movers checks and timer checks for moveEvents list, also check spline state, skipped time, elapsed time or some others) -> UpdatePlayerMovement as bool result. If return 0 - mover removes from moversList. Also in this function checked moveStartTime (the same event creation time in client clocks) for move event.
    (pastebin 2, PMOVE_ROOT event case, pastebin 3 - TryRoot)

  3. Client send CMSG_FORCE_MOVE_ROOT_ACK and CMovement_C::CallMoveEventHandlers call this code (pastebin 4)

  4. CMovement_C::OnSplineStop triggers fall notify if spline has falling flag, fully deletes spline and triggers CMSG_MOVE_SPLINE_DONE because local player is active mover (pastebin 5). Real fall execution is handled in CMovement_C::CallMoveEventHandlers after stop spline (LABEL_20 and further)

Many simple events is handled WoW as move events (pastebin 6), also TIME_SYNC - this is very important part in server-client synchronization. We have to do a great job if we want to do it right.

And the most important thing - we have to update other "observer" clients with very accurate timestamp (timestamp as real event call in target client in client clocks sync with server clocks, with RTT, lagSync, skipTime or some other situations).

For example, blizzard init moveEvent RTT as default 50 ms for 32 last moveEvents and recalculate in real-time queue.

https://pastebin.com/pMxaPFLP

@Kittnz
Copy link
Member

Kittnz commented Jun 25, 2019

Whats the status of this PR, can we get rid of the conflicts?

@chaodhib
Copy link
Member Author

Besides the conflicts, it's not ready yet. There are still issues with client transfer control that need to be solved before this PR can be merged.

@jackpoz
Copy link
Member

jackpoz commented Dec 5, 2019

Closing this PR as it seems abandoned. Feel free to ask here to reopen it if anyone would like to work on it.

@kvipka
Copy link
Contributor

kvipka commented May 21, 2021

@chaodhib hi, can you update it with your implemented gameclient? Please

@chaodhib
Copy link
Member Author

it's in my plan, once the changes introduced in #26348 are production tested and stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet