fix(Core/SmartAI) : use explicit stack DS for ProcessAction instead of recursion #16739
Conversation
yehonal-bot
added
CORE
|
@Winfidonarleyan @Nefertumm Can one of you review this please? :) |
|
@walkline you got any opinion on this? |
|
|
||
| class SmartScript | ||
| { | ||
| using SmartScriptFrame = std::tuple<SmartScriptHolder&, Unit*, uint32, uint32, bool, SpellInfo const*, GameObject*>; |
There was a problem hiding this comment.
Interesting solution, but for such a long list of parameters, I would prefer to use a struct type.
| SmartScriptHolder FindLinkedEvent (uint32 link) | ||
| std::optional<std::reference_wrapper< | ||
| SmartScriptHolder>> FindLinkedEvent(uint32 link) | ||
| { | ||
| if (!mEvents.empty()) | ||
| { | ||
| for (SmartAIEventList::iterator i = mEvents.begin(); i != mEvents.end(); ++i) | ||
| { | ||
| if (i->event_id == link) | ||
| { | ||
| return (*i); | ||
| return std::ref(*i); | ||
| } | ||
| } | ||
| } | ||
| SmartScriptHolder s; | ||
| return s; | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Lol, I was wondering why it was implemented that way.
I thought maybe it was because the object’s lifetime is short and we need to work with copies of the objects.
But then I noticed that we're not clearing the mEvents vector 😅
So, good catch!
|
@Dantsz can you please address the latest review :) |
Will try to find time to do so |
|
Thanks for the update! So how can we go about testing this PR - to my understanding this basically effects all SmartAI handling? Or just SmartAI with linked events? Can we expect a performance gain here or is this more of a refactoring into a "cleaner" handling? |
It does affect all scripts, I don't think script length would be relevant outside of the long scripts which crash, so testing it on smaller scripts should be fine.
Performance impact should be small and probably is platform dependent, the memory footprint could be larger, it's not a cleaner handling, as the links should be processed in the same order and with the same parameters, it's just moving memory usage from the stack (recursion) to the heap so we don't have a stack overflow when the links get big. |
sudlud
added
Tested
…f recursion (azerothcore#16739) * Use dequeue instead of recursion * Remove to do comments * Fix formatting * Fix more formatting :( * Use references instead of copies in the stack to correctly update event state * formatting * Revert FindLinkedEvent parameter name change and check for event type * Fix event processing in SmartScript::UpdateTimer * Use struct for defining SmartScriptFrame instead of tuple * Fix emplace_back not working on default constructor on clang 15 * Fix const placement
|
SMART_EVENT_LINK SAI entries currently do not seem to be working properly. Is it possible this commit caused it? I have written multiple scripts in last few months and whenever I have added a SMART_EVENT_LINK entry those linked entries would not execute. |
Changes Proposed:
SmartScript::ProcessEventrecursivelySmartScript::ProcessEventsForuses the deque to process eventsIssues Addressed:
SOURCE:
Before:
Desktop.2023.07.11.-.21.55.15.01.mp4
After:
No longer crashes
Tests Performed:
How to Test the Changes:
Run in Debug, MSVC (I have 19.37.32705.0 installed)
Scenario 1:
Scenario 2(video):
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.