Fix player position desync during movement#769
Fix player position desync during movement#769itayalroy 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 addresses persistent player position desynchronization issues by transitioning from a time-delayed background task movement system to a deterministic, on-demand projection model. By accurately simulating client-side movement speed rules—including equipment and environmental modifiers—the server can now reliably validate and synchronize player positions, significantly improving the overall movement experience and reducing forced corrections. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a ProjectedWalker to handle player movement by projecting positions based on elapsed time and movement speed, replacing the previous Walker implementation. It also adds the 'Blow of Destruction' effect and updates movement speed calculations to account for various equipment and status effects. However, several critical issues were identified in the review: the new Position setter in Player.cs fails to notify the IAttackableMovedPlugIn, which will cause spatial desynchronization in the bucket map system. Additionally, the network packet size calculations for movement in ObjectMovedPlugIn.cs and ObjectMovedPlugInExtended.cs incorrectly use the buffer capacity instead of the actual step count, leading to malformed packets.
| this.GameContext.PlugInManager?.GetPlugInPoint<IAttackableMovedPlugIn>()?.AttackableMoved(this); | ||
| } | ||
| } | ||
| set => this._projectedWalker.Position = value; |
There was a problem hiding this comment.
The new Position setter removed the call to IAttackableMovedPlugIn.AttackableMoved. This is a critical omission because this plugin is responsible for updating the player's position in the bucket map system. Without this call, manual position updates (e.g., from other game logic or teleports) will cause the player to become desynchronized from the spatial partitioning system, leading to issues with visibility and interaction range.
set
{
if (this.Position != value)
{
this._projectedWalker.Position = value;
this.GameContext.PlugInManager?.GetPlugInPoint<IAttackableMovedPlugIn>()?.AttackableMoved(this);
}
}| int Write() | ||
| { | ||
| var stepsSize = steps.Length == 0 ? 1 : (steps.Length / 2) + 2; | ||
| var stepsSize = stepsLength == 0 ? 0 : (steps.Length / 2) + 2; |
There was a problem hiding this comment.
The calculation of stepsSize incorrectly uses steps.Length (the capacity of the rented buffer, which is 16) instead of stepsLength (the actual number of steps). This will result in oversized network packets filled with zeroed step data, which can cause issues with client-side movement synchronization. For
var stepsSize = stepsLength == 0 ? 0 : (stepsLength / 2) + 1;| int Write() | ||
| { | ||
| var stepsSize = steps.Length == 0 ? 1 : (steps.Length / 2) + 2; | ||
| var stepsSize = stepsLength == 0 ? 0 : (steps.Length / 2) + 2; |
There was a problem hiding this comment.
The current Walker implementation is not aligned to client-side player movement. It is based on flaky task delays, uses wrong speeds, and does not account for many parameters that client takes into account when calculating movement speed (safezones, boots, different wings, pets, mounts, diagonal tiles, skill effects such as ice that slows movement, etc). As a result, the server frequently thinks a player was at an older or newer position than the client, rejects valid walk requests, and sends 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 attacks involving safe zones, and stops sending movement paths to observers by default (as MuMain does not use them anymore).