fix(Core/SmartAI): Unify code format and fix some potential null pointer crashes#25302
fix(Core/SmartAI): Unify code format and fix some potential null pointer crashes#25302PkllonG wants to merge 1 commit intoazerothcore:masterfrom
Conversation
github-actions
bot
added
CORE
There was a problem hiding this comment.
Pull request overview
This PR targets SmartAI/SmartScript stability by adding SmartAI-type guards and adjusting some SmartAI calls to reduce potential null-pointer/dynamic-cast crashes, while also aiming to unify formatting in a few action handlers.
Changes:
- Added
IsSmart()guards to severalSmartScript::ProcessActioncases to avoid operating on non-SmartAI/non-SmartGameObjectAI sources. - Simplified
SMART_ACTION_COMBAT_STOPto callSetSuppressEvade()directly after a SmartAI check. - Added an
IsSmart()guard beforeSMART_ACTION_CUSTOM_CAST.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (targets.empty()) | ||
| break; | ||
|
|
||
| if (!IsSmart()) | ||
| break; | ||
|
|
There was a problem hiding this comment.
SMART_ACTION_CAST now unconditionally bails out on !IsSmart(). For AreaTrigger-based SmartScripts (mScriptType == SMART_SCRIPT_TYPE_AREATRIGGER), both me and go are null, so IsSmart() always returns false and this prevents AreaTrigger scripts from ever executing SMART_ACTION_CAST (even though the code below has explicit e.IsAreatriggerScript() handling via unit->SummonTrigger(...)). Consider changing the guard to only require IsSmart() when the action will actually rely on SmartAI (e.g., when me is set), or otherwise exempt AreaTrigger scripts so casts still work.
Co-Authored-By: blinkysc <[email protected]>
| // JustExitedCombat does not trigger EnterEvadeMode. | ||
| if (SmartAI* sai = CAST_AI(SmartAI, me->AI())) | ||
| sai->SetSuppressEvade(true); | ||
| if (IsSmart()) |
There was a problem hiding this comment.
CAST_AI is just dynamic_cast. Old code is already null safe? This replaces one dynamic_cast with two? Maybe only thing is probably the null guard for SMARTCAST_COMBAT_MOVE
There was a problem hiding this comment.
Just for the sake of format consistency
| failedSpellCast = true; // Mark spellcast as failed so we can retry it later | ||
|
|
||
| if (me->IsRooted()) // Rooted inhabit type, never move/reposition | ||
| if (me->IsRooted() || !IsSmart()) // Rooted inhabit type, never move/reposition |
There was a problem hiding this comment.
I haven't tested with an ASSERT for this, but maybe that would be better then have a silent bug?
There was a problem hiding this comment.
There was a problem hiding this comment.
We also have ASSERT here.
Changes Proposed:
This PR proposes changes to:
AI-assisted Pull Requests
Important
While the use of AI tools when preparing pull requests is not prohibited, contributors must clearly disclose when such tools have been used and specify the model involved.
Contributors are also expected to fully understand the changes they are submitting and must be able to explain and justify those changes when requested by maintainers.
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.