Uh oh!
There was an error while loading. Please reload this page.
Feat: Patrols with AirWaypoint - #434
Conversation
WalkthroughThe changes introduce support for air waypoints and flying movement in the game server's NPC navigation system. The waypoint data structure ( Changes
Sequence Diagram(s)sequenceDiagram
participant GameServer
participant FieldNpc
participant MovementState
participant AgentNavigation
GameServer->>FieldNpc: NextWaypoint()
alt AirWayPoint
FieldNpc->>MovementState: TryFlyTo(position, isBattle, sequence, speed, lookAt)
MovementState->>MovementState: Create NpcFlyToTask
MovementState->>MovementState: FlyTo(task, position, sequence, speed, lookAt)
MovementState->>AgentNavigation: FlyAdvance(start, target, speed, deltaTime)
AgentNavigation-->>MovementState: (newPosition, reachedTarget)
else Ground WayPoint
FieldNpc->>MovementState: TryMoveTo(...)
MovementState->>...: (existing walking logic)
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Maple2.Model/Metadata/MapEntity/PatrolData.cs (1)
22-23: Added air waypoint support to the data modelThe new
AirWayPointparameter enables waypoints to be flagged for flying movement, which is essential for the air waypoint feature.For better naming consistency, consider aligning the parameter name with
MS2PatrolData.IsAirWayPointto eitherbool IsAirWayPointor rename theIsAirWayPointfield inMS2PatrolDatatoAirWayPoint.Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
276-284: Improve error message for air waypoints.When no flying animation is found, the error message refers to "walk sequence" which is confusing for air waypoints.
- Logger.Warning("No walk sequence found for npc {NpcId} in patrol {PatrolId}", Value.Metadata.Id, Patrol.Uuid);+ Logger.Warning("No fly sequence found for npc {NpcId} in patrol {PatrolId}", Value.Metadata.Id, Patrol.Uuid);Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateStates/MovementState.Walk.cs (1)
118-136: Flying movement implementation is clear and effective.The flying advance logic correctly uses direct linear movement for flying NPCs, with appropriate handling of velocity, rotation, and task completion. Consider adding collision detection for flying NPCs in a future update to prevent them from flying through obstacles.
In the future, you might want to implement collision detection for flying NPCs to prevent them from flying through solid objects, similar to how walking NPCs navigate around obstacles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Maple2.File.Ingest/Mapper/MapEntityMapper.cs(1 hunks)Maple2.Model/Metadata/MapEntity/PatrolData.cs(1 hunks)Maple2.Server.Game/Manager/Field/AgentNavigation.cs(2 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateStates/MovementState.Walk.cs(4 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.Game/Manager/Field/AgentNavigation.cs (1)
Maple2.Tools/DotRecast/DotRecastHelper.cs (1)
Vector3(46-49)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (8)
Maple2.File.Ingest/Mapper/MapEntityMapper.cs (1)
228-228: Implementation correctly propagates air waypoint flagThe change correctly passes the
patrolData.IsAirWayPointflag to theMS2WayPointconstructor, ensuring that waypoints are appropriately marked for air navigation.Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (1)
75-86: Well-structured implementation for flying movementThe new
TryFlyTomethod follows the same pattern as other movement methods, providing a clean interface for initiating flying movement with appropriate task priority handling and parameter propagation.Maple2.Server.Game/Manager/Field/AgentNavigation.cs (1)
224-243: Well-implemented linear movement for flyingThe
FlyAdvancemethod provides a clean implementation for linear flying movement, correctly handling edge cases and providing a straightforward calculation for position updates. The documentation is clear about the method's purpose and parameters.Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs (2)
79-103: Well-implemented flying task pattern!The
NpcFlyToTaskclass correctly follows the established pattern for NPC movement tasks, maintaining consistency with the existing codebase. Good job using the "Fly_A" animation for idle states when paused or finished.
137-158: The FlyTo implementation looks good.This follows the same pattern as MoveTo but correctly calls StartFlying instead of StartWalking. The implementation appropriately handles validation checks before initiating flying movement.
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
78-78: Good addition of FlySequence field.The FlySequence field properly matches the pattern established for other animation sequences.
284-296: Good ground movement validation.The implementation correctly checks if a path exists before creating a movement task, which prevents NPCs from attempting to move to unreachable locations. The warning message is appropriately descriptive.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateStates/MovementState.Walk.cs (1)
60-82: StartFlying method matches established patterns.The implementation mirrors the existing StartWalking method with appropriate adaptations for flying. Consider potential refactoring in the future to reduce code duplication between these two similar methods.
Uh oh!
There was an error while loading. Please reload this page.
131f5ba to
f40fb4eCompareThere was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
262-268: Handle potential null reference in patrol waypoint access.The code correctly gets the previous waypoint, but there's a risk of a null reference if the patrol data is unexpectedly null.
- MS2WayPoint currentWaypoint = Patrol!.WayPoints[currentWaypointIndex];+ if (Patrol == null || Patrol.WayPoints.Count == 0) {+ return MovementState.TryStandby(null, true);+ }++ MS2WayPoint currentWaypoint = Patrol.WayPoints[currentWaypointIndex];
🧹 Nitpick comments (2)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
278-298: Consider refactoring to reduce code duplication in animation handling.The approach animation handling logic is duplicated between air and ground waypoints. Consider extracting this common logic to reduce duplication.
NpcTask? approachTask = null; + AnimationSequenceMetadata? sequenceToUse = null;++ // Try to get approach animation from waypoint+ if (!string.IsNullOrEmpty(currentWaypoint.ApproachAnimation) && + Value.Animations.TryGetValue(currentWaypoint.ApproachAnimation, out sequenceToUse)) {+ // Use the approach animation defined in the waypoint+ } else if (currentWaypoint.AirWayPoint && FlySequence is not null) {+ sequenceToUse = FlySequence;+ } else if (!currentWaypoint.AirWayPoint && WalkSequence is not null) {+ sequenceToUse = WalkSequence;+ }++ if (sequenceToUse is null) {+ Logger.Warning("No {Type} sequence found for npc {NpcId} in patrol {PatrolId}", + currentWaypoint.AirWayPoint ? "fly" : "walk", Value.Metadata.Id, Patrol.Uuid);+ } if (currentWaypoint.AirWayPoint) { - if (Value.Animations.TryGetValue(currentWaypoint.ApproachAnimation, out AnimationSequenceMetadata? patrolSequence)) {- approachTask = MovementState.TryFlyTo(currentWaypoint.Position, false, sequence: patrolSequence.Name, speed: Patrol.PatrolSpeed, lookAt: true);- } else if (FlySequence is not null) {- approachTask = MovementState.TryFlyTo(currentWaypoint.Position, false, sequence: FlySequence.Name, speed: Patrol.PatrolSpeed, lookAt: true);- } else {- Logger.Warning("No walk sequence found for npc {NpcId} in patrol {PatrolId}", Value.Metadata.Id, Patrol.Uuid);- }+ if (sequenceToUse is not null) {+ approachTask = MovementState.TryFlyTo(currentWaypoint.Position, false, sequence: sequenceToUse.Name, speed: Patrol.PatrolSpeed, lookAt: true);+ } } else { if (Navigation is not null && Navigation.PathTo(currentWaypoint.Position)) { - if (Value.Animations.TryGetValue(currentWaypoint.ApproachAnimation, out AnimationSequenceMetadata? patrolSequence)) {- approachTask = MovementState.TryMoveTo(currentWaypoint.Position, false, sequence: patrolSequence.Name, speed: Patrol.PatrolSpeed);- } else if (WalkSequence is not null) {- approachTask = MovementState.TryMoveTo(currentWaypoint.Position, false, sequence: WalkSequence.Name, speed: Patrol.PatrolSpeed);- } else {- Logger.Warning("No walk sequence found for npc {NpcId} in patrol {PatrolId}", Value.Metadata.Id, Patrol.Uuid);- }+ if (sequenceToUse is not null) {+ approachTask = MovementState.TryMoveTo(currentWaypoint.Position, false, sequence: sequenceToUse.Name, speed: Patrol.PatrolSpeed);+ } } else { Logger.Warning("Failed to path to waypoint id({Id}) coord {Coord} for npc {NpcName} - {NpcId} in patrol {PatrolId}", currentWaypoint.Id, currentWaypoint.Position, Value.Metadata.Name, Value.Metadata.Id, Patrol.Uuid); } }
280-280: Consider making lookAt parameter consistent for both movement modes.The
TryFlyTomethod useslookAt: truebut theTryMoveTomethod doesn't include this parameter. For consistency, consider either adding it to ground movement or documenting why the behaviors differ.- approachTask = MovementState.TryMoveTo(currentWaypoint.Position, false, sequence: patrolSequence.Name, speed: Patrol.PatrolSpeed);+ approachTask = MovementState.TryMoveTo(currentWaypoint.Position, false, sequence: patrolSequence.Name, speed: Patrol.PatrolSpeed, lookAt: true);Also applies to: 289-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Maple2.File.Ingest/Mapper/MapEntityMapper.cs(1 hunks)Maple2.Model/Metadata/MapEntity/PatrolData.cs(1 hunks)Maple2.Server.Game/Manager/Field/AgentNavigation.cs(2 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateStates/MovementState.Walk.cs(4 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Maple2.File.Ingest/Mapper/MapEntityMapper.cs
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs
- Maple2.Server.Game/Manager/Field/AgentNavigation.cs
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateStates/MovementState.Walk.cs
- Maple2.Model/Metadata/MapEntity/PatrolData.cs
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (3)
78-78: LGTM: Flying animation support properly added.The implementation adds a nullable
FlySequencefield and initializes it from the NPC's animation dictionary, following the same pattern as other animation types.Also applies to: 102-102
270-274: LGTM: Improved arrival animation handling.The code now correctly checks the previous waypoint's
ArriveAnimationand ensures the idle task isn't an emote task or null before playing the animation.
278-283: LGTM: Well-implemented air waypoint movement logic.The implementation correctly handles air waypoints by using the new
TryFlyTomethod with appropriate fallback to the generalFlySequenceif no specific approach animation is defined.
Uh oh!
There was an error while loading. Please reload this page.
This reverts commit 0e5c339.
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements