fix: Refactor NextAction to remove all stringly typing#1
Open
SmashingQuasar wants to merge 62 commits intomainfrom
Open
fix: Refactor NextAction to remove all stringly typing#1SmashingQuasar wants to merge 62 commits intomainfrom
SmashingQuasar wants to merge 62 commits intomainfrom
Conversation
Zamralik
force-pushed
the
dev/SmashingQuasar/action-stringly-type-nuke
branch
from
714c671 to
acb8f43
Compare
SmashingQuasar
changed the title
SmashingQuasar
force-pushed
the
dev/SmashingQuasar/action-stringly-type-nuke
branch
4 times, most recently
from
445be7d to
3cbe8d5
Compare
SmashingQuasar
commented
Feb 8, 2026
Author
SmashingQuasar
left a comment
SmashingQuasar
left a comment
There was a problem hiding this comment.
I have done a complete review of each file, one at a time, one line at a time.
I have left comments for documentation purposes.
Because this PR requires an in-depth knowledge of how actions function, it is not possible to review properly for anyone else.
Therefore, we will proceed with a long phase of live testing to ensure stability.
Comment on lines
1
to
71
| #pragma once | ||
|
|
||
| #include "CreateNextAction.h" | ||
| #include "GenericSpellActions.h" | ||
| #include "AiFactory.h" | ||
| #include "DKActions.h" | ||
| #include "PaladinActions.h" | ||
| #include "WarriorActions.h" | ||
| #include "DruidBearActions.h" | ||
|
|
||
| class UniversalTauntAction : public Action | ||
| { | ||
| public: | ||
| UniversalTauntAction(PlayerbotAI* botAI) : Action(botAI) {} | ||
|
|
||
| bool isPossible() override | ||
| { | ||
| const uint8_t tab = AiFactory::GetPlayerSpecTab(this->bot); | ||
|
|
||
| switch (this->bot->getClass()) | ||
| { | ||
| case CLASS_DEATH_KNIGHT: | ||
| return tab == DEATH_KNIGHT_TAB_BLOOD; | ||
| case CLASS_PALADIN: | ||
| return tab == PALADIN_TAB_PROTECTION; | ||
| case CLASS_DRUID: | ||
| return tab == DRUID_TAB_FERAL; | ||
| case CLASS_WARRIOR: | ||
| return tab == WARRIOR_TAB_PROTECTION; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| bool isUseful() override | ||
| { | ||
| const Unit* const target = this->GetTarget(); | ||
|
|
||
| if (target == nullptr) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| const ObjectGuid targetTargetGUID = target->GetTarget(); | ||
|
|
||
| if (targetTargetGUID.GetEntry() == this->bot->GetGUID().GetEntry()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool Execute(Event) override | ||
| { | ||
| switch (this->bot->getClass()) | ||
| { | ||
| case CLASS_DEATH_KNIGHT: | ||
| return this->botAI->DoSpecificAction(CreateNextAction<CastDarkCommandAction>(ACTION_EMERGENCY).factory); | ||
| case CLASS_PALADIN: | ||
| return this->botAI->DoSpecificAction(CreateNextAction<CastHandOfReckoningAction>(ACTION_EMERGENCY).factory); | ||
| case CLASS_DRUID: | ||
| return this->botAI->DoSpecificAction(CreateNextAction<CastGrowlAction>(ACTION_EMERGENCY).factory); | ||
| case CLASS_WARRIOR: | ||
| return this->botAI->DoSpecificAction(CreateNextAction<CastTauntAction>(ACTION_EMERGENCY).factory); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| }; |
Author
There was a problem hiding this comment.
A new action that was made necessary as there is no longer a magic fallback on the current class "taunt" action.
Comment on lines
+37
to
+43
| class EndPullAction : public ChangeCombatStrategyAction | ||
| { | ||
| public: | ||
| EndPullAction(PlayerbotAI* botAI, std::string const name = "-pull") : ChangeCombatStrategyAction(botAI, name) {} | ||
| ~EndPullAction() override = default; | ||
| }; | ||
|
|
Author
There was a problem hiding this comment.
This new action was made necessary because we can't use action calls since we use a factory now.
Comment on lines
+86
to
+88
| std::unique_ptr<Action> action = nextAction.factory(this->botAI); | ||
|
|
||
| if (dynamic_cast<RpgEnabled*>(action)) | ||
| if (dynamic_cast<RpgEnabled*>(action.get())) |
Author
There was a problem hiding this comment.
NextAction::Factory returns an std::unique_ptr<Action> so I migrated the syntax here.
Comment on lines
+218
to
+222
| std::vector<NextAction> prerequisites = Action::getPrerequisites(); | ||
|
|
||
| prerequisites.push_back(CreateNextAction<ReachPartyMemberToResurrectAction>(1.0f)); | ||
|
|
||
| return prerequisites; |
Author
There was a problem hiding this comment.
NextAction being a simple struct instead of a class, it no longer has any attached method, which means merge does not exist. Since Action::getPrerequisites() returns an empty vector, this code was simply over-engineered.
Comment on lines
+116
to
+122
| // @TODO: This is a simple fallback. It should never be triggered. | ||
| NextAction::Factory RpgSubAction::getActionFactory() const | ||
| { | ||
| LOG_ERROR("playerbots.rpg.rpgsubaction", "Bot {} - No action factory defined for RpgSubAction", this->bot->GetName()); | ||
|
|
||
| Event RpgSubAction::ActionEvent(Event event) { return event; } | ||
| return CreateNextAction<EmoteAction>(1.0f).factory; | ||
| } |
Author
There was a problem hiding this comment.
Temporary solution to have a clean fallback until we can have a better workflow. This is mostly for debug purposes.
Comment on lines
+16
to
+38
| template <typename TAction> | ||
| void RegisterActionFactoryOnce(PlayerbotAI* const botAI) | ||
| { | ||
| static_assert(std::is_base_of<Action, TAction>::value == true); | ||
|
|
||
| static std::once_flag flag; | ||
|
|
||
| std::call_once( | ||
| flag, | ||
| [botAI]() | ||
| { | ||
| assert(botAI != nullptr); | ||
|
|
||
| ActionFactoryRegistry::RegisterByType(std::type_index(typeid(TAction)), &CreateAction<TAction>); | ||
|
|
||
| std::unique_ptr<Action> probe = std::unique_ptr<Action>(new TAction(botAI)); | ||
|
|
||
| const std::string& name = probe->getName(); | ||
|
|
||
| ActionFactoryRegistry::RegisterByName(name, &CreateAction<TAction>); | ||
| } | ||
| ); | ||
| } |
Author
There was a problem hiding this comment.
Since this is using both a string and type registry, if this was done every tick it would be very expensive in terms of performance. Still less expensive than the original design that was aggressively relying on strings, but we are here to optimise.
Comment on lines
-15
to
-48
| class NextAction | ||
| { | ||
| public: | ||
| NextAction(std::string const name, float relevance = 0.0f) | ||
| : relevance(relevance), name(name) {} | ||
|
|
||
| NextAction(NextAction const&) = default; | ||
| NextAction& operator=(NextAction const&) = default; | ||
|
|
||
| std::string const getName() { return name; } | ||
| float getRelevance() { return relevance; } | ||
|
|
||
| static std::vector<NextAction> merge(std::vector<NextAction> const& what, std::vector<NextAction> const& with) | ||
| { | ||
| std::vector<NextAction> result = {}; | ||
|
|
||
| for (NextAction const& action : what) | ||
| { | ||
| result.push_back(action); | ||
| } | ||
|
|
||
| for (NextAction const& action : with) | ||
| { | ||
| result.push_back(action); | ||
| } | ||
|
|
||
| return result; | ||
| }; | ||
|
|
||
| private: | ||
| float relevance; | ||
| std::string name; | ||
| }; | ||
|
|
Author
There was a problem hiding this comment.
Moved to its own file and refactored as a simple struct.
Comment on lines
-84
to
-125
| class ActionNode | ||
| { | ||
| public: | ||
| ActionNode( | ||
| std::string name, | ||
| std::vector<NextAction> prerequisites = {}, | ||
| std::vector<NextAction> alternatives = {}, | ||
| std::vector<NextAction> continuers = {} | ||
| ) : | ||
| name(std::move(name)), | ||
| action(nullptr), | ||
| continuers(continuers), | ||
| alternatives(alternatives), | ||
| prerequisites(prerequisites) | ||
| {} | ||
|
|
||
| virtual ~ActionNode() = default; | ||
|
|
||
| Action* getAction() { return action; } | ||
| void setAction(Action* action) { this->action = action; } | ||
| const std::string getName() { return name; } | ||
|
|
||
| std::vector<NextAction> getContinuers() | ||
| { | ||
| return NextAction::merge(this->continuers, action->getContinuers()); | ||
| } | ||
| std::vector<NextAction> getAlternatives() | ||
| { | ||
| return NextAction::merge(this->alternatives, action->getAlternatives()); | ||
| } | ||
| std::vector<NextAction> getPrerequisites() | ||
| { | ||
| return NextAction::merge(this->prerequisites, action->getPrerequisites()); | ||
| } | ||
|
|
||
| private: | ||
| const std::string name; | ||
| Action* action; | ||
| std::vector<NextAction> continuers; | ||
| std::vector<NextAction> alternatives; | ||
| std::vector<NextAction> prerequisites; | ||
| }; |
Comment on lines
-127
to
-148
| class ActionBasket | ||
| { | ||
| public: | ||
| ActionBasket(ActionNode* action, float relevance, bool skipPrerequisites, Event event); | ||
|
|
||
| virtual ~ActionBasket(void) {} | ||
|
|
||
| float getRelevance() { return relevance; } | ||
| ActionNode* getAction() { return action; } | ||
| Event getEvent() { return event; } | ||
| bool isSkipPrerequisites() { return skipPrerequisites; } | ||
| void AmendRelevance(float k) { relevance *= k; } | ||
| void setRelevance(float relevance) { this->relevance = relevance; } | ||
| bool isExpired(uint32_t msecs); | ||
|
|
||
| private: | ||
| ActionNode* action; | ||
| float relevance; | ||
| bool skipPrerequisites; | ||
| Event event; | ||
| uint32_t created; | ||
| }; |
Comment on lines
+96
to
+97
| CreateNextAction<CastHeroicThrowAction>(1.0f), | ||
| CreateNextAction<CastShieldSlamAction>(1.0f), |
Author
There was a problem hiding this comment.
This was originally a convoluted system or recursive creators being called. I simplified it this way which preserves the same behaviour.
SmashingQuasar
force-pushed
the
dev/SmashingQuasar/action-stringly-type-nuke
branch
from
2eadf42 to
1b362ad
Compare
…l health trigger.
There was a problem hiding this comment.
Pull request overview
Refactors the bot AI action selection pipeline to avoid string-based (“stringly typed”) NextAction usage by introducing typed factories, and updates a wide set of class strategies/actions to use the new mechanism.
Changes:
- Added
CreateAction/CreateNextAction+ one-time factory registration to buildNextActionfrom action types instead of string names. - Updated many class strategies/triggers/action-node factories to use
CreateNextAction<TAction>(weight)and removed several shared action-context registries. - Migrated several
Multiplier::GetValueimplementations fromAction*toAction&and updated related call sites.
Reviewed changes
Copilot reviewed 174 out of 333 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Ai/Class/Warlock/Action/WarlockActions.h | Adds typed actions for using spellstone/firestone items |
| src/Ai/Class/Shaman/Strategy/EnhancementShamanStrategy.cpp | Replaces string NextAction with typed CreateNextAction in strategy/triggers |
| src/Ai/Class/Shaman/Strategy/ElementalShamanStrategy.cpp | Replaces string NextAction with typed CreateNextAction in strategy/triggers |
| src/Ai/Class/Shaman/ShamanAiObjectContext.h | Removes shared action context wiring for Shaman |
| src/Ai/Class/Rogue/Strategy/GenericRogueNonCombatStrategy.cpp | Replaces string NextAction with typed CreateNextAction in triggers |
| src/Ai/Class/Rogue/RogueAiObjectContext.h | Removes shared action context wiring for Rogue |
| src/Ai/Class/Rogue/RogueAiObjectContext.cpp | Removes Rogue action context registry and adjusts base context construction |
| src/Ai/Class/Priest/Strategy/ShadowPriestStrategyActionNodeFactory.h | Converts action nodes to typed follow-up actions |
| src/Ai/Class/Priest/Strategy/ShadowPriestStrategy.cpp | Converts default actions and triggers to typed CreateNextAction |
| src/Ai/Class/Priest/Strategy/PriestNonCombatStrategy.cpp | Converts non-combat/buff triggers to typed actions |
| src/Ai/Class/Priest/Strategy/HolyPriestStrategy.h | Removes include dependency (factory header) |
| src/Ai/Class/Priest/Strategy/HealPriestStrategy.cpp | Converts triggers/default actions to typed actions |
| src/Ai/Class/Priest/PriestAiObjectContext.h | Removes shared action context wiring for Priest |
| src/Ai/Class/Paladin/Strategy/TankPaladinStrategy.cpp | Converts tank paladin strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Paladin/Strategy/PaladinBuffStrategies.cpp | Converts paladin buff triggers to typed actions |
| src/Ai/Class/Paladin/Strategy/HealPaladinStrategy.cpp | Converts heal paladin triggers/default actions to typed actions |
| src/Ai/Class/Paladin/Strategy/GenericPaladinStrategy.cpp | Converts generic paladin triggers to typed actions |
| src/Ai/Class/Paladin/Strategy/GenericPaladinNonCombatStrategy.cpp | Converts non-combat paladin triggers and spec branching to typed actions |
| src/Ai/Class/Paladin/Strategy/DpsPaladinStrategy.cpp | Converts DPS paladin strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Paladin/PaladinAiObjectContext.h | Removes shared action context wiring for Paladin |
| src/Ai/Class/Mage/Strategy/GenericMageNonCombatStrategy.cpp | Converts mage non-combat triggers to typed actions |
| src/Ai/Class/Mage/Strategy/FrostMageStrategy.cpp | Converts frost mage actions/triggers to typed actions |
| src/Ai/Class/Mage/Strategy/FrostFireMageStrategy.cpp | Converts frostfire mage actions/triggers to typed actions |
| src/Ai/Class/Mage/Strategy/FireMageStrategy.cpp | Converts fire mage actions/triggers to typed actions |
| src/Ai/Class/Mage/Strategy/ArcaneMageStrategy.cpp | Converts arcane mage actions/triggers to typed actions |
| src/Ai/Class/Mage/MageAiObjectContext.h | Removes shared action context wiring for Mage |
| src/Ai/Class/Mage/Action/MageActions.h | Adds CastArcaneExplosionAction |
| src/Ai/Class/Hunter/Strategy/MarksmanshipHunterStrategy.cpp | Converts hunter strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Hunter/Strategy/HunterBuffStrategies.cpp | Converts hunter buff triggers/action nodes to typed actions |
| src/Ai/Class/Hunter/Strategy/GenericHunterNonCombatStrategy.cpp | Converts hunter non-combat triggers to typed actions |
| src/Ai/Class/Hunter/Strategy/BeastMasteryHunterStrategy.cpp | Converts BM hunter actions/triggers to typed actions |
| src/Ai/Class/Hunter/HunterAiObjectContext.h | Removes shared action context wiring for Hunter |
| src/Ai/Class/Hunter/Action/HunterActions.h | Adds SayLowAmmoAction and CastTrackHumanoidsAction |
| src/Ai/Class/Druid/Strategy/MeleeDruidStrategy.cpp | Converts melee druid defaults/triggers to typed actions |
| src/Ai/Class/Druid/Strategy/FeralDruidStrategy.h | Adds includes and converts shapeshift action nodes to typed actions |
| src/Ai/Class/Druid/Strategy/FeralDruidStrategy.cpp | Converts feral druid action nodes/triggers to typed actions |
| src/Ai/Class/Druid/DruidAiObjectContext.h | Removes shared action context wiring for Druid |
| src/Ai/Class/Druid/Action/DruidShapeshiftActions.cpp | Refactors alternatives to (intended) typed actions |
| src/Ai/Class/Druid/Action/DruidActions.cpp | Refactors prerequisites/alternatives to typed actions |
| src/Ai/Class/Dk/Trigger/DKTriggers.cpp | Removes unused include |
| src/Ai/Class/Dk/Strategy/UnholyDKStrategy.cpp | Converts DK strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Dk/Strategy/GenericDKNonCombatStrategy.cpp | Converts DK non-combat triggers to typed actions |
| src/Ai/Class/Dk/Strategy/FrostDKStrategy.cpp | Converts DK frost strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Dk/Strategy/BloodDKStrategy.cpp | Converts DK blood strategy/action nodes/triggers to typed actions |
| src/Ai/Class/Dk/DKAiObjectContext.h | Removes shared action context wiring for DK |
| src/Ai/Class/Dk/Action/DKActions.cpp | Refactors DK prerequisites to typed actions |
| src/Ai/Base/WorldPacketTriggerContext.h | Adds missing include for range triggers |
| src/Ai/Base/Trigger/ChatCommandTrigger.cpp | Narrows include usage to Player.h |
| src/Ai/Base/Strategy/WorldPacketHandlerStrategy.h | Refactors supported actions list to use factories |
| src/Ai/Base/Strategy/UsePotionsStrategy.cpp | Converts triggers/action node to typed actions |
| src/Ai/Base/Strategy/UseFoodStrategy.cpp | Converts food/drink triggers to typed actions |
| src/Ai/Base/Strategy/TravelStrategy.cpp | Converts travel default actions/triggers to typed actions |
| src/Ai/Base/Strategy/ThreatStrategy.h | Changes multipliers to take Action& |
| src/Ai/Base/Strategy/ThreatStrategy.cpp | Updates multiplier implementations for Action& |
| src/Ai/Base/Strategy/TellTargetStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/Strategy/TankAssistStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/Strategy/StayStrategy.cpp | Converts triggers/default actions to typed actions |
| src/Ai/Base/Strategy/SayStrategy.h | Removes SayStrategy (deleted) |
| src/Ai/Base/Strategy/SayStrategy.cpp | Removes SayStrategy implementation (deleted) |
| src/Ai/Base/Strategy/RunawayStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/Strategy/RpgStrategy.h | Refactors RPG multiplier to use Action& and context value |
| src/Ai/Base/Strategy/RpgStrategy.cpp | Converts RPG defaults/triggers to typed actions |
| src/Ai/Base/Strategy/ReturnStrategy.cpp | Converts trigger actions to typed actions |
| src/Ai/Base/Strategy/RangedCombatStrategy.cpp | Converts trigger actions to typed actions |
| src/Ai/Base/Strategy/RacialsStrategy.cpp | Converts triggers/action node to typed actions |
| src/Ai/Base/Strategy/QuestStrategies.h | Refactors supported actions to factory-based list |
| src/Ai/Base/Strategy/QuestStrategies.cpp | Converts quest triggers to typed actions |
| src/Ai/Base/Strategy/PullStrategy.cpp | Converts multiplier signature and partially refactors defaults |
| src/Ai/Base/Strategy/PassiveStrategy.cpp | Removes unused include |
| src/Ai/Base/Strategy/PassTroughStrategy/definition/struct/passthrough-strategy-supported-actions.struct.h | Adds struct for supported passthrough actions |
| src/Ai/Base/Strategy/PassTroughStrategy.h | Changes supported list from names to {name,factory} |
| src/Ai/Base/Strategy/PassTroughStrategy.cpp | Builds triggers using factories instead of string action names |
| src/Ai/Base/Strategy/NonCombatStrategy.cpp | Converts non-combat triggers to typed actions |
| src/Ai/Base/Strategy/MoveFromGroupStrategy.cpp | Converts defaults to typed action |
| src/Ai/Base/Strategy/MeleeCombatStrategy.cpp | Converts triggers to typed actions |
| src/Ai/Base/Strategy/MarkRtiStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/Strategy/MaintenanceStrategy.cpp | Converts maintenance triggers to typed actions; comments out reset |
| src/Ai/Base/Strategy/LootNonCombatStrategy.cpp | Converts loot/gather triggers to typed actions |
| src/Ai/Base/Strategy/LfgStrategy.cpp | Converts LFG triggers to typed actions |
| src/Ai/Base/Strategy/KiteStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/Strategy/GuildStrategy.cpp | Converts guild triggers to typed actions |
| src/Ai/Base/Strategy/GuardStrategy.cpp | Converts default actions to typed action |
| src/Ai/Base/Strategy/GroupStrategy.cpp | Converts group triggers to typed actions |
| src/Ai/Base/Strategy/GrindingStrategy.cpp | Converts defaults/triggers to typed actions |
| src/Ai/Base/Strategy/FollowMasterStrategy.cpp | Converts default actions to typed action |
| src/Ai/Base/Strategy/FleeStrategy.cpp | Converts flee triggers to typed actions |
| src/Ai/Base/Strategy/EmoteStrategy.cpp | Converts emote/talk/greet triggers to typed actions and config accessors |
| src/Ai/Base/Strategy/DuelStrategy.cpp | Converts duel triggers to typed actions |
| src/Ai/Base/Strategy/DpsAssistStrategy.cpp | Converts DPS assist triggers to typed actions |
| src/Ai/Base/Strategy/DebugStrategy.cpp | Removes a now-unused debug strategy implementation (deleted) |
| src/Ai/Base/Strategy/DeadStrategy.cpp | Converts death/release triggers to typed actions |
| src/Ai/Base/Strategy/ConserveManaStrategy.h | Changes multiplier to take Action& |
| src/Ai/Base/Strategy/ConserveManaStrategy.cpp | Updates multiplier implementation for Action& |
| src/Ai/Base/Strategy/CombatStrategy.cpp | Converts common combat triggers/defaults to typed actions; comments out reset |
| src/Ai/Base/Strategy/ChatCommandHandlerStrategy.h | Expands includes likely to support factory-based action creation |
| src/Ai/Base/Strategy/CastTimeStrategy.h | Changes multiplier to take Action& |
| src/Ai/Base/Strategy/CastTimeStrategy.cpp | Updates multiplier implementation for Action& |
| src/Ai/Base/Strategy/AttackEnemyPlayersStrategy.cpp | Converts trigger to typed action |
| src/Ai/Base/RegisterActionFactoryOnce.h | Introduces one-time registration for action factories (new) |
| src/Ai/Base/CreateNextAction.h | Introduces typed helper to build NextAction (new) |
| src/Ai/Base/CreateAction.h | Introduces typed helper to build Actions and register factories (new) |
| src/Ai/Base/Actions/combat/UniversalTauntAction.h | Adds a universal taunt action dispatching by class/spec (new) |
| src/Ai/Base/Actions/TravelAction.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/TradeStatusExtendedAction.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/TellMasterAction.h | Adds dedicated TellMaster actions for money/reputation errors |
| src/Ai/Base/Actions/TellLosAction.cpp | Adds include for DynamicObject |
| src/Ai/Base/Actions/SayAction.h | Minor formatting tweak |
| src/Ai/Base/Actions/RpgSubActions.h | Refactors sub-actions to return factories instead of string action names |
| src/Ai/Base/Actions/RpgAction.cpp | Refactors RPG action selection to use NextAction factories |
| src/Ai/Base/Actions/ReviveFromCorpseAction.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/RemoveAuraAction.cpp | Narrows include to PlayerbotAI.h |
| src/Ai/Base/Actions/ReachTargetActions.h | Moves CastReachTargetSpellAction out (now in GenericSpellActions) |
| src/Ai/Base/Actions/ReachTargetActions.cpp | Removes CastReachTargetSpellAction implementation |
| src/Ai/Base/Actions/PetsAction.cpp | Removes unused include |
| src/Ai/Base/Actions/PetAttackAction.h | Adds chat pet attack action wrapper (new) |
| src/Ai/Base/Actions/PassLeadershipToMasterAction.h | Adds GiveLeaderInDungeonAction |
| src/Ai/Base/Actions/MovementActions.cpp | Adds include for DynamicObject |
| src/Ai/Base/Actions/InviteToGroupAction.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/GuildManagementActions.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/GuildCreateActions.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/GenericSpellActions.h | Adds CastReachTargetSpellAction and refactors resurrect prereqs |
| src/Ai/Base/Actions/GenericSpellActions.cpp | Adds CastReachTargetSpellAction::isUseful implementation |
| src/Ai/Base/Actions/CustomStrategyEditAction.cpp | Narrows include usage |
| src/Ai/Base/Actions/ChooseRpgTargetAction.cpp | Refactors RPG action probing to use factories |
| src/Ai/Base/Actions/ChangeStrategyAction.h | Adds EndPullAction wrapper |
| src/Ai/Base/Actions/BuyAction.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/BossAuraActions.h | Adds default bossName and virtual destructors |
| src/Ai/Base/Actions/BossAuraActions.cpp | Replaces string action dispatch with typed factory dispatch |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsUseBuff.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsResetObjectiveForce.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsProtectFC.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsMoveToStart.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsMoveToObjective.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsCheckObjective.h | Adds BG tactics action wrapper (new) |
| src/Ai/Base/Actions/BattleGroundTactics/BattlegroundTacticsCheckFlag.h | Adds BG tactics action wrapper (new) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 176 out of 335 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
15
to
27
| std::vector<NextAction> CastDeathchillAction::getPrerequisites() | ||
| { | ||
| return NextAction::merge({ NextAction("frost presence") }, | ||
| CastSpellAction::getPrerequisites()); | ||
| return { | ||
| CreateNextAction<CastFrostPresenceAction>(1.0f) | ||
| }; | ||
| } | ||
|
|
||
| std::vector<NextAction> CastUnholyMeleeSpellAction::getPrerequisites() | ||
| { | ||
| return NextAction::merge({ NextAction("unholy presence") }, | ||
| CastMeleeSpellAction::getPrerequisites()); | ||
| return { | ||
| CreateNextAction<CastUnholyPresenceAction>(1.0f) | ||
| }; | ||
| } |
There was a problem hiding this comment.
These overrides drop the base-class prerequisites entirely (previously merged with CastSpellAction::getPrerequisites() / CastMeleeSpellAction::getPrerequisites()). That can break required setup steps inherited from the base action. Build the prerequisite list by starting from the base prerequisites and appending the presence action (or merge them), rather than returning a one-element vector.
Comment on lines
20
to
27
| std::vector<NextAction> CastDireBearFormAction::getAlternatives() | ||
| { | ||
| return NextAction::merge({ NextAction("bear form") }, | ||
| CastSpellAction::getAlternatives()); | ||
| std::vector<NextAction> alternatives; | ||
|
|
||
| alternatives.push_back(CreateNextAction<CastBearFormAction>(1.0f)); | ||
|
|
||
| return alternatives; | ||
| } |
There was a problem hiding this comment.
This change removes the inherited alternatives previously returned from CastSpellAction::getAlternatives(). If those alternatives are still expected (e.g., fallbacks or shared behavior), they should be preserved by starting from the base alternatives and then adding CastBearFormAction.
Comment on lines
+24
to
+40
| static std::once_flag flag; | ||
|
|
||
| std::call_once( | ||
| flag, | ||
| [botAI]() | ||
| { | ||
| assert(botAI != nullptr); | ||
|
|
||
| ActionFactoryRegistry::RegisterByType(std::type_index(typeid(TAction)), &CreateAction<TAction>); | ||
|
|
||
| std::unique_ptr<Action> probe = std::unique_ptr<Action>(new TAction(botAI)); | ||
|
|
||
| const std::string& name = probe->getName(); | ||
|
|
||
| ActionFactoryRegistry::RegisterByName(name, &CreateAction<TAction>); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Because RegisterActionFactoryOnce is a function template defined in a header, static std::once_flag flag; is instantiated per translation unit (per TAction), not truly process-wide. That means the registry can be populated multiple times concurrently from different TUs, defeating the 'once' intent and potentially racing inside ActionFactoryRegistry. Consider moving the once-guard into a single TU, using an inline static once_flag in a templated struct/class, or implementing a registry-internal, type-keyed synchronization mechanism.
Comment on lines
+22
to
+23
| .name = "accept quest", | ||
| .factory = &CreateAction<AcceptQuestAction> |
There was a problem hiding this comment.
This uses C++20 designated initializers (.name = ..., .factory = ...). If the project is still compiled as C++17 (common in many game-server codebases), this will not compile. To keep compatibility, prefer non-designated aggregate initialization (e.g., { \"accept quest\", &CreateAction<AcceptQuestAction> }) or provide a constructor for PassthroughStrategySupportedActionsStruct.
Suggested change
| .name = "accept quest", | |
| .factory = &CreateAction<AcceptQuestAction> | |
| "accept quest", | |
| &CreateAction<AcceptQuestAction> |
| #include "Playerbots.h" | ||
| #include "SpellInfo.h" | ||
| #include "SpellMgr.h" | ||
| #include "DKActions.h" |
There was a problem hiding this comment.
DKActions.h is included twice in this source file (once at the top and again at line 13). Dropping the redundant include will reduce compile overhead and avoid confusion when resolving dependencies.
Suggested change
| #include "DKActions.h" |
| #include "RpgSubActions.h" | ||
| #include "ServerFacade.h" | ||
| #include "PossibleRpgTargetsValue.h" | ||
| #include "RpgSubActions.h" |
There was a problem hiding this comment.
RpgSubActions.h is included twice. Removing the duplicate include will keep the include list clean and reduce compilation work.
Suggested change
| #include "RpgSubActions.h" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.