feat(Core/AuctionHouse): rework auction house #17644
Conversation
github-actions
bot
added
CORE
|
Hi @Winfidonarleyan , looks like a ton of work went into this! Is there anything in specific we should test as far as functionality, features, or differences in performances between master and this PR? I don't necessarily have the C++ chops to review all your changes, but I'd be happy to put some time into testing with my fairly-large (25k items) AH. EDIT: I see this is still a draft, let us know if it's not quite ready for testing yet. |
I will prepare the PR and let you know what you need to test |
Winfidonarleyan
force-pushed
the
async-auction-rework
branch
from
5951313 to
638ec11
Compare
|
@kjack9 Done, You can take a look, if there's anything you don't understand, ask away |
| thread.join(); | ||
| } | ||
|
|
||
| /*static*/ AsyncAuctionMgr* AsyncAuctionMgr::instance() |
There was a problem hiding this comment.
Why comment it? If not used as static we can just delete it?
|
|
||
| if (_packet->Bid > MAX_MONEY_AMOUNT || _packet->Buyout > MAX_MONEY_AMOUNT) | ||
| { | ||
| LOG_ERROR("network", "WORLD: HandleAuctionSellItem - Player {} ({}) attempted to sell item with higher price than max gold amount.", player->GetName(), player->GetGUID().ToString()); |
There was a problem hiding this comment.
Should this really be classed as network? Why dont we combine all auctionhouse related messages into an auctionhouse logger?
|
|
||
| if (listItems->GetAll || (listItems->IsNoFilter() && packet.WSearchedName.empty())) | ||
| { | ||
| ForEachAuctions([&packet](AuctionEntry* auction) |
There was a problem hiding this comment.
I was working on improving this, and added a AuctionShortlist.reserve() here.
Because we know how many auctions we have, we can reserve beforehand to save a bit of allocation time
|
Conflict |
Winfidonarleyan
force-pushed
the
async-auction-rework
branch
from
20444db to
bfc7ada
Compare
|
@azerothcore/testers A good PR that needs testing |
|
@Winfidonarleyan, can you confirm if this PR resolves #5068? |
Yes |
| if (!player) | ||
| return; | ||
|
|
||
| auto session{ player->GetSession() }; |
There was a problem hiding this comment.
@Winfidonarleyan It looks like async tasks are still thread unsafe.
If I understood correctly this task can be executed in parallel with a map update for example.
And if that is true, then you can't use player/session objects in several threads without synchronisation mechanisms (mutexes/locks/semaphores).
In your shoes I would try to copy all required player/session related data into a new object(s) during enqueueing/scheduling a task. And use this new object inside of these tasks instead of real player/session objects.
|
conflicts |
|
abandoned |
Kitzunu
added
the
PR Abandoned
Continue #15334
Changes Proposed:
ProducerConsumerQueueCMSG_AUCTION_SELL_ITEMCMSG_AUCTION_PLACE_BIDCMSG_AUCTION_LIST_BIDDER_ITEMSAuction.Async.ThreadsIssues Addressed:
SOURCE:
Tests Performed:
This PR has been:
How to Test the Changes:
TODO testing:
Auction.Async.ThreadsKnown 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.