Fix bot account creation on DB with existing player accounts before mod init, refactor and improve readability of functions

[icemansparks]

This pull request fixes a problem with the bot account creation in case of pre-existing player accounts / characters.
closes: #449
maybe also address: #815

refactors and optimizes the [RandomPlayerbotFactory.cpp] file to improve readability, maintainability, and testability.
The changes include splitting the CreateRandomBot function into dedicated functions, adding error handling, improving logging, and optimizing the code.

Changes Made

1) Refactored CreateRandomBot Function:
Split the CreateRandomBot function into dedicated functions:

• GenerateRandomGender: Generates a random gender.
• SelectRandomRace: Filters available races based on the alliance flag and selects a random race.
• GetRandomBotName: Creates or retrieves a random bot name from the name cache.
• CollectCustomizationOptions: Collects customization options (skin color, face, hair, facial hair) based on the race and gender.
• SelectRandomCustomizationOptions: Selects random customization options from the collected options.
• CreateAndInitializePlayer: Creates and initializes the player object with the generated name, race, class, gender, and customization options.

2) Improved Error Handling and Logging:

• Added error handling to ensure that any issues are reported.
• Improved logging to provide more detailed information about the process.

3) Optimized Code:

• Used std::move to avoid unnecessary copying of strings.
• Used std::uniform_int_distribution for better random number generation.

[nl-saw]

Thanks for raising a PR. I just would like to point out that no one will be able to test this as it isn't compiling due to an uncommitted header file.

[icemansparks]

For testing I made sure that:

• I have exissting player / admin accounts
• The existing accounts have characters on them

I tested:

• Compiling
• starting up the server with playerbots enabled
• verified account, character and playerbots_random_bots entries are chreated with expected IDs and references
• restarted with "delete bots" option to verify all bots get deleted and no player account or character is being removed
• restarted again with bots active to verify bot generation

One Problem I had: the first bot account seems to be skipped in character creation:

I'll try to fix this today.

I observed:

• Errors on restart after deting bots since apparently some entries for bots will not be deleted (like entries in character_action, pet_spell or the arena team association, possibly more as well - this should probably be an optimization in a separate issue)
• A weird error with the message that an accoutn cannot be delted because it isn't a bot account, while the stated account ID was a bot account and the account was deleted in the process as expected... I'll try to figure this out today as well.
  Deleting account accID: 816(19)... Deleting characters for account accID: 817(20)... Account ID 817 is not a bot account. Skipping character deletion.

[icemansparks]

I tested again and cannot reproduce the previuosly problem of characters being only created for the second and thrird bot account - all following initialisations and removals went as expected (as far as i can tell).

I think this might be ready to be reviewed. :)

Btw. i have no clue why the windows build is failing in CI.

[liyunfan1223]

Some log info and condition checks should be fixed.

Calling AccountMgr::DeleteAccount should also delete all characters under the account at the same time. So is DeleteBotCharacter redundant?

In addition, there is also a known issue that the database cannot be completely cleared when deleting the account, so sometimes we need scripts in https://github.com/liyunfan1223/mod-playerbots/wiki/Playerbot-Queries#clear-bots. It would be best if the scripts could be added to the deletion process, or if there were other methods to ensure a complete clearing of the database. It is not required in this PR, I just raised this issue if you are interested.

[icemansparks]

AccountMgr::DeleteAccount

I'll have a look at this and the requested changes as well - it seems like I can probably get rid of DeleteBotCharacter in the end.

[icemansparks]

https://github.com/liyunfan1223/mod-playerbots/wiki/Playerbot-Queries#clear-bots.

I also left a comment in the code to add a cleanup method which could handle removing of outdated botaccount reference data like mentioned in the link.

I don't have time in the next two weeks, but will probably have a look in february again.

[nl-saw]

https://github.com/liyunfan1223/mod-playerbots/wiki/Playerbot-Queries#clear-bots.

I also left a comment in the code to add a cleanup method which could handle removing of outdated botaccount reference data like mentioned in the link.

I think the following makes more sense:

USE acore_playerbots;
DELETE FROM acore_playerbots;

[icemansparks]

https://github.com/liyunfan1223/mod-playerbots/wiki/Playerbot-Queries#clear-bots.

I also left a comment in the code to add a cleanup method which could handle removing of outdated botaccount reference data like mentioned in the link.

I think the following makes more sense:

USE acore_playerbots; DELETE FROM acore_playerbots;

I agree, but its not only the data in the dedicated table, but as mentioned by @liyunfan1223 before, there are sometimes problems with deleting botAccounts where some things don't get deleted properly.
I observed this as well, when continuosly creating and deleting accounts - sometimes upon creating accounts in the same database again leads to error with pet_spell or arena team association (for example) as the entries are still present and have not been deleted before when the account was deleted. (I can't tell yet if this is a general problem in the core, or just a problem of bot accounts)
However, as staed before, this should probably be a dedicated issue :)

[nl-saw]

acore_playerbots;

I meant specifically for this table, I think there's already an idea to refactor the bot deletion so it is done proper from the server itself and no scripts required anymore.

[liyunfan1223]

After testing, there are still some things that are not as expected. Account creation is a relatively fragile part, and I'm worried that this refactor will cause more unpredictable issues.

I re-read the issues you mentioned, and I think they have nothing to do with account creation. A simple way is to clear acore_playerbots.playerbots_random_bots by .playerbot rndbot reset command after the database is modified. The server will automatically repopulate this table after restart, and it should work normally. I plan to submit a PR to clear the add value in playerbots_random_bots every time server restarts, which should solve these issues.

[icemansparks]

After testing, there are still some things that are not as expected. Account creation is a relatively fragile part, and I'm worried that this refactor will cause more unpredictable issues.

I re-read the issues you mentioned, and I think they have nothing to do with account creation. A simple way is to clear acore_playerbots.playerbots_random_bots by .playerbot rndbot reset command after the database is modified. The server will automatically repopulate this table after restart, and it should work normally. I plan to submit a PR to clear the add value in playerbots_random_bots every time server restarts, which should solve these issues.

I'll gladly have a look at that PR as well and test if it addresses the mentioned problems.
I am pretty sure however, that just resetting playerbots is not resolving the original issues regarding existing "non-bot accounts" mentioned in the bugreport.

Since the problem (at least form my tests) was, that adding entries in acore_playerbots.playerbots_random_bots does currently not take into account if there are existing non-bot-accounts / doesn't make sure that the assignment of the enrty is only inked to the bot account it should be assigned to (since it uses a simple counter variable instad of the account ids for referencing).

Maybe there is a much easier fix to this without reworking as much as I proposed in this PR though 🤔 (still learning how the different processes of the mod work together and may miss some insights).
I understand that the creation is a fragile and important part of the process and should not be changed lightly or without proper testing of course.

[liyunfan1223]

My thought is that non-bot characters will never be added to playerbots_random_bots. If it happens due to accidental reasons such as importing manual sqls, just clear playerbots_random_bots table. This table will be dynamically regenerated based on the current accounts and chars.

PR is here: #913

[icemansparks]

My thought is that non-bot characters will never be added to playerbots_random_bots. If it happens due to accidental reasons such as importing manual sqls, just clear playerbots_random_bots table. This table will be dynamically regenerated based on the current accounts and chars.

PR is here: #913

The issue was never caused by any manual interaction with the playerbots_random_bots table, but rather by the function that adds data to this table based on random bot account count while the account referenced in the entry is not the correct one (Caused by using loop variables as reference for the account in this table instead of using the actual account id).

As described in the original issue, this can be easily reproduced by creating a normal player / admin account before enabling playerbots.

I'll have a look at your PR as soon as I can and test this scenario with your changes as well.

[liyunfan1223]

Can you point out where the loop variable is? If you are talking about the id column of playerbots_random_bots, this is just an auto-incremented primary key. It does not represent the account id. The only identifier recorded in the table is the guid of characters.

[icemansparks]

I tried to point it out here: #449 (comment) - but at the time i did not dive into the code that much yet.

I am trying to reproduce the behavior which i observed in august 2024, but can't get the same results. Since there are a ton of new commits since my original bug-report, it will be tedious to find out what went wrong on my end at this point.

As I could safely reproduce the error back then, i came to the conclusion that the problem did lie in the following loop

Nonetheless, this screenshot is now taken from the commit I referenced in the original report and I cannot seem to find any problem with it... leaving me wondering how I ended up with players' character guids in playerbots_random_bots table in the first place...

I'll only be able to investigate this further next week though.

[icemansparks]

I would not have put all the effort in this PR if there hadn't been (in my eyes) a serious problem when enabling playerbots / adding playerbots to an existing playerbase.

@liyunfan1223 honestly, I'll probably have to revisit the whole thing from scratch and if there is still something to find, I'll make a fresh PR with a smaller amount of changes in the upcoming weeks.
This way we don't need to worry about too many adjustments in the account / character creation process at once.

Of course, if you don't see any more problems with the current PR, I am happy if the work doesn't go to waste - but if there is any uncertainty, lets close it for the better :)

Still, a bit refactoring of the creation process would probably be good to have it structured and more maintainable as it really took some time to get into all the things happening in this process and I think it could be easier with the use of smaller functions for some of the included tasks.

[liyunfan1223]

I appreciate your improvement to the readability of the code, but it actually introduces potential issues that, although a large part has been fixed, I don't think have been fully fixed yet.

We have spent a lot of effort, and my preference is to close this PR first. We need to clarify the purpose of the PR. If it is just for readability, we should not make numerous modifications to the logic without full assurance.

[liyunfan1223]

Order by on username is unreasonable, consider rndbot99>rndbot300.

[icemansparks]

Closed after discussion. Maybe there will be minor readability improvements without larger changes in a dedicated new PR in the future.