Implement smooth move to location broadcasting mechanism#20
Implement smooth move to location broadcasting mechanism#20
Conversation
WalkthroughAdds a movement subsystem: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Game Client
participant Handler as MoveToLocation Handler
participant Player as PlayerClient (actor)
participant Movement as MovementState
participant Task as Periodic Broadcast Task
participant Nearby as Nearby Players
Client->>Handler: Send MoveToLocation packet
Handler->>Player: start_movement(dest_x,dest_y,dest_z)
Player->>Movement: cancel existing / create new MovementState
Player-->>Handler: Return current position
Handler->>Nearby: Send initial movement packet
Handler->>Task: Spawn periodic broadcast task
loop until arrival or cancellation
Task->>Player: GetMovementPosition
Player->>Movement: calculate_current_position()
Movement-->>Player: (x,y,z) + arrived?
Player-->>Task: Reply position + arrival flag
Task->>Nearby: Broadcast current position
end
alt arrival detected
Task->>Task: Exit (task completes)
else client requests stop
Client->>Handler: Send StopMove packet
Handler->>Player: stop_movement()
Player->>Movement: cancel_task() / clear state
Handler->>Nearby: (TODO) broadcast stop position
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
game/src/packets/from_client/move_to_location.rs (3)
54-54: Unused return value fromstart_movement.The
source_x,source_y,source_ztuple is captured but never used. If this is intentional (e.g., for future logging or validation), consider prefixing with_. Otherwise, simplify the call.- let (source_x, source_y, source_z) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?; + let _ = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?;
92-97: Redundant actor call in broadcast loop.Each tick calls
GetCharInfoto get a fullPlayerclone when position is already retrieved fromGetMovementPosition. Consider caching theobject_idbefore the loop or extendingGetMovementPositionto return it, avoiding the extra actor round-trip every 300ms.
137-140: Side effect in getter method.
GetMovementPositionupdates the player's stored location as a side effect, which is unexpected for a method named "Get...". Consider either renaming toUpdateAndGetMovementPositionor documenting this behavior clearly, as callers may not expect a query to mutate state.game/src/movement.rs (2)
82-84: Consider rounding instead of truncating.The
as i32cast truncates the interpolated value. For more accurate positioning, consider using.round()before casting:- let current_x = self.source_x + ((self.dest_x - self.source_x) as f64 * progress) as i32; - let current_y = self.source_y + ((self.dest_y - self.source_y) as f64 * progress) as i32; - let current_z = self.source_z + ((self.dest_z - self.source_z) as f64 * progress) as i32; + let current_x = self.source_x + ((self.dest_x - self.source_x) as f64 * progress).round() as i32; + let current_y = self.source_y + ((self.dest_y - self.source_y) as f64 * progress).round() as i32; + let current_z = self.source_z + ((self.dest_z - self.source_z) as f64 * progress).round() as i32;
128-136: Test has timing dependency.
test_calculate_current_position_at_startchecks position immediately after creation but allows for "small movement" (x < 50). This test could be flaky on slow CI systems. Consider either:
- Testing with zero speed (instant arrival)
- Using a mock time source
- Accepting this as sufficient for the current use case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
game/src/main.rs(1 hunks)game/src/movement.rs(1 hunks)game/src/packets/from_client/move_to_location.rs(2 hunks)game/src/packets/from_client/stop_move.rs(3 hunks)game/src/pl_client.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
game/src/pl_client.rs (1)
game/src/movement.rs (1)
new(30-50)
game/src/packets/from_client/move_to_location.rs (3)
game/src/packets/from_client/stop_move.rs (2)
read(14-16)handle(23-42)game/src/movement.rs (2)
new(30-50)has_arrived(90-94)game/src/packets/to_client/char_move_to_location.rs (1)
new(16-34)
game/src/movement.rs (3)
game/src/pl_client.rs (5)
new(82-97)handle(463-474)handle(483-490)handle(496-499)handle(508-514)game/src/packets/from_client/move_to_location.rs (2)
handle(44-118)handle(128-146)game/src/packets/from_client/stop_move.rs (1)
handle(23-42)
🔇 Additional comments (5)
game/src/main.rs (1)
21-21: LGTM!Module declaration correctly wires the new movement subsystem into the game binary.
game/src/packets/from_client/stop_move.rs (1)
28-37: LGTM with a minor observation.The handler correctly stops movement and updates position. Consider whether silently ignoring
set_locationerrors (line 34) is intentional - if the character lookup fails, no position update occurs and no error is logged. This may be acceptable for a stop-move packet but worth noting.game/src/pl_client.rs (2)
322-359: Well-structured movement initialization.The
start_movementmethod correctly:
- Cancels any existing movement before starting new one (handles rerouting)
- Calculates current interpolated position when rerouting
- Selects appropriate speed based on run/walk state
One consideration: if
try_get_selected_char()fails (line 339), movement_state has already been taken (line 329) but won't be restored. This seems acceptable since a missing character indicates a more fundamental problem.
361-369: LGTM!Clean implementation of
stop_movementwith proper task cancellation and position interpolation.game/src/movement.rs (1)
104-108: LGTM!Good use of
Droptrait to ensure task cleanup whenMovementStateis dropped, preventing orphaned broadcast tasks.
| // Store the task handle in movement state | ||
| if let Some(movement) = self.movement_state.as_mut() { | ||
| movement.task_handle = Some(task_handle); | ||
| } |
There was a problem hiding this comment.
Race condition: task handle stored after spawn.
There's a window between spawning the task (line 72) and storing its handle (line 114) where the task could complete. For very short movements, the task might finish before the handle is stored. Consider restructuring to set the handle atomically with movement creation, or accept this as benign since the task skips the first tick.
🤖 Prompt for AI Agents
In game/src/packets/from_client/move_to_location.rs around lines 112–115 there
is a race where the task is spawned before its JoinHandle is stored in
movement_state; a very short task can complete before the handle is saved. Fix
by making the spawned task wait on a small start signal so you can store the
handle first: create a tokio::sync::oneshot channel, spawn the task passing the
receiver and immediately set movement.task_handle = Some(join_handle) in
movement_state, then send the start signal on the sender so the task begins;
this ensures the handle is stored before the task can complete.
… unused `ActorRef` import.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
game/src/packets/from_client/move_to_location.rs (1)
114-117: Race condition: task handle stored after spawn.This issue was flagged in a previous review. There's a window between spawning the task (line 72) and storing its handle (line 114) where a very short movement could complete before the handle is stored. While the first tick is skipped, this is still a theoretical race. Consider using a oneshot channel to synchronize, or accept this as benign for the typical use case.
🧹 Nitpick comments (6)
game/src/movement.rs (2)
69-86: Potential truncation issues in position interpolation.The cast
(... as f64 * progress) as i32may lose precision and behave unexpectedly for large coordinate differences. Using.round()before casting would provide more accurate interpolation.- let current_x = self.source_x + ((self.dest_x - self.source_x) as f64 * progress) as i32; - let current_y = self.source_y + ((self.dest_y - self.source_y) as f64 * progress) as i32; - let current_z = self.source_z + ((self.dest_z - self.source_z) as f64 * progress) as i32; + let current_x = self.source_x + ((self.dest_x - self.source_x) as f64 * progress).round() as i32; + let current_y = self.source_y + ((self.dest_y - self.source_y) as f64 * progress).round() as i32; + let current_z = self.source_z + ((self.dest_z - self.source_z) as f64 * progress).round() as i32;
109-142: Test coverage looks good, consider adding edge case tests.The tests cover the basic scenarios well. Consider adding tests for:
- Negative coordinates
- Very large distances (to catch potential overflow)
- Movement mid-progress (using a mock or adjusted start_time)
game/src/packets/from_client/move_to_location.rs (4)
72-112: Consider adding a timeout or maximum iteration guard.The spawned task loops indefinitely until arrival or cancellation. If
has_arrivednever returns true due to a bug (e.g., speed is 0 but distance > 0), this task would loop forever. Consider adding a maximum duration check or iteration count as a safety net.let task_handle = tokio::spawn(async move { let mut tick = interval(Duration::from_millis(300)); tick.tick().await; // Skip first immediate tick + let start = std::time::Instant::now(); + let max_duration = Duration::from_secs(300); // 5 minute safety limit loop { + if start.elapsed() > max_duration { + warn!("Movement task exceeded maximum duration, stopping"); + break; + } + // Check movement state BEFORE waiting...
127-148: Side effect in a query message handler.
GetMovementPositionis semantically a query (get position), but it also mutates state viaplayer.set_location(x, y, z). This coupling of read and write operations can make the code harder to reason about and test. Consider separating the position update into a distinct message or documenting this behavior explicitly.If keeping the current design, at minimum add a doc comment clarifying the side effect:
/// Message to get current movement position and status +/// Note: This also updates the player's actual position as a side effect #[derive(Debug)] pub struct GetMovementPosition;
140-142: Silently ignoringset_locationerrors.The
let _ = player.set_location(x, y, z);pattern discards any error. If setting location can fail, consider logging the error to aid debugging.// Update player's actual position if let Ok(player) = self.try_get_selected_char_mut() { - let _ = player.set_location(x, y, z); + if let Err(e) = player.set_location(x, y, z) { + warn!("Failed to update player location: {}", e); + } }
91-96: Double actor message round-trip per tick.Each iteration sends two
askmessages:GetMovementPositionandGetCharInfo. The position query already has access toselfand could return the player info needed for the packet, reducing latency and actor contention.Consider extending
GetMovementPositionto return the object ID directly, or create a combined message that returns all needed data in one round-trip.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
game/src/movement.rs(1 hunks)game/src/packets/from_client/move_to_location.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
game/src/packets/from_client/move_to_location.rs (2)
game/src/movement.rs (2)
new(29-49)has_arrived(89-93)game/src/packets/to_client/char_move_to_location.rs (1)
new(16-34)
game/src/movement.rs (2)
game/src/pl_client.rs (5)
new(82-97)handle(463-474)handle(483-490)handle(496-499)handle(508-514)game/src/packets/from_client/move_to_location.rs (2)
handle(44-120)handle(130-148)
🔇 Additional comments (4)
game/src/movement.rs (3)
1-25: LGTM! Clean struct design.The
MovementStatestruct is well-documented and captures all necessary state for tracking player movement. The public fields allow direct access when needed while thetask_handlebeingOption<JoinHandle<()>>correctly models the optional async task.
88-93: Edge case:has_arrivedreturns true immediately when duration is 0.When
speed > 0but source equals destination,calculate_travel_duration()returns 0, andelapsed >= 0.0is always true. This is correct behavior (covered by test), but worth noting for clarity.
103-107: Good use of Drop for cleanup.The
Dropimplementation ensures the broadcast task is aborted whenMovementStateis dropped, preventing orphaned tasks. This is a solid pattern for resource cleanup.game/src/packets/from_client/move_to_location.rs (1)
1-10: Imports look appropriate for the new functionality.The added imports support the actor-based movement tracking, interval timing, and logging requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
game/src/packets/from_client/move_to_location.rs (1)
159-162: Race condition between task spawn and handle storage.This concern was raised in a previous review. The task is spawned before the handle is stored, creating a window where short movements could complete before the handle is saved.
game/src/movement.rs (1)
51-57: Potential overflow in distance calculation - inconsistent withmove_to_location.rs.This was flagged in a previous review. Note that
move_to_location.rs(lines 14-24) now useschecked_subfor overflow protection, but this method still performs unchecked subtraction. Consider aligning both implementations.Apply this diff to match the safer pattern:
pub fn calculate_distance(&self) -> f64 { - let dx = f64::from(self.dest_x - self.source_x); - let dy = f64::from(self.dest_y - self.source_y); - let dz = f64::from(self.dest_z - self.source_z); + let dx = f64::from(self.dest_x) - f64::from(self.source_x); + let dy = f64::from(self.dest_y) - f64::from(self.source_y); + let dz = f64::from(self.dest_z) - f64::from(self.source_z); (dx * dx + dy * dy + dz * dz).sqrt() }
🧹 Nitpick comments (2)
game/src/packets/from_client/move_to_location.rs (2)
99-99: Unused return value fromstart_movement.The tuple
(source_x, source_y, source_z)returned bystart_movementis assigned but never used. Consider prefixing with underscore or removing the binding.- let (source_x, source_y, source_z) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?; + let _ = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?;
117-157: Consider logging failures in the broadcast loop.When
actor_ref.ask(GetCharInfo)fails at line 137, the error is silently ignored and the loop continues. While this prevents the task from crashing, repeated failures could indicate a problem. Consider adding a debug/trace log for visibility.// Broadcast current position - if let Ok(player) = actor_ref.ask(crate::pl_client::GetCharInfo).await { + match actor_ref.ask(crate::pl_client::GetCharInfo).await { + Ok(player) => { if let Ok(packet) = CharMoveToLocation::new(&player, dest_x, dest_y, dest_z) { controller.broadcast_packet(packet); } + } + Err(e) => { + tracing::trace!("Failed to get char info during movement broadcast: {}", e); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/game.yaml(1 hunks)game/src/movement.rs(1 hunks)game/src/packets/from_client/move_to_location.rs(2 hunks)l2-core/src/config/gs.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/game.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
game/src/movement.rs (1)
game/src/packets/from_client/move_to_location.rs (3)
calculate_distance(15-25)handle(59-165)handle(175-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-only
🔇 Additional comments (7)
l2-core/src/config/gs.rs (1)
51-64: LGTM!The configuration addition follows the established patterns in this file. The
max_movement_distancefield with a default of 15000 integrates cleanly with the existing serde deserialization approach.game/src/packets/from_client/move_to_location.rs (2)
13-25: LGTM!Good use of
checked_subto handle potential overflow cases. The function safely returnsNonewhen coordinate subtraction would overflow, which is then properly handled by the caller.
172-193: LGTM with note on side effect.The handler updates the player's position as a side effect of querying movement state (lines 185-187). This coupling ensures position synchronization during movement. The design is acceptable, though worth documenting that callers should be aware position is mutated on each query.
game/src/movement.rs (4)
4-25: LGTM!The struct design is appropriate for tracking movement state. Using
Instantfor timing andOption<JoinHandle<()>>for task management is idiomatic Rust.
27-49: LGTM!Clean constructor that properly initializes the movement state with the current instant as start time.
103-107: LGTM!The
Dropimplementation ensures proper cleanup of the broadcast task, preventing resource leaks whenMovementStateis dropped.
109-142: Good test coverage.The tests validate core movement calculations including distance (3-4-5 triangle), travel duration, initial position, and instant arrival edge case.
… overflow and ensure precise calculations.
…ement broadcast loop logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
game/src/packets/from_client/move_to_location.rs (1)
149-151: Race condition acknowledged.The race between spawning the task and storing its handle was flagged in a previous review. For typical movement durations, this is benign since the task's first action involves an async
askthat yields.
🧹 Nitpick comments (4)
game/src/packets/from_client/move_to_location.rs (3)
9-11: Unused import:intervalis referenced inline.
intervalfromtokio::timeis imported but line 118 usestokio::time::intervaldirectly. Either use the import or remove it.use std::time::Duration; -use tokio::time::interval; use tracing::{info, instrument, warn};
99-99: Unused variables will trigger compiler warnings.
source_x,source_y,source_zare returned but never used. Prefix with underscore or use..pattern to suppress warnings.- let (mut movement, source_x, source_y, source_z) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?; + let (mut movement, ..) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?;
173-176: Consider logging or propagatingset_locationerrors.Silently discarding the result could mask position update failures, potentially causing desync between movement interpolation and stored player position.
// Update player's actual position if let Ok(player) = self.try_get_selected_char_mut() { - let _ = player.set_location(x, y, z); + if let Err(e) = player.set_location(x, y, z) { + warn!("Failed to update player position: {:?}", e); + } }game/src/pl_client.rs (1)
417-431: Missing movement task cleanup on actor stop.If the player disconnects while moving, the spawned broadcast task continues running until
actor_ref.askfails. Consider canceling the movement task explicitly inon_stopfor cleaner shutdown.async fn on_stop( &mut self, _actor_ref: WeakActorRef<Self>, _reason: ActorStopReason, ) -> anyhow::Result<()> { info!("Disconnecting Client..."); + // Cancel any ongoing movement task + if let Some(mut movement) = self.movement_state.take() { + movement.cancel_task(); + } if let Some(s) = self.packet_sender.as_ref() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
game/src/packets/from_client/move_to_location.rs(2 hunks)game/src/pl_client.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
game/src/packets/from_client/move_to_location.rs (2)
game/src/movement.rs (2)
calculate_distance(52-57)new(29-49)game/src/packets/to_client/char_move_to_location.rs (1)
new(16-34)
game/src/pl_client.rs (1)
game/src/movement.rs (1)
new(29-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-only
🔇 Additional comments (5)
game/src/packets/from_client/move_to_location.rs (2)
13-25: LGTM!Overflow-safe distance calculation with proper
checked_subusage.
68-96: LGTM!Distance validation with overflow protection and configurable max distance check is well implemented.
game/src/pl_client.rs (3)
70-70: LGTM!Adding
movement_statefield to track ongoing player movement is appropriate.
322-358: LGTM with minor note.The method correctly manages movement lifecycle. The design of returning
MovementStatewithout storing allows the caller to set thetask_handlebefore storage, which is used inmove_to_location.rs.
361-368: LGTM!Clean implementation of movement cancellation with position retrieval.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
game/src/packets/from_client/move_to_location.rs (1)
110-144: Race condition between task spawn and movement state storage.The task spawns at line 110 but
self.movement_stateisn't set until line 144. When the task immediately callsGetMovementPosition, it will findmovement_stateasNoneand exit. This effectively breaks the movement system.Consider setting
movement_statebefore spawning the task, then updating it with the handle afterward:+ // Set movement state first so task can find it + self.movement_state = Some(movement); + let task_handle = tokio::spawn(async move { // ... task body ... }); - // Store the task handle in movement state and set it in client - movement.task_handle = Some(task_handle); - self.movement_state = Some(movement); + // Store task handle in existing movement state + if let Some(ref mut m) = self.movement_state { + m.task_handle = Some(task_handle); + }
🧹 Nitpick comments (3)
game/src/packets/from_client/move_to_location.rs (3)
9-11: Unused imports.
Durationandintervalare imported but the code uses fully qualified paths (std::time::Duration::from_millisandtokio::time::interval) on line 111 instead.Either use the imports:
- let mut interval = tokio::time::interval(std::time::Duration::from_millis(300)); + let mut interval = interval(Duration::from_millis(300));Or remove the unused imports:
-use std::time::Duration; -use tokio::time::interval;
99-99: Unused variables fromstart_movementreturn.The
source_x,source_y,source_zvalues are extracted but never used. If they're no longer needed, consider updatingstart_movementto not return them, or prefix with underscore.- let (mut movement, source_x, source_y, source_z) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?; + let (mut movement, _source_x, _source_y, _source_z) = self.start_movement(msg.x_to, msg.y_to, msg.z_to)?;
162-174: Side effect in getter message handler.
GetMovementPositionboth queries the current position and mutates the player's location (lines 167-169). This couples reading and writing, which can lead to unexpected behavior if the message is used elsewhere for read-only purposes.Consider separating concerns:
- Have
GetMovementPositiononly return the interpolated position without mutation- Update the player's position when movement completes or in a dedicated update path
Alternatively, rename to
UpdateAndGetMovementPositionto make the side effect explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
game/src/packets/from_client/enter_world.rs(1 hunks)game/src/packets/from_client/move_to_location.rs(2 hunks)game/src/packets/from_client/restart.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
game/src/packets/from_client/move_to_location.rs (2)
game/src/movement.rs (3)
calculate_distance(52-57)has_arrived(89-93)new(29-49)game/src/packets/to_client/char_move_to_location.rs (1)
new(16-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-only
🔇 Additional comments (5)
game/src/packets/from_client/enter_world.rs (1)
62-62: LGTM!Stopping movement before transitioning to InGame ensures a clean state when the player enters the world, preventing any residual movement from causing issues.
game/src/packets/from_client/restart.rs (1)
31-31: LGTM!Stopping movement at the start of restart handling ensures proper cleanup before character state transitions and broadcasting
DeleteObjectto other players.game/src/packets/from_client/move_to_location.rs (3)
13-25: LGTM!The overflow-safe distance calculation using
checked_subis a good defensive measure for validating client-supplied coordinates before trusting them.
68-96: LGTM!The distance validation with overflow detection and max distance checking provides good protection against malicious movement requests. Returning silently (rather than erroring) on excessive distance is appropriate.
113-139: Loop structure is correct.The broadcast logic is properly structured: immediate first broadcast followed by 300ms intervals. The previous concern about duplicate broadcasts no longer applies since there's no initial packet send outside the task.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.