fix(Core/Movement): Remove duplicate functions PathEndReached - #27121
fix(Core/Movement): Remove duplicate functions PathEndReached#27121PkllonG wants to merge 4 commits into
Conversation
github-actions
Bot
added
CORE
PkllonG
changed the title
📝 WalkthroughWalkthroughThe obsolete Mergeability Score: 🟡 Moderate · up to Waypoint-driven behavior may trigger completion handling too early and possibly trigger it again when the path actually ends, causing incorrect AI reactions or path state. The PR is not merge-ready until path-end processing occurs only in the path-completion callback. 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46f1f6ad-fe58-4dbb-8cd6-064e326c47e8
📒 Files selected for processing (18)
src/server/game/AI/CreatureAI.hsrc/server/game/AI/SmartScripts/SmartAI.cppsrc/server/game/AI/SmartScripts/SmartAI.hsrc/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cppsrc/server/scripts/EasternKingdoms/BlackrockMountain/BlackwingLair/boss_chromaggus.cppsrc/server/scripts/EasternKingdoms/BlackrockMountain/BlackwingLair/boss_nefarian.cppsrc/server/scripts/EasternKingdoms/Karazhan/boss_nightbane.cppsrc/server/scripts/EasternKingdoms/ZulGurub/boss_jeklik.cppsrc/server/scripts/EasternKingdoms/zone_elwynn_forest.cppsrc/server/scripts/EasternKingdoms/zone_eversong_woods.cppsrc/server/scripts/Kalimdor/CavernsOfTime/BattleForMountHyjal/boss_anetheron.cppsrc/server/scripts/Kalimdor/CavernsOfTime/BattleForMountHyjal/boss_rage_winterchill.cppsrc/server/scripts/Kalimdor/CavernsOfTime/BattleForMountHyjal/hyjal.cppsrc/server/scripts/Kalimdor/RuinsOfAhnQiraj/boss_ayamiss.cppsrc/server/scripts/Northrend/Ulduar/HallsOfStone/brann_bronzebeard.cppsrc/server/scripts/Outland/Auchindoun/ShadowLabyrinth/boss_ambassador_hellmaw.cppsrc/server/scripts/Outland/BlackTemple/boss_illidan.cppsrc/server/scripts/Outland/zone_shadowmoon_valley.cpp
💤 Files with no reviewable changes (3)
- src/server/game/AI/CreatureAI.h
- src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
- src/server/game/AI/SmartScripts/SmartAI.h
| GetScript()->ProcessEventsFor(SMART_EVENT_WAYPOINT_ENDED, nullptr, 0, me->GetWaypointPath()); | ||
| me->LoadPath(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A35 -B10 \
'WaypointReached|WaypointPathEnded|pathEnded|LoadPath\(0\)' \
src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp \
src/server/game/AI/SmartScripts/SmartAI.cppRepository: azerothcore/azerothcore-wotlk
Length of output: 23972
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- changed SmartAI diff ---'
git diff -- src/server/game/AI/SmartScripts/SmartAI.cpp
printf '%s\n' '--- callback declarations and implementations ---'
rg -n -A12 -B8 \
'WaypointPathEnded|WaypointReached|LoadPath\(uint32|LoadPath\(0\)|SMART_EVENT_WAYPOINT_ENDED|SMART_EVENT_WAYPOINT_REACHED' \
src/server/game/AI src/server/game/Entities src/server/game/Movement
printf '%s\n' '--- relevant current source ---'
sed -n '1315,1365p' src/server/game/AI/SmartScripts/SmartAI.cpp
sed -n '125,205p' src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cppRepository: azerothcore/azerothcore-wotlk
Length of output: 46034
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- checkout state ---'
git status --short
git log -1 --oneline
printf '%s\n' '--- parent implementation ---'
git show HEAD^:src/server/game/AI/SmartScripts/SmartAI.cpp | sed -n '1328,1362p'
git show HEAD^:src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp | sed -n '135,202p'
printf '%s\n' '--- all Creature::LoadPath(0) callers ---'
rg -n -B8 -A8 'me->LoadPath\(0\)|LoadPath\(0\)' src
printf '%s\n' '--- event filter and path state ---'
sed -n '5018,5036p' src/server/game/AI/SmartScripts/SmartScript.cpp
sed -n '350,366p' src/server/game/Entities/Creature/Creature.hRepository: azerothcore/azerothcore-wotlk
Length of output: 9425
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- merge commit parents ---'
git show -s --format='%H%n%P%n%B' HEAD
printf '%s\n' '--- file diff against first parent ---'
git diff --stat HEAD^1 HEAD -- src/server/game/AI/SmartScripts/SmartAI.cpp src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
git diff HEAD^1 HEAD -- src/server/game/AI/SmartScripts/SmartAI.cpp src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
printf '%s\n' '--- file diff against second parent ---'
git diff --stat HEAD^2 HEAD -- src/server/game/AI/SmartScripts/SmartAI.cpp src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
git diff HEAD^2 HEAD -- src/server/game/AI/SmartScripts/SmartAI.cpp src/server/game/Movement/MovementGenerators/WaypointMovementGenerator.cpp
printf '%s\n' '--- combined diff for the target method ---'
git diff --cc HEAD -- src/server/game/AI/SmartScripts/SmartAI.cppRepository: azerothcore/azerothcore-wotlk
Length of output: 3153
Move path-end processing to WaypointPathEnded. WaypointReached runs for every waypoint, so these lines dispatch SMART_EVENT_WAYPOINT_ENDED and clear Creature::m_path_id before the path ends; the final callback can dispatch the event again. Remove both lines from WaypointReached and process the event and path reset once in WaypointPathEnded.
There was a problem hiding this comment.
I need help here, should I keep it like this or is there another way?
There was a problem hiding this comment.
@PkllonG Do not keep these lines in WaypointReached; that callback runs at every waypoint. Move both lines to WaypointPathEnded, before its return, so SMART_EVENT_WAYPOINT_ENDED dispatches once when the path ends and me->LoadPath(0) clears the completed path once.
You are interacting with an AI system.
There was a problem hiding this comment.
No errors were found after testing, like .go c 139275
There was a problem hiding this comment.
@PkllonG, the test result is useful, but it does not validate the callback timing. WaypointReached still runs at every node, so SMART_EVENT_WAYPOINT_ENDED and me->LoadPath(0) must remain in WaypointPathEnded to run once at path completion.
You are interacting with an AI system.
…erothcore-wotlk into Remove-PathEndReached
1 participant
Changes Proposed:
This PR proposes changes to:
AI-assisted Pull Requests
Important
Using AI tools to prepare pull requests is allowed, but it must be disclosed and it must follow our AC guidelines for AI Agentic Engineering (link below).
You are expected to fully understand the changes you submit and to be able to explain and justify them when maintainers ask.
Issues Addressed:
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
How to Test the Changes:
Known Issues and TODO List:
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.