<!--
> [!WARNING]
> **This is a DRAFT PR.**
> The structure is not definitive. The code might not be optimised yet.
It might not even start nor compile yet.
> **Don't panic. ✋ It's going to be ok. 👌 We can make modifications, we
can fix things.** 😁
-->
# Description
This PR aims to refactor the NextAction declaration to achieve two
goals:
## Eliminate C-style sentinel arrays
Currently, a double pointer (`NextAction**`) approach is being used.
This an old pre-C++11 (< 2011) trick before `std::vector<>` became a
thing.
This approach is painful for developers because they constantly need to
declare their `NextAction` arrays as:
```cpp
NextAction::array(0, new NextAction("foo", 1.0f), nullptr)
```
Instead of:
```cpp
{ new NextAction("foo", 1.0f) }
```
The first argument of `NextAction::array` is actually a hack. It is used
to have a named argument so `va_args` can find the remaining arguments.
It is set to 0 everywhere but in fact does nothing. This is very
confusing to people unfamiliar with this antiquated syntax.
The last argument `nullptr` is what we call a sentinel. It's a `nullptr`
because `va_args` is looking for a `nullptr` to stop iterating. It's
also a hack and also leads to confusion.
## Eliminate unnecessary pointers for `NextAction`
Pointers can be used for several reasons, to cite a few:
- Indicate strong, absolute identity.
- Provide strong but transferable ownership (unlike references).
- When a null value is acceptable (`nullptr`).
- When copy is expensive.
`NextAction` meets none of these criteria:
- It has no identity because it is purely behavioural.
- It is never owned by anything as it is passed around and never fetched
from a registry.
- The only situations where it can be `nullptr` are errors that should
in fact throw an `std::invalid_argument` instead.
- They are extremely small objects that embark a single `std::string`
and a single `float`.
Pointers should be avoided when not strictly necessary because they can
quickly lead to undefined behaviour due to unhandled `nullptr`
situations. They also make the syntax heavier due to the necessity to
constantly check for `nullptr`. Finally, they aren't even good for
performance in that situation because shifting a pointer so many times
is likely more expensive than copying such a trivial object.
# End goal
The end goal is to declare `NextAction` arrays this way:
```cpp
{ NextAction("foo", 1.0f) }
```
> [!NOTE]
> Additional note: `NextAction` is nothing but a hacky proxy to an
`Action` constructor. This should eventually be reworked to use handles
instead of strings. This would make copying `NextAction` even cheaper
and remove the need for the extremely heavy stringly typed current
approach. Stringly typed entities are a known anti-pattern so we need to
move on from those.
### Update :Thank you to @notOrrytrout from prompting me to work on
this. Its been a huge learning experience.
With @notOrrytrout I started working on enabling bot fishing with
master, but also on their own.
The first commit didnt crash, showing that it was possible to have a bot
cast when master does. Currently it compiles but crashes when you try to
fish with a bot in the group, whether the bot has fishing or not. It
makes me think that the check in FishingValues is broken somehow, but I
cant figure out how.
---------
Co-authored-by: bash <31279994+hermensbas@users.noreply.github.com>
* Revert "[Large server fix] #1537 Serialize playerBots/botLoading with a mutex and use snapshot-based loops to fix concurrency crashes (#1540)"
This reverts commit 3fff58df1a.
* Revert "[Fix] teleport to invalid map or invalid coordinates (x , y , z 200000, o ) given when teleporting player (g UI d full type player low , name , map , x , y , z , o ) (#1538)"
This reverts commit ca2e2ef0db.
* Revert "Fix: prevent MoveSplineInitArgs::Validate velocity asserts (velocity > 0.01f) for bots, pets, and charmed units (#1534)"
This reverts commit 4e3ac609bd.
* Revert "[Fix issue #1527] : startup crash in tank target selection — add TOCTOU & null-safety guards (#1532)"
This reverts commit c6b0424c29.
* Revert "[Fix issue #1528] Close small window where the “in a BG/arena” state can change between the check (InBattleground() / InArena()) and grabbing the pointer (GetBattleground()), which leads to a null dereference. (#1530)"
This reverts commit 2e0a161623.
* Revert "Harden playerbot logout & packet dispatch; add null-safety in chat hooks and RPG checks (#1529)"
This reverts commit e4ea8e2694.
* Revert "Dont wait to travel when in combat. (#1524)"
This reverts commit ddfa919154.
* Revert "nullptr fix (#1523)"
This reverts commit 380312ffd2.
* Revert "Playerbots/LFG: fix false not eligible & dungeon 0/type 0, add clear diagnostics (#1521)"
This reverts commit 872e417613.
* Revert "nullptr exception (#1520)"
This reverts commit 3d28a81508.
* Revert "Removed bot freezing at startup and system message, not relevant anymore (#1519)"
This reverts commit bcd6f5bc06.
PlayerbotAI::FindOilFor was making the server randomly crashing
ChooseTravelTargetAction::getNewTarget: when active bot groupping was making the server crash as looking for unexisting params
Several bug fixes and tweak to Quest and Group
New fucntionnality:
Bots will now share quests randomly to their party
Bots will try to accomplish group member quest before moving on to new target
Bots will try to sells items only after few levels ( 5 ) when in group
When dropping a quest bots will try to select a new one they are on instead of idling for few time
Bots will no longuer try to invite themselfs to group or if group is full
Bots are now allowed to leave party by themself
Bots in groupe if not leader are forbbiden to tag in bgs
Bots in bot-groups no have a more limited range to look for grind target
Polish logs
