From 1bfb47e93703b4b6fb3dcfaf28ed949af69dba12 Mon Sep 17 00:00:00 2001 From: bash Date: Thu, 14 May 2026 22:38:12 +0200 Subject: [PATCH] chore: Tighten comments in travel and movement code --- src/Ai/Base/Actions/BattleGroundTactics.cpp | 2 +- src/Ai/Base/Actions/MovementActions.cpp | 43 +++------ src/Ai/World/Rpg/Action/NewRpgBaseAction.cpp | 93 ++++++-------------- src/Mgr/Travel/TravelNode.cpp | 24 ++--- src/Mgr/Travel/TravelNode.h | 15 +--- 5 files changed, 51 insertions(+), 126 deletions(-) diff --git a/src/Ai/Base/Actions/BattleGroundTactics.cpp b/src/Ai/Base/Actions/BattleGroundTactics.cpp index 238a2291e..26271446b 100644 --- a/src/Ai/Base/Actions/BattleGroundTactics.cpp +++ b/src/Ai/Base/Actions/BattleGroundTactics.cpp @@ -173,7 +173,7 @@ std::vector const vFlagsIC = {GO_HORDE_BANNER, GO_HORDE_BANNER_GRAVEYARD_H, GO_HORDE_BANNER_GRAVEYARD_H_CONT}; -// BG Waypoints (vmangos) +// BG Waypoints // Horde Flag Room to Horde Graveyard BattleBotPath vPath_WSG_HordeFlagRoom_to_HordeGraveyard = { diff --git a/src/Ai/Base/Actions/MovementActions.cpp b/src/Ai/Base/Actions/MovementActions.cpp index d7b4de023..f0f5fb522 100644 --- a/src/Ai/Base/Actions/MovementActions.cpp +++ b/src/Ai/Base/Actions/MovementActions.cpp @@ -386,14 +386,8 @@ bool MovementAction::MoveTo(uint32 mapId, float x, float y, float z, bool idle, else { // Direct dispatch — engine MovePoint(generatePath=true) handles - // path-finding internally. Previously called SearchForBestPath - // here to probe ±step around the target z; that helped find - // polygons when the input z was several yards off the navmesh, - // but its "shortest path" preference would shift modifiedZ to - // an unreachable nearby polygon (upper terrace, ledge above) - // and then the engine's straight-spline NOPATH fallback would - // air-walk the bot up to it. cmangos doesn't have an - // equivalent — single-z PathFinder call is sufficient. + // pathfinding. Avoid ±z probes: their "shortest path" preference + // can pick an unreachable ledge and air-walk via NOPATH fallback. float distance = bot->GetExactDist(x, y, z); if (distance > 0.01f) { @@ -1179,7 +1173,7 @@ void MovementAction::UpdateMovementState() // { // bot->SetSpeedRate(MOVE_RUN, 1.0f); // } - // check if target is not reachable (from Vmangos) + // check if target is not reachable // if (bot->GetMotionMaster()->GetCurrentMovementGeneratorType() == CHASE_MOTION_TYPE && bot->CanNotReachTarget() && // !bot->InBattleground()) // { @@ -3221,20 +3215,12 @@ bool MovementAction::RefineWalkPoints(std::vector& walkPoints) WorldPosition aPos(mapId, a.x, a.y, a.z); WorldPosition bPos(mapId, b.x, b.y, b.z); - // Per-segment mmap query against the live navmesh. The - // travel-node graph stores offline-baked waypoints; if the - // straight line A->B crosses geometry the live navmesh has - // (mountain, ledge, model edit since offline gen), this - // returns either an mmap-routed path around it (NORMAL/ - // INCOMPLETE) or empty (NOT_USING_PATH was rejected as - // "would walk through walls"). + // Per-segment mmap query: routes around geometry the offline + // graph didn't account for, or returns empty if unreachable. std::vector segPath = bPos.getPathStepFrom(aPos, bot); - // Travelnode waypoints are authoritative once a plan is - // active. When AC mmap can't validate the segment, dispatch - // the raw (A, B) pair instead of aborting the plan. Common - // cases: stored waypoints landing in 1y navmesh gaps from - // extractor differences, tile-edge artifacts at zone borders. + // Trust the raw waypoint pair when mmap can't validate it — + // navmesh gaps/tile-edge artifacts shouldn't kill an active plan. bool const trustRaw = segPath.empty() || TravelPath::IsPathCheating(segPath, aPos.distance(bPos)); @@ -3246,9 +3232,8 @@ bool MovementAction::RefineWalkPoints(std::vector& walkPoints) continue; } - // First segment: include its start point so the spline - // begins from the original A. Later segments: skip the first - // point — it duplicates the previous segment's tail. + // Include the first segment's start; skip subsequent starts + // to avoid duplicating the prior segment's tail. size_t startK = (i == 0) ? 0 : 1; for (size_t k = startK; k < segPath.size(); ++k) refined.emplace_back(segPath[k].GetPositionX(), @@ -3471,14 +3456,8 @@ bool MovementAction::ExecuteTravelPlan(TravelPlan& state) return true; } - // Re-validate each consecutive (A, B) pair against the - // live navmesh. The graph's offline-baked coords can - // produce a chain whose straight-line interpolation - // passes through geometry (mountains, ledges, model - // edits). RefineWalkPoints substitutes mmap-routed - // sub-paths between each pair; if any segment is - // unwalkable, abort the plan so MoveFarTo's own probe - // can re-derive a route. + // Re-validate each segment against the live navmesh and + // substitute mmap-routed sub-paths where needed. if (!RefineWalkPoints(state.walkPoints)) { G3D::Vector3 const& failPt = state.walkPoints.empty() diff --git a/src/Ai/World/Rpg/Action/NewRpgBaseAction.cpp b/src/Ai/World/Rpg/Action/NewRpgBaseAction.cpp index c66e8025a..372608e26 100644 --- a/src/Ai/World/Rpg/Action/NewRpgBaseAction.cpp +++ b/src/Ai/World/Rpg/Action/NewRpgBaseAction.cpp @@ -71,11 +71,8 @@ bool NewRpgBaseAction::MoveFarTo(WorldPosition dest) } } - // Let previously committed movement finish before recomputing. - // If the bot is still actively walking toward its last committed - // point on the same map, just let the current spline finish. - // Prevents oscillation when a re-resolve produces a slightly - // different partial-path endpoint mid-walk. + // Let an in-flight spline finish before recomputing — prevents + // oscillation when re-resolve produces a slightly different endpoint. { LastMovement& lastMove = AI_VALUE(LastMovement&, "last movement"); if (bot->isMoving() && lastMove.lastMoveToMapId == bot->GetMapId()) @@ -93,9 +90,9 @@ bool NewRpgBaseAction::MoveFarTo(WorldPosition dest) // 10% lastPath reuse — if the cached path's endpoint is still // close (within 10%) to the new dest, trim the cached path to // the bot's current position via makeShortCut and re-dispatch. - // Mirrors cmangos ResolveMovePath: per-tick re-dispatch of the - // (trimmed) last path keeps the bot on-route after interrupts - // (knockback, combat, manual move) without needing a full replan. + // Per-tick re-dispatch of the (trimmed) last path keeps the bot + // on-route after interrupts (knockback, combat, manual move) + // without needing a full replan. { LastMovement& lastMove = AI_VALUE(LastMovement&, "last movement"); if (!lastMove.lastPath.empty()) @@ -140,26 +137,16 @@ bool NewRpgBaseAction::MoveFarTo(WorldPosition dest) float disToDest = bot->GetDistance(dest); float dis = bot->GetExactDist(dest); - // Decision tree: - // 1. Active node plan with matching dest → ride it. - // 2. Long-distance / cross-map: try the node graph FIRST. - // Graph internally probes mmap and falls back to A* route. - // 3. Else: 40-step chained mmap probe + regression guard. - // 4. Empty / non-progressing probe: single-waypoint MoveTo. - // - // needsLongPath gate — cross-map or > 50y go to graph. - // BG gating: graph holds open-world routes only. + // Try the travel-node graph first for cross-map or > 50y moves; + // fall back to chained mmap probe otherwise. BGs skip the graph. constexpr float TRAVELNODE_THRESHOLD = 50.0f; bool tryNodes = sPlayerbotAIConfig.enableTravelNodes && !bot->InBattleground() && ((bot->GetMapId() != dest.GetMapId()) || (dis > TRAVELNODE_THRESHOLD)); - // If a node plan is already active, ride it — but only if its - // destination still matches the requested dest. Otherwise the - // old plan (e.g. built toward a quest objective POI) would keep - // driving the bot after the caller switched targets (e.g. to a - // turn-in NPC). + // Ride the active node plan only if its dest still matches. + // A stale plan would steer the bot past a new target. if (tryNodes && botAI->rpgInfo.HasActiveTravelPlan()) { if (botAI->rpgInfo.travelPlan.destination.distance(dest) > 10.0f) @@ -194,13 +181,8 @@ bool NewRpgBaseAction::MoveFarTo(WorldPosition dest) WorldPosition botPos(bot); std::vector probe = botPos.getPathTo(dest, bot); - // Regression guard (cmangos ResolveMovePath parity): if a cached - // lastPath ends at least as close to dest as the new probe's - // endpoint, prefer the cached path. The 10% reuse block above - // already returned early when cached was within 10% of dest; - // this catches "cached is far (>10%) but still better than the - // probe" — typically when the probe got blocked by geometry and - // ended much farther from dest than where cached had reached. + // Regression guard: prefer cached lastPath if it still ends closer + // to dest than the new probe — catches probes blocked by geometry. { LastMovement& lastMove = AI_VALUE(LastMovement&, "last movement"); if (!lastMove.lastPath.empty() && !probe.empty() && probe.size() >= 2) @@ -275,18 +257,13 @@ bool NewRpgBaseAction::MoveFarTo(WorldPosition dest) } } - // Empty-probe fallback — single-waypoint MoveTo. Engine - // MovePoint(generatePath=true) does the local PathGenerator - // resolution. Cross-map can't be served by a single-map spline - // — bail rather than dispatching toward unreachable coords. + // Empty-probe fallback: single-waypoint MoveTo via engine PathGenerator. + // Cross-map can't be served by a single-map spline — bail. if (bot->GetMapId() != dest.GetMapId()) return false; - // LOS gate — refuse to dispatch a straight-line spline when - // the dest isn't in line of sight. Engine PathGenerator may - // return a BuildShortcut 2-point line through visual obstacles - // (trees, walls) when start/end polyref resolution fails. Let - // UnstuckAction handle prolonged stuck states. + // LOS gate: don't air-walk through trees/walls when the engine + // would otherwise drop to a straight-line BuildShortcut spline. if (!bot->IsWithinLOS(dest.GetPositionX(), dest.GetPositionY(), dest.GetPositionZ())) { EmitDebugMove("MoveFar", "spline-blocked", @@ -307,9 +284,8 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, if (points.size() < 2) return false; - // Save planner output BEFORE any clip/fixup mutation so next-tick - // reuse/regress branches see the original intent, not a clip- - // truncated tail. + // Save planner output before clip/fixup so next-tick reuse sees + // the original intent, not a truncated tail. { LastMovement& lm = AI_VALUE(LastMovement&, "last movement"); std::vector wpts; @@ -319,9 +295,8 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, lm.setPath(TravelPath(wpts)); } - // Underwater fixup — push waypoints submerged below the water - // surface up to the surface itself, unless the destination is - // itself underwater. + // Underwater fixup: lift submerged waypoints to the surface, + // unless the destination is itself underwater. if (Map* map = bot->GetMap()) { WorldPosition destWp = dest; @@ -391,10 +366,8 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, LastMovement& lastMove = AI_VALUE(LastMovement&, "last movement"); - // Inactive-bot teleport — when the path is longer than reactDistance - // and no real player is around to witness, jump to the path tail and - // schedule a cooldown. Skips cosmetic walking for unobserved random - // bots. Self-bots are excluded so observed sessions always walk. + // Skip cosmetic walking for random bots with no nearby player — + // teleport to the path tail and schedule a cooldown instead. if (sRandomPlayerbotMgr.IsRandomBot(bot)) { WorldPosition tail(dest.GetMapId(), last.x, last.y, last.z); @@ -417,9 +390,7 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, } } - // masterWalking — match the master's walk pace when they're nearby - // and walking. Lets a follower bot trail at walk speed instead of - // sprinting past. No-op for masterless RPG bots. + // Match master's walk pace when they're nearby and walking. ForcedMovement moveMode = FORCED_MOVEMENT_RUN; if (sPlayerbotAIConfig.walkDistance > 0.0f) { @@ -433,8 +404,7 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, } } - // Pre-dispatch state cleanup. Clear emote / stand up / interrupt - // any non-melee cast so the spline can begin without state conflicts. + // Clear emote/sit/cast so the spline can begin cleanly. bot->ClearEmoteState(); if (!bot->IsStandState()) bot->SetStandState(UNIT_STAND_STATE_STAND); @@ -446,9 +416,7 @@ bool NewRpgBaseAction::DispatchPathPoints(WorldPosition const& dest, EmitDebugMove("MoveFar", label, last.x, last.y, last.z); - // WaitForReach scheduling. - // waitDist = (totalDist > reactDistance) ? totalDist - 10 : totalDist - // duration = 1000 * (waitDist / speed) + reactDelay, capped at maxWaitForMove + // WaitForReach: leave ~10y headroom on long paths. float waitDist = totalDist > sPlayerbotAIConfig.reactDistance ? std::max(totalDist - 10.0f, 0.0f) : totalDist; UnitMoveType const speedType = (moveMode == FORCED_MOVEMENT_WALK) ? MOVE_WALK : MOVE_RUN; @@ -513,11 +481,8 @@ bool NewRpgBaseAction::MoveWorldObjectTo(ObjectGuid guid, float distance) y = object->GetPositionY(); z = object->GetPositionZ(); } - // Delegate to MoveFarTo so every approach gets the chained mmap - // probe + spellDistance shortcut + travel-node fallback instead - // of a single direct MoveTo. The debug-move trace then labels - // the actual mechanism (spline / mmap / nodetravel) rather than - // a generic "MoveWorldObjectTo:spline". + // Route through MoveFarTo so every approach gets the full probe + // + travel-node fallback (and a precise debug label). return MoveFarTo(WorldPosition(object->GetMapId(), x, y, z)); } @@ -530,11 +495,7 @@ bool NewRpgBaseAction::MoveRandomNear(float moveStep, MovementPriority priority, const float x = bot->GetPositionX(); const float y = bot->GetPositionY(); const float z = bot->GetPositionZ(); - // Previously: attempts = 1. A single random sample often landed in - // water / blocked geometry / unreachable poly, the function returned - // false, and the caller had no fallback — bot stood still. Retry a - // handful of times with a fresh distance each loop so a bad roll - // doesn't lock the bot in place. + // Retry random samples so one bad roll doesn't lock the bot in place. for (int attempt = 0; attempt < 8; ++attempt) { float distance = (0.4f + rand_norm() * 0.6f) * moveStep; diff --git a/src/Mgr/Travel/TravelNode.cpp b/src/Mgr/Travel/TravelNode.cpp index 6b9ecea4b..510a80a09 100644 --- a/src/Mgr/Travel/TravelNode.cpp +++ b/src/Mgr/Travel/TravelNode.cpp @@ -226,11 +226,8 @@ TravelNodePath* TravelNode::BuildPath(TravelNode* endNode, Unit* bot, bool postP bool canPath = endPos->isPathTo(path); // Check if we reached our destination. - // Reject "pathfinder cheating" — too-short or too-steep results - // that mmap accepts but a player can't actually walk. Without this, - // the segment gets cached + saved to playerbots_travelnode_path - // and dispatched at runtime as straight-line spline through whatever - // mountain/cliff sat between A and B (cmangos parity). + // Reject too-short or too-steep results — geometry shortcut that + // mmap returns but a player can't actually walk. if (canPath && TravelPath::IsPathCheating(path, getPosition()->distance(endNode->getPosition()))) canPath = false; @@ -732,9 +729,9 @@ bool TravelPath::makeShortCut(WorldPosition startPos, float maxDist, Unit* bot) for (auto& p : fullPath) // cycle over the full path { - // Walkability filter (cmangos parity): portals/transports/taxis - // aren't valid anchor points — picking one as the new start of - // the trimmed path would leave the bot anchored on a hop. + // Walkability filter: portals/transports/taxis aren't valid + // anchor points — picking one as the new start of the trimmed + // path would leave the bot anchored on a hop. if (p.point.GetMapId() == startPos.GetMapId() && p.isWalkable()) { float curDist = p.point.sqDistance(startPos); @@ -792,9 +789,8 @@ bool TravelPath::makeShortCut(WorldPosition startPos, float maxDist, Unit* bot) } // Pass the bot into getPathTo so PathGenerator picks up its - // collision / swimming / flying flags. cmangos parity — passing - // nullptr here drops to a default mover and can produce a path - // the bot itself can't actually walk. + // collision/swim/fly state. nullptr defaults to a generic mover + // which can produce paths the bot can't actually walk. std::vector toPath = startPos.getPathTo(beginPos, bot); // We can not reach the new begin position. Follow the complete path. @@ -1290,10 +1286,8 @@ bool TravelNodeMap::GetFullPath(TravelPlan& plan, plan.Reset(); plan.destination = destination; - // mmap-probe-first. Run a 40-step chained probe; if it gets within - // spellDistance of dest, emit it as plan steps and skip the graph - // entirely (a short walk is always better than a node hop). When - // the probe falls short, fall through to graph routing. + // mmap-probe first: if a 40-step probe reaches dest, skip the + // graph entirely — a direct walk beats a node hop. if (botPos.GetMapId() == destination.GetMapId()) { std::vector probe = destination.getPathFromPath({botPos}, bot, 40); diff --git a/src/Mgr/Travel/TravelNode.h b/src/Mgr/Travel/TravelNode.h index 2481ac2cd..552bb9bbc 100644 --- a/src/Mgr/Travel/TravelNode.h +++ b/src/Mgr/Travel/TravelNode.h @@ -13,8 +13,7 @@ // THEORY // -// Pathfinding in (c)mangos is based on detour recast, an opensource navmesh creation and pathfinding codebase. -// This system is used for mob and npc pathfinding and in this codebase also for bots. +// Pathfinding uses the detour recast navmesh engine for mob, npc, and bot movement. // Because mobs and npc movement is based on following a player or a set path the PathGenerator is limited to 296y. // This means that when trying to find a path from A to B distances beyond 296y will be a best guess often moving in a // straight path. Bots would get stuck moving from Northshire to Stormwind because there is no 296y path that doesn't @@ -493,16 +492,8 @@ public: bool makeShortCut(WorldPosition startPos, float maxDist, Unit* bot = nullptr); - // Detect "pathfinder cheating" — paths that PathGenerator accepts - // but a player can't actually walk: - // * a 2-point path for an endpoint distance > 5y means navmesh - // gave up and returned the straight A->B line. - // * a vertical drop > 10y combined with a slope steeper than - // 2:1 at either start or end means the pathfinder hopped - // through a near-vertical step the navmesh permits but a - // player wouldn't survive. - // cmangos applies the same two checks in TravelNode::buildPath - // before caching a node-to-node segment. + // Reject paths the navmesh accepts but a player can't walk: + // 2-point shortcut over 5y, or > 10y vertical drop with slope steeper than 2:1. static bool IsPathCheating(std::vector const& path, float endpointDistance);