Aq40 #9
Aq40 #9
Conversation
Summary of ChangesHello @Celandriel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a complete and intelligent strategy for player bots to navigate and effectively participate in the Ahn'Qiraj 40-man raid. It provides the necessary framework for bots to understand and react to the unique mechanics of various boss encounters, from managing resistance buffs and positioning to prioritizing targets and executing specific actions, significantly enhancing their raid performance and utility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds AI strategies for the AQ40 raid, along with several other raids. While the core logic for the new strategies is a good starting point, there are several critical issues that need to be addressed, including memory leaks, potential crashes from incorrect return values, and improper class inheritance. I've also pointed out some areas for improvement in code style and maintainability, such as removing duplicate code, replacing magic numbers with constants, and using safer type casting and object initialization.
| Aq40AttackTargetByNameAction(PlayerbotAI* botAI, std::string const name) : AttackAction(botAI, name) {} | ||
| bool Execute(Event event) override; | ||
|
|
||
| virtual std::string const WhichEmperor() { return NULL; } |
There was a problem hiding this comment.
Returning NULL from a function that returns a std::string const will likely cause a crash due to constructing a string from a null pointer. This function should return an empty string "" or std::string() instead.
| virtual std::string const WhichEmperor() { return NULL; } | |
| virtual std::string const WhichEmperor() { return ""; } |
| class Aq40MoveTowardsEmperorVekLorAction : Aq40MoveTowardsEmperorAction | ||
| { | ||
| public: | ||
| Aq40MoveTowardsEmperorVekLorAction(PlayerbotAI* botAI) | ||
| : Aq40MoveTowardsEmperorAction(botAI, "aq40 move towards emperor vek'lor") {} | ||
|
|
||
| std::string const WhichEmperor() override { return "emperor vek'lor"; } | ||
| }; | ||
|
|
||
| class Aq40MoveTowardsEmperorVekNilashAction : Aq40MoveTowardsEmperorAction |
There was a problem hiding this comment.
The classes Aq40MoveTowardsEmperorVekLorAction and Aq40MoveTowardsEmperorVekNilashAction are missing the public access specifier for their inheritance. In C++, class inheritance is private by default, which prevents polymorphic behavior (like upcasting to the base class Action*). This is likely not the intended behavior and will cause issues. Please add public.
class Aq40MoveTowardsEmperorVekLorAction : public Aq40MoveTowardsEmperorAction
{
public:
Aq40MoveTowardsEmperorVekLorAction(PlayerbotAI* botAI)
: Aq40MoveTowardsEmperorAction(botAI, "aq40 move towards emperor vek'lor") {}
std::string const WhichEmperor() override { return "emperor vek'lor"; }
};
class Aq40MoveTowardsEmperorVekNilashAction : public Aq40MoveTowardsEmperorAction| const Position* torch_left = new Position(-8894.3, 1285.5, -112.25); | ||
| const Position* torch_right = new Position(-9029.1, 1261.8, -112.25); |
There was a problem hiding this comment.
These members are initialized with new, but there is no corresponding delete in a destructor. This will cause a memory leak. Since these positions are constant, consider making them static const Position objects instead of pointers to avoid dynamic allocation.
| for (int n = 0; n < outerpointscount; n++) | ||
| { | ||
| Position* what = new Position(-8579.0F, 1987.0F, 101.0F); | ||
| // 45.0: outside of 25 yard range, inside of 30 yard range | ||
| what->RelocatePolarOffset(n * M_PI * 2.0 / outerpointscount, 43.0); | ||
| outerpoints[n] = what; | ||
| //printf("build outerpoint %d: x=%f y=%f z=%f\n",n,what->GetPositionX(),what->GetPositionY(),what->GetPositionZ()); | ||
| } | ||
|
|
||
| for (int n = 0; n < innerpointscount; n++) | ||
| { | ||
| Position* what = new Position(-8579.0F, 1987.0F, 101.0F); | ||
| // 45.0: outside of 25 yard range, inside of 30 yard range | ||
| what->RelocatePolarOffset(n * M_PI * 2.0 / outerpointscount, 31.0); | ||
| innerpoints[n] = what; | ||
| //printf("build innerpoint %d: x=%f y=%f z=%f\n",n,what->GetPositionX(),what->GetPositionY(),what->GetPositionZ()); | ||
| } | ||
| } | ||
| bool Execute(Event event) override; | ||
|
|
||
| protected: | ||
| static int wrappingdistancebetween(int src, int dst, int scope); | ||
| int getnearestpoint(int excludeouter = -1, bool doinner = true); | ||
|
|
||
| static const int outerpointscount = 23; | ||
| static const int innerpointscount = 17; | ||
|
|
||
| Position* outerpoints[outerpointscount]; | ||
| Position* innerpoints[innerpointscount]; | ||
| }; | ||
|
|
||
| class Aq40Cthun2PositionAction : public Aq40Cthun1PositionAction | ||
| { | ||
| public: | ||
| Aq40Cthun2PositionAction(PlayerbotAI* botAI, std::string const name = "aq40 cthun2 get positioned") | ||
| : Aq40Cthun1PositionAction(botAI, name) | ||
| { | ||
| // oldworld trigger 87646 entry 15384 | ||
| getout = new Position(-8546.2, 1987.2, -96.52); | ||
| insideattack = new Position(-8550.7, 2000.4, -97.495); |
There was a problem hiding this comment.
The constructors for Aq40Cthun1PositionAction and Aq40Cthun2PositionAction allocate memory using new Position(...) but never free it, leading to memory leaks. A destructor should be added to deallocate this memory, or preferably, use smart pointers (std::unique_ptr) or store the Position objects directly in a std::vector<Position> to manage memory automatically.
| #include "raids/RaidStrategyContext.h" | ||
| #include "raids/blackwinglair/RaidBwlActionContext.h" | ||
| #include "raids/blackwinglair/RaidBwlTriggerContext.h" | ||
| #include "raids/naxxramas/RaidNaxxActionContext.h" | ||
| #include "raids/naxxramas/RaidNaxxTriggerContext.h" | ||
| #include "raids/icecrown/RaidIccActionContext.h" | ||
| #include "raids/icecrown/RaidIccTriggerContext.h" | ||
| #include "raids/obsidiansanctum/RaidOsActionContext.h" | ||
| #include "raids/obsidiansanctum/RaidOsTriggerContext.h" | ||
| #include "raids/eyeofeternity/RaidEoEActionContext.h" | ||
| #include "raids/vaultofarchavon/RaidVoATriggerContext.h" | ||
| #include "raids/onyxia/RaidOnyxiaActionContext.h" | ||
| #include "raids/onyxia/RaidOnyxiaTriggerContext.h" | ||
| #include "raids/vaultofarchavon/RaidVoAActionContext.h" | ||
| #include "raids/eyeofeternity/RaidEoETriggerContext.h" | ||
| #include "raids/moltencore/RaidMcActionContext.h" | ||
| #include "raids/moltencore/RaidMcTriggerContext.h" | ||
| #include "raids/aq20/RaidAq20ActionContext.h" | ||
| #include "raids/aq20/RaidAq20TriggerContext.h" | ||
| #include "raids/aq40/RaidAq40ActionContext.h" | ||
| #include "raids/aq40/RaidAq40TriggerContext.h" |
There was a problem hiding this comment.
This large block of new #include directives is unsorted. Sorting them alphabetically would improve readability and make it easier to spot duplicates or missing headers in the future.
|
|
||
| if (moveAwayFrom != NULL) | ||
| { | ||
| long distToTravel = radius - bot->GetDistance(moveAwayFrom); |
There was a problem hiding this comment.
The result of bot->GetDistance(), which is a float, is being assigned to a long variable. This can lead to a loss of precision. Please use a float or double for distToTravel.
float distToTravel = radius - bot->GetDistance(moveAwayFrom);| { | ||
| Unit* current = AI_VALUE(Unit*, "current target"); | ||
|
|
||
| if (current && (current->GetName() == "qiraji scarab" || current->GetName() == "qiraji scorpion")) |
There was a problem hiding this comment.
Comparing units by name is not robust, as names can be localized or may not be unique. It's better to compare them by their entry ID using GetEntry().
| const int SPELL_GREEN_BEAM = 26134; | ||
| const int SPELL_RED_COLORATION = 22518; //Probably not the right spell but looks similar | ||
|
|
||
|
|
||
| ObjectGuid botguid = bot->GetGUID(); | ||
|
|
||
| if (Unit* boss = AI_VALUE2(Unit*, "find target", "eye of c'thun")) | ||
| { | ||
| Position* point = NULL; | ||
|
|
||
| ObjectGuid bosstargetguid = boss->GetTarget(); | ||
| if (boss->HasAura(SPELL_RED_COLORATION)) | ||
| { | ||
| int bossd = (int)(boss->GetOrientation() * outerpointscount / (M_PI * 2.0)); | ||
| if (bossd < 0) | ||
| { | ||
| bossd = bossd + outerpointscount * ((-bossd) / outerpointscount + 1); | ||
| } | ||
| bossd %= outerpointscount; | ||
|
|
||
| int nearest = getnearestpoint(bossd, false); | ||
| int dist = wrappingdistancebetween(bossd, nearest, outerpointscount); | ||
|
|
||
| if (abs(dist) <= outerpointscount / 6) | ||
| { | ||
| nearest = (nearest + outerpointscount - dist * (outerpointscount / 6) / abs(dist)) % outerpointscount; | ||
| } | ||
|
|
||
| point = outerpoints[nearest]; | ||
|
|
||
| if (bot->GetDistance(*point) < 0.5) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (bosstargetguid == botguid) | ||
| { | ||
| bot->StopMoving(); | ||
| return true; | ||
| } | ||
| else | ||
| { | ||
| Spell* spell = boss->GetCurrentSpell(CURRENT_GENERIC_SPELL); | ||
|
|
||
| if (spell and spell->m_spellInfo->Id == SPELL_GREEN_BEAM) | ||
| { | ||
| Unit* bosstarget = botAI->GetUnit(bosstargetguid); | ||
| float bdist = bot->GetDistance(bosstarget); | ||
|
|
||
| if (bdist < 20.0) | ||
| { | ||
| if (bdist <= 10.0) | ||
| { | ||
| bot->AttackStop(); | ||
| //printf("%s: still %f away from %s\n",bot->GetName().c_str(),bdist,bosstarget->GetName().c_str()); | ||
| } | ||
|
|
||
| return MoveAway(bosstarget, 20.0); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!point) | ||
| { | ||
| Group* group = bot->GetGroup(); | ||
| if (bot->GetDistance(boss) < 60.0F && group) | ||
| { | ||
| Player* closest = NULL; | ||
| float closestdist = 0.0F; | ||
|
|
||
| for (GroupReference* itr = group->GetFirstMember(); itr != nullptr; itr = itr->next()) | ||
| { | ||
| Player* member = itr->GetSource(); | ||
|
|
||
| if (member->GetGUID() != botguid && member->IsAlive()) | ||
| { | ||
| float dist = bot->GetDistance(member); | ||
|
|
||
| if (dist < 10.0 && (closest == NULL || closestdist < dist)) | ||
| { | ||
| closest = member; | ||
| closestdist = dist; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (closest) | ||
| { | ||
| return MoveAway(closest, 10.0); | ||
| } | ||
| } | ||
|
|
||
| int nearest = getnearestpoint(); | ||
|
|
||
| if (nearest >= outerpointscount) | ||
| { | ||
| point = innerpoints[nearest - outerpointscount]; | ||
| } | ||
| else | ||
| { | ||
| point = outerpoints[nearest]; | ||
| } | ||
|
|
||
| if (bot->GetDistance(*point) < 0.5) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if (point) | ||
| { | ||
| return MoveTo(bot->GetMapId(), point->GetPositionX(), point->GetPositionY(), point->GetPositionZ(), | ||
| false, false, false, true, MovementPriority::MOVEMENT_COMBAT); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| bool Aq40Cthun2PositionAction::Execute(Event event) | ||
| { | ||
| // boss_cthun.cpp | ||
| const int NPC_TRIGGER = 15384; | ||
| const int NPC_EXIT_TRIGGER = 15800; | ||
|
|
||
| const int SPELL_DIGESTIVE_ACID = 26476; | ||
|
|
||
| // Areatriggers | ||
| const int SPELL_SPIT_OUT = 25383; | ||
| const int SPELL_EXIT_STOMACH = 26221; | ||
| const int SPELL_RUBBLE_ROCKY = 26271; | ||
|
|
||
| const int SPELL_CARAPACE_CTHUN = 26156; // Server-side | ||
|
|
||
|
|
||
| // temple_of_ahn_quiraj.h | ||
| const int NPC_CLAW_TENTACLE = 15725; | ||
| const int NPC_EYE_TENTACLE = 15726; | ||
| const int NPC_GIANT_CLAW_TENTACLE = 15728; | ||
| const int NPC_GIANT_EYE_TENTACLE = 15334; | ||
| const int NPC_FLESH_TENTACLE = 15802; |
There was a problem hiding this comment.
This file contains many hardcoded "magic numbers" for spell IDs, NPC IDs, and coordinates. This makes the code harder to read and maintain. Please define these as named constants at the top of the file or in a relevant header to improve clarity and ease of future updates.
| const int NPC_VEKNILASH = 15275; | ||
|
|
||
| ObjectGuid botguid = bot->GetGUID(); | ||
| ObjectGuid petguid = (ObjectGuid)0UL; |
There was a problem hiding this comment.
Using a C-style cast (ObjectGuid)0UL to create an empty ObjectGuid is not type-safe. It's better to use the default constructor ObjectGuid() or the static member ObjectGuid::Empty for clarity and safety.
ObjectGuid petguid;| std::ostringstream out; | ||
| out << "attack vek'lor"; | ||
| botAI->TellError(out.str()); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| for (int n = 0; n < boss2assignedindex; n++) | ||
| { | ||
| if (boss2assigned[n] == self) | ||
| { | ||
| if (IsVekNilash()) | ||
| { | ||
| std::ostringstream out; | ||
| out << "attack vek'nilash"; | ||
| botAI->TellError(out.str()); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if (IsPestControl()) | ||
| { | ||
| std::ostringstream out; | ||
| out << "attack pests"; | ||
| botAI->TellError(out.str()); | ||
|
|
There was a problem hiding this comment.
The calls to botAI->TellError() seem to be for debugging purposes. These should be removed from production code or wrapped in a conditional compilation block (e.g., #ifdef _DEBUG) to avoid spamming chat or logs.
No description provided.