Fix player position desync during movement#768
Fix player position desync during movement#768itayalroy wants to merge 4 commits intoMUnique:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the player movement system to eliminate desynchronization issues and enhance the overall movement experience. By introducing a projected walker that dynamically calculates player positions and step durations based on client-like rules, the server's understanding of player location is now much more accurate. This change resolves flaky movement behavior and improves consistency between client and server states, while also refining related game mechanics like observer updates and safezone attack validations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the timer-based Walker with a new ProjectedWalker that calculates player positions based on elapsed time. It also introduces detailed movement speed logic influenced by equipment (wings, pets, and mounts) and implements safezone-based attack blocking. Review feedback identifies a critical issue in the AOI manager where players may fail to update their bucket during a walk, a logic error in the projection causing a one-tile desync, and the presence of side effects within the Position property getter.
| if (obj is ISupportWalk supportWalk) | ||
| { | ||
| target = supportWalk.WalkTarget; | ||
| target = obj is IBucketMapObserver ? obj.Position : supportWalk.WalkTarget; |
There was a problem hiding this comment.
Changing the target to obj.Position for players (IBucketMapObserver) when the move type is Walk prevents the player from being moved to their destination bucket in the AOI manager. Since the ProjectedWalker does not trigger periodic bucket updates as the player moves, the player will remain stuck in their starting bucket for the entire duration of the walk. This will cause them to miss updates for objects appearing near their destination and potentially make them invisible to observers at the target location. It is recommended to revert to using supportWalk.WalkTarget to ensure the player is moved to the destination bucket immediately, which is the standard behavior for this architecture.
target = supportWalk.WalkTarget;There was a problem hiding this comment.
Updating the bucket to the walk target immediately (as done on main branch) is wrong as well, since it immediately removes the player from the current bucket making him invisible for players nearby. I will figure out a way to fix this.
| public Point Position | ||
| { | ||
| get | ||
| { | ||
| this.Refresh(); | ||
| lock (this._walkStateLock) | ||
| { | ||
| return this.GetProjectedPositionNoLock(out _); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Position property getter calls Refresh(), which mutates the internal state of the ProjectedWalker and triggers external side effects via _positionChanged (which eventually calls IAttackableMovedPlugIn). Mutating state and triggering plugin logic inside a property getter is generally considered a bad practice as it can lead to unexpected side effects during debugging or simple state inspection. Furthermore, since Refresh updates the character's stored coordinates without a lock on the character object, it introduces potential thread-safety issues if the position is queried from multiple threads (e.g., AOI updates from other players).
itayalroy
force-pushed
the
player_movement
branch
2 times, most recently
from
6afb1a5 to
a59ae7a
Compare
1 participant
The current
Walkerimplementation is too inaccurate compared to client-side player movement. As a result, the server could think a player was at an older or newer position than the client, reject valid walk requests, and send forced move corrections back, causing a flaky movement experience overall.Instead of updating player position from a background task with a fixed 300ms/500ms step delay, this PR calculates the player's current position on demand from the active walk path and elapsed movement time. Step duration is calculated per step using client-like movement speed rules, including safezone walking, wings, pets, Fenrir, and diagonal tile distance. When a walk completes or is interrupted, the projected position is persisted back to the stored character position.
Additionally, this PR fixes some issues with observer range, attacks involving safe zones, and stops sending movement paths to observers by default (as MuMain does not use them anymore).