refactor(creature): replace polling-based pathfinding with event-driven follow system#183
refactor(creature): replace polling-based pathfinding with event-driven follow system#183ramon-bernardo wants to merge 12 commits intodevfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced immediate per-step path recalculation with a scheduled follow-path event: Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant Creature
participant FollowTarget as "FollowTarget (Creature)"
participant Scheduler
Player->>Creature: setFollowCreature(newTarget)
Creature->>FollowTarget: remove/add follower (old/new)
Creature->>Creature: updateFollowPath() -- cancel existing eventFollowPath
Creature->>Scheduler: schedule follow-path event (store id in eventFollowPath)
Scheduler-->>Creature: invoke scheduled follow-path task
Creature->>FollowTarget: getPathTo(target)
Creature->>Creature: goToFollowCreature()
Creature->>Creature: completeEventFollowWalk()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/creature.cpp (1)
798-805:⚠️ Potential issue | 🟠 MajorPre-existing: the condition on line 799 appears inverted compared to
onWalk(line 183).In
onWalk,lastPathUpdate < OTSYS_TIME()means "the delay has elapsed, dispatch a task." Here, the same condition causes acontinue(skip), meaning followers whose timer expired are skipped, while followers with a pending future timestamp get a redundant task dispatched. This seems backwards and would cause the exact kind of duplicate/unnecessary task scheduling this PR aims to eliminate.Should this be
>=instead of<?- if (follower->lastPathUpdate < OTSYS_TIME()) { + if (follower->lastPathUpdate >= OTSYS_TIME()) { continue; }Based on learnings,
lastPathUpdatestores a future timestamp representing when the next path update should occur, not when the last update happened (a rename tonextPathUpdatewas previously suggested for clarity).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/creature.cpp`:
- Line 799: Remove the unused local variable declaration "const auto size =
followers.size();" in src/creature.cpp (it was left from the removed
per-follower scheduling logic); delete that line so there are no unused
variables referencing followers, ensuring no other code expects "size" in the
surrounding function or block.
🧹 Nitpick comments (1)
src/creature.h (1)
396-396: Consider renaminglastPathUpdatetonextPathUpdate.This variable stores a future timestamp (when the next update is allowed), not the time of the last update. The current name is misleading. Based on learnings, the maintainer has previously agreed to rename
lastPathUpdate→nextPathUpdatein bothsrc/creature.handsrc/creature.cpp(from PR#166).♻️ Suggested rename
In
src/creature.h:- int64_t lastPathUpdate = 0; + int64_t nextPathUpdate = 0;And correspondingly in
src/creature.cpp(line 136 and 141):- if (lastPathUpdate >= OTSYS_TIME()) { + if (nextPathUpdate >= OTSYS_TIME()) {- lastPathUpdate = OTSYS_TIME() + getNumber(ConfigManager::PATHFINDING_DELAY); + nextPathUpdate = OTSYS_TIME() + getNumber(ConfigManager::PATHFINDING_DELAY);
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/creature.cpp (1)
751-759: Consider cancelingeventFollowPathwhen unfollowing.After
followCreature.reset(), any pending scheduled event fromupdateFollowPath()becomes an unnecessary no-op. While it's harmless (the handler will find no follow creature and bail out), explicitly canceling it here would be more consistent with the cleanup intent and avoid a wasted dispatcher task.♻️ Suggested cleanup
void Creature::removeFollowCreature() { if (const auto& oldFollow = getFollowCreature()) { oldFollow->removeFollower(asCreature()); } + if (eventFollowPath != 0) { + g_scheduler.stopEvent(eventFollowPath); + eventFollowPath = 0; + } + followCreature.reset(); onUnfollowCreature(); }
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/creature.cpp (1)
343-346: Minor redundancy:updateFollowPath()may be called twice per followed creature's step.When creature B moves, follower A calls
updateFollowPath()here (viaonCreatureMove), and then again when B'sonWalk()→updateFollowersPaths()iterates B's followers. The second call harmlessly cancels and replaces the first event, so correctness is fine — just a minor inefficiency.You could consider guarding one of the two call sites, but given that
updateFollowPath()is idempotent (cancel + reschedule), this is low priority.
@coderabbitai Not always when calling onCreatureMove does it call onWalk beforehand; note that we can use Game:moveCreature, which I see as requiring the updateFollowPath |
|
@coderabbitai review the changes, check quality and rewrite pull request name/summay according to changes, again |
|
Tip For best results, initiate chat on the files or code changes.
Thank you for the clarification — the dual call sites are intentional and handle different movement entry points correctly. My nitpick was incomplete. 👍 ✏️ Learnings added
🧠 Learnings used |
|
Code Quality Assessment ✅The implementation remains solid with excellent architectural improvements:
No issues found. The code is production-ready. Recommended PR TitleAlternative: Recommended PR SummaryRefactors the creature pathfinding system from a global polling approach to an event-driven architecture, eliminating redundant task scheduling and improving performance.
**Architecture Changes:**
- Removed global pathfinding polling system (`pathfindingInterval`, `pathfindingDelay`, `Game::updateCreaturesPath()`)
- Converted timestamp-based tracking (`lastPathUpdate`) to event-based scheduling (`eventFollowPath`)
- Renamed `forceUpdatePath()` → `updateFollowPath()` with event-driven behavior
**Follower Management Fixes:**
- `setFollowCreature()` now removes creature from previous target's follower list before switching
- `removeFollowCreature()` properly detaches creature from target's follower list
- Separated attack and follow behaviors - attacking no longer auto-registers as follower
**Benefits:**
- Prevents duplicate walk tasks and concurrent path updates
- Reduces dispatcher queue load and CPU overhead
- Ensures at most one pending path update per creature
- More predictable and maintainable pathfinding behavior
**Technical Details:**
- Removed redundant dispatcher tasks from player actions and creature movement handlers
- Added proper null checks in `updateFollowCreaturePath()`
- Event cancellation before scheduling prevents race conditions
- Dual call sites in `onCreatureMove()` and `updateFollowersPaths()` handle both normal walking and direct `Game::moveCreature()` paths (teleportation, admin commands)Would you like me to update the PR title and description with these recommendations using the GitHub CLI? 🧠 Learnings used✅ Actions performedReview triggered.
|
ramon-bernardo
changed the title
|
These changes seem like the best of both worlds. By using scheduled events, we avoid flooding the dispatcher with tasks; for instance, when a creature moves multiple times within EVENT_CHECK_CREATURE_INTERVAL (currently 100ms, but adjustable), or when a pathing recalculation is triggered during that window time. We aren't just piling up tasks, and we aren't bound by a global interval either. The creatures feel much more 'alive' now; however, this does come with an increased CPU cost. I haven't quite mastered the best way to profile this accurately yet; @gesior @NRH-AA @ranisalt would you mind sharing your feedback here? Similar to how PRs are handled on otland |
|
Looks better than previous code. What most of big OTSes use: And to save CPU, OTS Stats from fast exp evo with 600+ online: [17/2/2026 15:14:17]
Thread: 1 Cpu usage: 81.1385% Idle: 18.8826% Other: -0.0210705% Players online: 0
Time (ms) Calls Rel usage % Real usage % Description
8684 300 35.67780% 28.94842% std::bind(&Game::checkCreatures, this)
5526 401353 22.70212% 18.42015% std::bind(&Game::checkCreatureWalk, &g_game, id)
2376 171826 9.76364% 7.92207% std::bind(&Game::updateCreatureWalkFromCreatureMove, &g_game, getID())
1975 44696 8.11405% 6.58362% &Game::playerSay
862 40726 3.54253% 2.87435% &Game::playerUseItemEx
831 120 3.41683% 2.77237% std::bind(&Game::checkDecay, this)
819 31878 3.36501% 2.73032% &Game::playerMove
776 28051 3.19004% 2.58835% &Game::playerUseBattleWindow171.826 events This is official Kondra code for that - a bit messy, as you have both codes in 1 function that is called with true/false to change logic between dispatcher thread and workers thread: void Game::checkFollow(bool thread) {
static std::mutex checkFollowMutex;
static std::set<Creature *>::iterator it;
static std::set<Creature *> checkFollowSetNew;
if (!thread) {
g_scheduler.addEvent(createSchedulerTask(EVENT_PATH_FINDING, std::bind(&Game::checkFollow, this, false)));
checkFollowSetNew = std::move(checkFollowSet);
checkFollowSet.clear();
auto it2 = checkFollowSetNew.begin();
int skipped = 0;
while (it2 != checkFollowSetNew.end()) {
// queue walking creatures path finding for next checkFollow
if (!(*it2)->isRemoved() && (*it2)->getWalkDelay() > EVENT_PATH_FINDING) {
checkFollowSet.insert((*it2));
it2 = checkFollowSetNew.erase(it2);
skipped++;
} else {
++it2;
}
}
it = checkFollowSetNew.begin();
g_pathfinding.runTask(std::bind(&Game::checkFollow, this, true));
for (auto &creature : checkFollowSetNew) {
creature->goToFollowCreatureContinue();
ReleaseCreature(creature);
}
} else {
while (true) {
checkFollowMutex.lock();
if (it == checkFollowSetNew.end()) {
checkFollowMutex.unlock();
break;
}
Creature *creature = *it;
++it;
checkFollowMutex.unlock();
creature->goToFollowCreature();
}
}
} |
| virtual void onAttacking(uint32_t) {} | ||
|
|
||
| virtual void forceUpdatePath(); | ||
| void updateFollowPath(); |
There was a problem hiding this comment.
Consider marking this final if it shouldn't be overridden
3 participants
Pull Request Prelude
otland#4637
otland#4934
Changes Proposed
Refactors the creature pathfinding system from a global polling approach to an event-driven architecture, eliminating redundant task scheduling and improving performance.
Architecture Changes:
pathfindingInterval,pathfindingDelay,Game::updateCreaturesPath())lastPathUpdate) to event-based scheduling (eventFollowPath)forceUpdatePath()→updateFollowPath()with event-driven behaviorFollower Management Fixes:
setFollowCreature()now removes creature from previous target's follower list before switchingremoveFollowCreature()properly detaches creature from target's follower listBenefits:
Technical Details:
updateFollowCreaturePath()onCreatureMove()andupdateFollowersPaths()handle both normal walking and directGame::moveCreature()paths (teleportation, admin commands)Summary by CodeRabbit
Refactor
Chores