Item storage rework

[sanctum32]

Changes Proposed:

This PR proposes changes to:

• This PR is initial rewrite of item storage. This work replaces raw pointer usage with shared_ptr pointer usage, where shared_ptr prevents deallocation, lifetime, multithreading and potential memory leak issues.

Information about shared_ptr can be found at https://en.cppreference.com/w/cpp/memory/shared_ptr

Issues Addressed:

• Closes

Tests Performed:

This PR has been:

• Tested in-game by the author.
• Tested in-game by other community members/someone else other than the author/has been live on production servers.
• This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

• This pull request can be tested by following the reproduction steps provided in the linked issue
• This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

To correctly test this pr, you must use runtime debugger and memory analysis tools (valgrind or memaloc sanitizer)
Any item operation can be viable to perform tests, like:

1. Item trading
2. Item destruction:
   2.1) removing item from bag
   2.2) Destroying item with disenchanting
   2.3) Destroying item with milling
   2.4) Destroying item as reagent for buffs
   2.5) Destroying item as reagent for crafting (blacksmithing, alchemy and etc)
   2.6) Selling item to vendor and then logging out. Buyout tab should be empty after relog
3. Test item with buyout - sell item to vendor and buyback without relogging in game
4. Test item in auction house (deposit, buyout or auction cancellation)
5. Try to send & receive item via mail:
   5.1) Try to send item with and without empty sockets
   5.2) Try to send item with and without permament and temporal enchants
   5.3) Try to split item (stackable items, like reagents)
6. Basic player relogging
7. Basic player teleportation
8. Player kick from world
9. Try item spell casts, like trinkets, scrolls or other temporal (or permament) items like potions or permament flasks

At base analyse memory usage, in case you can create custom item destructor to analyse destructor (for console output or debugging points).
At base, due shared_ptr mechanic, shared_ptr object is destroyed when atomic counter reaches 0

Known Issues and TODO List:

• optimize this pr and add std::move calls to avoid non necessary atomic counter increments (good for performance)
• add template based function to add container security, which would prevent several containers keeping memory address. For example, to ensure that item is not stored in bank and in player's inventory at same time
• Add custom destructor to help for analysis (only for WITH_CORE_DEBUG cmake option)

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.

[sanctum32]

I'm sorry about this, i'm using clangd in vscode and it generates temporal idx files for indexing, I will revert this line change once pr state will get changed from draft pull request to normal pull request

[Grimdhex]

I see changes in AuctionHouseMrg, take care that we plan to merge this PR: #20830

[Takenbacon]

Why a shared_ptr instead of unique_ptr? Are there any scenarios where an Item object should be owned by multiple objects?

[sanctum32]

Why a shared_ptr instead of unique_ptr? Are there any scenarios where an Item object should be owned by multiple objects?

item can have multiple ownerships in functions and containers, for example at item copy code, trade data or especially in spell cast data, where item pointer should be alive for spell memory lifetime. Even if item is removed from main containers, memory gonna stay valid where pointer was passed

[Takenbacon]

Why a shared_ptr instead of unique_ptr? Are there any scenarios where an Item object should be owned by multiple objects?

item can have multiple ownerships in functions and containers, for example at item copy code, trade data or especially in spell cast data, where item pointer should be alive for spell memory lifetime. Even if item is removed from main containers, memory gonna stay valid where pointer was passed

I can see it going both ways honestly so I'm not so sure either way.

A unique_ptr is to dictate there is a single owner at a time who manages the object, which "theoretically" should be the case for Items, but yes, there are cases where an Item can be stored in a temporary list. You could get around that by just passing the raw pointer and leave the underlying type a unique_ptr but then obviously you'd lose some of the benefits in that case.

However, at the same time, I've personally seen poorly implemented shared_ptr usages cause more problems than they solve by creating resource leaks and hiding bugs (e.g. a list not being cleared some where, the object will never be deleted). Use of a weak_ptr and passing Item by reference rather than pointer in as many places as possible would address that but I don't see either used here.

[sanctum32]

Why a shared_ptr instead of unique_ptr? Are there any scenarios where an Item object should be owned by multiple objects?

item can have multiple ownerships in functions and containers, for example at item copy code, trade data or especially in spell cast data, where item pointer should be alive for spell memory lifetime. Even if item is removed from main containers, memory gonna stay valid where pointer was passed

I can see it going both ways honestly so I'm not so sure either way.

A unique_ptr is to dictate there is a single owner at a time who manages the object, which "theoretically" should be the case for Items, but yes, there are cases where an Item can be stored in a temporary list. You could get around that by just passing the raw pointer and leave the underlying type a unique_ptr but then obviously you'd lose some of the benefits in that case.

However, at the same time, I've personally seen poorly implemented shared_ptr usages cause more problems than they solve by creating resource leaks and hiding bugs (e.g. a list not being cleared some where, the object will never be deleted). Use of a weak_ptr and passing Item by reference rather than pointer in as many places as possible would address that but I don't see either used here.

actually passing shared_ptr as reference improperly could cause that issue, i instead used assign copy way, it doesn't creates actual memory copy, but increments atomic counter, so passing it as shared_ptr value would ensure pointer lifetime with unique counter id, in some areas i already used std::move call which allowed to skip making new atomic counter id and just pass existing counter id.

Again, passing shared_ptr as value increments only atomic counter, it doesn't create memory copy.

Major area where shared_ptr could fail is when object is placed into container which is never cleaned (global containers for example). This would be actual memory leak, because counter in such scenario would never reach 0

[Takenbacon]

This is just my personal opinion but I just don't see it worth it as a shared_ptr. This is worth a read as a reference: https://stackoverflow.com/questions/37801184/new-to-c11-features-proper-use-of-shared-ptr

[sanctum32]

This is just my personal opinion but I just don't see it worth it as a shared_ptr. This is worth a read as a reference: https://stackoverflow.com/questions/37801184/new-to-c11-features-proper-use-of-shared-ptr

https://cplusplus.com/forum/general/65888/#msg355856 pretty much was working around this case, also "Effective C++: 55" book has some ideas for handling similar cases

BUT i do not disagree on reference usage, just extra weak_ptrs will need to be made then, i think std::move can be used for this case, but has to be carefully made, need to avoid "use after move" scenario

[Grimdhex]

I'm agree to use a unique_ptr instead. We can't said that we have a good pointer management and even if you know what you do, it's important to think for the future if you are not here. We have contributor that are mostly non-experts

I know that TC have done a similar work and choice also to use unique_ptr. It's also my personal opinion but I prefer to say instead

[sanctum32]

I'm agree to use a unique_ptr instead. We can't said that we have a good pointer management and even if you know what you do, it's important to think for the future if you are not here. We have contributor that are mostly non-experts

I know that TC have done a similar work and choice also to use unique_ptr. It's also my personal opinion but I prefer to say instead

i explained every reason why shared_ptr has to be used. Trinitycore themselves didn't applied unique_ptr on items yet and i explained multiple times that items can have several ownerships... Literally I don't know what other explanations are needed...

This maybe could clearify:
https://stackoverflow.com/questions/7657718/when-to-use-shared-ptr-and-when-to-use-raw-pointers
https://books.goalkicker.com/
https://learn.microsoft.com/en-us/cpp/cpp/how-to-create-and-use-shared-ptr-instances?view=msvc-170

[Grimdhex]

I will add a feedback if you want. We may be wrong ^^

@Winfidonarleyan question around to use a shared or unique ptr approach for items. What is your opinion ?

[sanctum32]

In addition, before my change, items were using self destruction model when ITEM_REMOVED state was set, several instances could set that state before my change and item object memory gets deleted and it easily could go to undefined behavior area. Item itself doesn't require player class to be deleted on self destruction.

Lets say we use unique_ptr, spell cast removes player item from player related storage, while item still is needed/assigned in spell class, if unique_ptr pointer triggers reset call (assigning null also works, or basic smart pointer destructor call) without doing release() call, other related pointer calls will have nullptr memory, even if there was nullptr checks before. What i mean, several threads would conflict

Main reason why i want to use shared_ptr is to ensure pointer memory lifetime in all calls, because several threads may can effect main storage, could cause conflict in other threads and object removal could cause undefined behavior in other threads. I also could use mutex locking too. Also there is several areas where same item could be stored (several ownerships), i could use reference based or raw pointer storage in other areas but from reading code, i have no guarantee that pointer memory wont get erased before other threads finishes their tasks.

[Winfidonarleyan]

Need add logs about this

[Winfidonarleyan]

Move to all inclues

[sanctum32]

i wouldn't move to global includes, because that would slow down compile time, would increase binary size and i think that can make non necessary header usage in other areas

[sanctum32]

Edit: i missunderstood a little, i will move inclusion above code comment. At base i thought you meant to move to global header (like pch header or similar)

[Winfidonarleyan]

I will add a feedback if you want. We may be wrong ^^

@Winfidonarleyan question around to use a shared or unique ptr approach for items. What is your opinion ?

I would use unique. As far as I remember, there is no need to store items in different places