Skip to content

Fix player position desync during movement#769

Closed
itayalroy wants to merge 4 commits intoMUnique:masterfrom
itayalroy:player_movement
Closed

Fix player position desync during movement#769
itayalroy wants to merge 4 commits intoMUnique:masterfrom
itayalroy:player_movement

Conversation

@itayalroy
Copy link
Copy Markdown
Contributor

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).

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, 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

  • Movement System Overhaul: Replaced the legacy Walker implementation with a new ProjectedWalker that calculates position on-demand based on elapsed time and client-like movement speed rules, eliminating flaky background task delays.
  • Movement Speed Accuracy: Implemented detailed movement speed calculations that account for various factors including safezones, wings, pets, Fenrir, and active status effects like ice or the new Blow of Destruction.
  • Safezone & Combat Fixes: Added robust safezone checks for attacks and movement, and introduced the Blow of Destruction effect to the game logic.
  • Network Optimization: Stopped sending redundant movement paths to observers by default, as the modern client handles pathing independently.
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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/GameLogic/Player.cs
this.GameContext.PlugInManager?.GetPlugInPoint<IAttackableMovedPlugIn>()?.AttackableMoved(this);
}
}
set => this._projectedWalker.Position = value;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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 $N$ steps, the required byte count for the packed path is typically $1 + \lfloor(N-1)/2\rfloor$.

            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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The calculation of stepsSize incorrectly uses steps.Length instead of stepsLength. This leads to incorrect packet sizes being sent to the client, potentially including garbage data from the rented buffer.

            var stepsSize = stepsLength == 0 ? 0 : (stepsLength / 2) + 1;

@itayalroy itayalroy closed this May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant