Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Appearance Collection #72

Merged
merged 11 commits into from
Mar 13, 2022
Merged

Conversation

ZhengPeiRu21
Copy link
Contributor

This PR is implement a Appearance Collection system like later expansions. When an item is equipped or a quest is completed, data is saved to the account. Players can use any of the items they have collected as a transmog candidate, rather than only items in the bag. If this is disabled through config option, behavior will be the same as before and require items in bag to be transmog.
chatMessage
appearanceSelection

Because players will now have many more items available for transmog, I have also implemented paging in the gossip menu so that they can browse through many items.

I have created a client addon that can track which item still need to have appearance collected and show tooltip:
newAppearanceTooltip

I also have modified version of MogIt that can show collected appearance:
mogItToolTip

Note that these addons still need to manually sync data with server, but this is a separate addon issue not related for this PR.

@r-o-b-o-t-o
Copy link
Member

Hi, just a quick few comments after trying the PR out:

  • Paging doesn't seem to work on my local realm. I have approximately 50 chest items in my custom_unlocked_appearances table. There are 23 gossip items displayed on the first page (instead of 25?) and the "Next Page" button shows an empty second page.
  • It would be nice to have a "Previous Page" button.
  • You could also add OnLootItem, OnCreateItem and OnAfterStoreOrEquipNewItem to PlayerScript. This way we could add items to the collection automatically without needing to equip them. Of course for these hooks you would need to check if the item is soulbound, for example with:
if (item->GetTemplate()->Bonding == ItemBondingType::BIND_WHEN_PICKED_UP || item->IsSoulBound())
{
    // AddToDatabase(...);
}

Thank you for your contribution! Keep up the good work 👍🏻

@ZhengPeiRu21
Copy link
Contributor Author

Hi, just a quick few comments after trying the PR out:

* Paging doesn't seem to work on my local realm. I have approximately 50 chest items in my `custom_unlocked_appearances` table. There are 23 gossip items displayed on the first page (instead of 25?) and the "Next Page" button shows an empty second page.

* It would be nice to have a "Previous Page" button.

* You could also add `OnLootItem`, `OnCreateItem` and `OnAfterStoreOrEquipNewItem` to `PlayerScript`. This way we could add items to the collection automatically without needing to equip them. Of course for these hooks you would need to check if the item is soulbound, for example with:
if (item->GetTemplate()->Bonding == ItemBondingType::BIND_WHEN_PICKED_UP || item->IsSoulBound())
{
    // AddToDatabase(...);
}

Thank you for your contribution! Keep up the good work 👍🏻

Thank you for the feedbacks! With the paging, it is 24 items per page, but because the "usable for transmog" filter takes place after get data from the DB, it is possible for one page to have less or even no items. Are your items appear if you press Next Page even after the empty page? Working around this would require a heavier DB call because we would need to fetch all items before filtering, but maybe is worth the trouble to avoid confusing user experience. I am just hope not to too affect server performance!

Previous page is easy to add and a good idea. It will reduce items per page by 1 but that is not too large concern, I think. I will add this.

I can also add the other hooks for other methods of obtain soulbound items. I have not played retail server for a long time, so I did not know it is correct behavior to add soulbound item before equip it!

@ZhengPeiRu21
Copy link
Contributor Author

I have pushed some change that improve paging logic. Now every page will only fill with acceptable item, and next page will only appear when one page is full. Previous page is also implemented. I also added the item collection hooks suggested by @r-o-b-o-t-o. Please let me know if you are think of any more improvements you would like to see!

Copy link
Member

@r-o-b-o-t-o r-o-b-o-t-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are using the character's guid instead of the account ID. I'm assuming you wanted to make the collection account-bound since you named the column account_id in the table custom_unlocked_appearances 😄

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Axel Cocat <[email protected]>
@ZhengPeiRu21
Copy link
Contributor Author

You are using the character's guid instead of the account ID. I'm assuming you wanted to make the collection account-bound since you named the column account_id in the table custom_unlocked_appearances 😄

Oops, you are right! Thank you for the fix suggestions!

@r-o-b-o-t-o
Copy link
Member

r-o-b-o-t-o commented Mar 3, 2022

Ah, I understand now why paging didn't seem to work for me when I posted my first comment: paging always redirects you to the Head category.
For example, if you are in the Chest category, then click "Next Page" or "Previous Page", it will then show the page with the helmets:
transmog-paging-bug

Apart from this issue, good job on the paging, it's a neat feature 💜

@ZhengPeiRu21
Copy link
Contributor Author

Oh no, that is a problem! That is happen because I only ever tested the Head slot. I will create a fix. Thank you again for spend time to test!

@r-o-b-o-t-o
Copy link
Member

Another thing, in my opinion it would be better to make the queries async, otherwise it could put an unnecessary strain on the server when players browse their collections or acquire a lot of items, since synchronous database queries make the core hang while waiting for the results.
Here's an example implementation:
(sorry, Github wouldn't let me post it as a review/suggestion since it's a bit large)

Show code diff
diff --git a/src/transmog_scripts.cpp b/src/transmog_scripts.cpp
index 3a2a995..f460a7e 100644
--- a/src/transmog_scripts.cpp
+++ b/src/transmog_scripts.cpp
@@ -342,6 +342,7 @@ public:
     {
         WorldSession* session = player->GetSession();
         Item* oldItem = player->GetItemByPos(INVENTORY_SLOT_BAG_0, slot);
+        bool sendGossip = true;
         if (oldItem)
         {
             uint32 price = sT->GetSpecialPrice(oldItem->GetTemplate());
@@ -351,67 +352,77 @@ public:
             ss << std::endl;
             if (sT->GetRequireToken())
                 ss << std::endl << std::endl << sT->GetTokenAmount() << " x " << sT->GetItemLink(sT->GetTokenEntry(), session);
+            std::string lineEnd = ss.str();
 
             if (sT->GetUseCollectionSystem())
             {
-                uint16 pageNumber = 0;
-                uint32 startValue = 0;
-                uint32 endValue = MAX_OPTIONS - 3;
-                bool lastPage = false;
-                if (gossipPageNumber > EQUIPMENT_SLOT_END + 10)
-                {
-                    pageNumber = gossipPageNumber - EQUIPMENT_SLOT_END - 10;
-                    startValue = (pageNumber * (MAX_OPTIONS - 2));
-                    endValue = (pageNumber + 1) * (MAX_OPTIONS - 2) - 1;
-                }
-                QueryResult result = CharacterDatabase.Query(
-                        "SELECT item_template_id FROM custom_unlocked_appearances WHERE account_id = {} ORDER BY item_template_id",
-                        player->GetSession()->GetAccountId());
-                if (result)
-                {
-                    std::vector<Item*> allowedItems;
-                    do {
-                        uint32 newItemEntryId = (*result)[0].Get<uint32>();
-                        Item *newItem = Item::CreateItem(newItemEntryId, 1, 0);
-                        if (!newItem)
-                            continue;
-                        if (!sT->CanTransmogrifyItemWithItem(player, oldItem->GetTemplate(), newItem->GetTemplate()))
-                            continue;
-                        if (sT->GetFakeEntry(oldItem->GetGUID()) == newItem->GetEntry())
-                            continue;
-                        allowedItems.push_back(newItem);
-                    } while (result -> NextRow());
-                    for (uint32 i = startValue; i <= endValue; i++)
+                sendGossip = false;
+
+                std::string query = "SELECT item_template_id FROM custom_unlocked_appearances WHERE account_id = " + std::to_string(player->GetSession()->GetAccountId()) + " ORDER BY item_template_id";
+                session->GetQueryProcessor().AddCallback(CharacterDatabase.AsyncQuery(query).WithCallback([=](QueryResult result)
                     {
-                        if (allowedItems.empty() || i > allowedItems.size() - 1)
+                        uint16 pageNumber = 0;
+                        uint32 startValue = 0;
+                        uint32 endValue = MAX_OPTIONS - 3;
+                        bool lastPage = false;
+                        if (gossipPageNumber > EQUIPMENT_SLOT_END + 10)
                         {
-                            lastPage = true;
-                            break;
+                            pageNumber = gossipPageNumber - EQUIPMENT_SLOT_END - 10;
+                            startValue = (pageNumber * (MAX_OPTIONS - 2));
+                            endValue = (pageNumber + 1) * (MAX_OPTIONS - 2) - 1;
                         }
-                        Item* newItem = allowedItems.at(i);
-                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, sT->GetItemIcon(newItem->GetEntry(), 30, 30, -18, 0) + sT->GetItemLink(newItem, session), slot, newItem->GetEntry(), "Using this item for transmogrify will bind it to you and make it non-refundable and non-tradeable.\nDo you wish to continue?\n\n" + sT->GetItemIcon(newItem->GetEntry(), 40, 40, -15, -10) + sT->GetItemLink(newItem, session) + ss.str(), price, false);
-                    }
-                }
-                if (gossipPageNumber == EQUIPMENT_SLOT_END + 11)
-                {
-                    AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Previous Page", EQUIPMENT_SLOT_END, 0);
-                    if (!lastPage)
-                    {
-                    AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", gossipPageNumber + 1, 0);
-                    }
-                }
-                else if (gossipPageNumber > EQUIPMENT_SLOT_END + 11)
-                {
-                    AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Previous Page", gossipPageNumber - 1, 0);
-                    if (!lastPage)
-                    {
-                    AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", gossipPageNumber + 1, 0);
-                    }
-                }
-                else if (!lastPage)
-                {
-                    AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", EQUIPMENT_SLOT_END + 11, 0);
-                }
+
+                        if (result)
+                        {
+                            std::vector<Item*> allowedItems;
+                            do {
+                                uint32 newItemEntryId = (*result)[0].Get<uint32>();
+                                Item* newItem = Item::CreateItem(newItemEntryId, 1, 0);
+                                if (!newItem)
+                                    continue;
+                                if (!sT->CanTransmogrifyItemWithItem(player, oldItem->GetTemplate(), newItem->GetTemplate()))
+                                    continue;
+                                if (sT->GetFakeEntry(oldItem->GetGUID()) == newItem->GetEntry())
+                                    continue;
+                                allowedItems.push_back(newItem);
+                            } while (result->NextRow());
+                            for (uint32 i = startValue; i <= endValue; i++)
+                            {
+                                if (allowedItems.empty() || i > allowedItems.size() - 1)
+                                {
+                                    lastPage = true;
+                                    break;
+                                }
+                                Item* newItem = allowedItems.at(i);
+                                AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, sT->GetItemIcon(newItem->GetEntry(), 30, 30, -18, 0) + sT->GetItemLink(newItem, session), slot, newItem->GetEntry(), "Using this item for transmogrify will bind it to you and make it non-refundable and non-tradeable.\nDo you wish to continue?\n\n" + sT->GetItemIcon(newItem->GetEntry(), 40, 40, -15, -10) + sT->GetItemLink(newItem, session) + lineEnd, price, false);
+                            }
+                        }
+                        if (gossipPageNumber == EQUIPMENT_SLOT_END + 11)
+                        {
+                            AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Previous Page", EQUIPMENT_SLOT_END, 0);
+                            if (!lastPage)
+                            {
+                                AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", gossipPageNumber + 1, 0);
+                            }
+                        }
+                        else if (gossipPageNumber > EQUIPMENT_SLOT_END + 11)
+                        {
+                            AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Previous Page", gossipPageNumber - 1, 0);
+                            if (!lastPage)
+                            {
+                                AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", gossipPageNumber + 1, 0);
+                            }
+                        }
+                        else if (!lastPage)
+                        {
+                            AddGossipItemFor(player, GOSSIP_ICON_CHAT, "Next Page", EQUIPMENT_SLOT_END + 11, 0);
+                        }
+
+                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/INV_Enchant_Disenchant:30:30:-18:0|tRemove transmogrification", EQUIPMENT_SLOT_END + 3, slot, "Remove transmogrification from the slot?", 0, false);
+                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/PaperDollInfoFrame/UI-GearManager-Undo:30:30:-18:0|tUpdate menu", EQUIPMENT_SLOT_END, slot);
+                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/Ability_Spy:30:30:-18:0|tBack...", EQUIPMENT_SLOT_END + 1, 0);
+                        SendGossipMenuFor(player, DEFAULT_GOSSIP_MESSAGE, creature->GetGUID());
+                    }));
             }
             else
             {
@@ -448,16 +459,19 @@ public:
                         if (sT->GetFakeEntry(oldItem->GetGUID()) == newItem->GetEntry())
                             continue;
                         ++limit;
-                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, sT->GetItemIcon(newItem->GetEntry(), 30, 30, -18, 0) + sT->GetItemLink(newItem, session), slot, newItem->GetGUID().GetCounter(), "Using this item for transmogrify will bind it to you and make it non-refundable and non-tradeable.\nDo you wish to continue?\n\n" + sT->GetItemIcon(newItem->GetEntry(), 40, 40, -15, -10) + sT->GetItemLink(newItem, session) + ss.str(), price, false);
+                        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, sT->GetItemIcon(newItem->GetEntry(), 30, 30, -18, 0) + sT->GetItemLink(newItem, session), slot, newItem->GetGUID().GetCounter(), "Using this item for transmogrify will bind it to you and make it non-refundable and non-tradeable.\nDo you wish to continue?\n\n" + sT->GetItemIcon(newItem->GetEntry(), 40, 40, -15, -10) + sT->GetItemLink(newItem, session) + lineEnd, price, false);
                     }
                 }
             }
         }
 
-        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/INV_Enchant_Disenchant:30:30:-18:0|tRemove transmogrification", EQUIPMENT_SLOT_END + 3, slot, "Remove transmogrification from the slot?", 0, false);
-        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/PaperDollInfoFrame/UI-GearManager-Undo:30:30:-18:0|tUpdate menu", EQUIPMENT_SLOT_END, slot);
-        AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/Ability_Spy:30:30:-18:0|tBack...", EQUIPMENT_SLOT_END + 1, 0);
-        SendGossipMenuFor(player, DEFAULT_GOSSIP_MESSAGE, creature->GetGUID());
+        if (sendGossip)
+        {
+            AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/INV_Enchant_Disenchant:30:30:-18:0|tRemove transmogrification", EQUIPMENT_SLOT_END + 3, slot, "Remove transmogrification from the slot?", 0, false);
+            AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/PaperDollInfoFrame/UI-GearManager-Undo:30:30:-18:0|tUpdate menu", EQUIPMENT_SLOT_END, slot);
+            AddGossipItemFor(player, GOSSIP_ICON_MONEY_BAG, "|TInterface/ICONS/Ability_Spy:30:30:-18:0|tBack...", EQUIPMENT_SLOT_END + 1, 0);
+            SendGossipMenuFor(player, DEFAULT_GOSSIP_MESSAGE, creature->GetGUID());
+        }
     }
 };
 
@@ -477,12 +491,16 @@ private:
         tempStream << std::hex << ItemQualityColors[itemTemplate->Quality];
         std::string itemQuality = tempStream.str();
         bool showChatMessage = !(player->GetPlayerSetting("mod-transmog", SETTING_HIDE_TRANSMOG).value);
-        QueryResult result = CharacterDatabase.Query("SELECT account_id, item_template_id FROM custom_unlocked_appearances WHERE account_id = {} AND item_template_id = {}", accountId, itemId);
-        if (!result) {
-            if (showChatMessage)
-                ChatHandler(player->GetSession()).PSendSysMessage(R"(|c%s|Hitem:%u:0:0:0:0:0:0:0:0|h[%s]|h|r has been added to your appearance collection.)", itemQuality.c_str(), itemId, itemName.c_str());
-            CharacterDatabase.Execute("INSERT INTO custom_unlocked_appearances (account_id, item_template_id) VALUES ({}, {})", accountId, itemId);
-        }
+
+        std::string query = "SELECT account_id, item_template_id FROM custom_unlocked_appearances WHERE account_id = " + std::to_string(accountId) + " AND item_template_id = " + std::to_string(itemId);
+        player->GetSession()->GetQueryProcessor().AddCallback(CharacterDatabase.AsyncQuery(query).WithCallback([=](QueryResult result)
+            {
+                if (!result) {
+                    if (showChatMessage)
+                        ChatHandler(player->GetSession()).PSendSysMessage(R"(|c%s|Hitem:%u:0:0:0:0:0:0:0:0|h[%s]|h|r has been added to your appearance collection.)", itemQuality.c_str(), itemId, itemName.c_str());
+                    CharacterDatabase.Execute("INSERT INTO custom_unlocked_appearances (account_id, item_template_id) VALUES ({}, {})", accountId, itemId);
+                }
+            }));
     }
 public:
     PS_Transmogrification() : PlayerScript("Player_Transmogrify") { }

This is basically the same thing, but with the code after the queries in AddToDatabase and ShowTransmogItems moved to an async callback. Plus a few adjustments to fit the lambdas.

@ZhengPeiRu21
Copy link
Contributor Author

Another thing, in my opinion it would be better to make the queries async, otherwise it could put an unnecessary strain on the server when players browse their collections or acquire a lot of items, since synchronous database queries make the core hang while waiting for the results. Here's an example implementation: (sorry, Github wouldn't let me post it as a review/suggestion since it's a bit large)
Show code diff

This is basically the same thing, but with the code after the queries in AddToDatabase and ShowTransmogItems moved to an async callback. Plus a few adjustments to fit the lambdas.

Thank you! This is very helpful lesson for learning how best to perform the async queries in these modules. I am sure I will use this many times in the future! I have applied your suggestions and everything seems to working for me.

@LukasVolgger
Copy link
Member

So, what's the status here? I like this PR very much and can test smthg if needed.

@ZhengPeiRu21
Copy link
Contributor Author

So, what's the status here? I like this PR very much and can test smthg if needed.

I think all suggested have been implemented and everything should now be work! Please let me know if there are other change you would like to see included.

@Helias
Copy link
Member

Helias commented Mar 8, 2022

Please, fix this build issue to make succeed the github pipeline.

[ 23%] Building CXX object src/server/scripts/CMakeFiles/scripts.dir/Commands/cs_arena.cpp.o
/home/runner/work/mod-transmog/mod-transmog/modules/mod-transmog/src/transmog_scripts.cpp:566:10: fatal error: 'OnAfterSetVisibleItemSlot' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    void OnAfterSetVisibleItemSlot(Player* player, uint8 slot, Item *item)
         ^
/home/runner/work/mod-transmog/mod-transmog/src/server/game/Scripting/ScriptMgr.h:1125:18: note: overridden virtual function is here
    virtual void OnAfterSetVisibleItemSlot(Player* /*player*/, uint8 /*slot*/, Item* /*item*/) { }
                 ^
1 error generated.

P.S. = good job, I strongly love this PR 🚀

@ZhengPeiRu21
Copy link
Contributor Author

P.S. = good job, I strongly love this PR 🚀

Thank you ^_^. I have fixed the issue with the override annotations and the build should now success.

@mountlin
Copy link

Another similar issue, please fix it again, looking forward to the PR being merged.

[ 22%] Building CXX object src/server/game/CMakeFiles/game.dir/AI/CoreAI/PassiveAI.cpp.o
/home/runner/work/mod-transmog/mod-transmog/modules/mod-transmog/src/transmog_scripts.cpp:618:10: fatal error: 'OnLogout' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    void OnLogout(Player* player)
         ^
/home/runner/work/mod-transmog/mod-transmog/src/server/game/Scripting/ScriptMgr.h:1056:18: note: overridden virtual function is here
    virtual void OnLogout(Player* /*player*/) { }
                 ^
1 error generated.

@r-o-b-o-t-o
Copy link
Member

Another similar issue, please fix it again, looking forward to the PR being merged.

This error is not related to this PR though, the OnLogout was already there before

@ZhengPeiRu21
Copy link
Contributor Author

Another similar issue, please fix it again, looking forward to the PR being merged.

This error is not related to this PR though, the OnLogout was already there before

Yes, the previous missing override was not in my changes either. But I will still fix it. ^_^ Haha.

I found several more override also missing and fixed as well. Now everything should build with no troubles!

@Helias Helias merged commit 3ca5bab into azerothcore:master Mar 13, 2022
@ZhengPeiRu21 ZhengPeiRu21 deleted the appearanceCollection branch March 13, 2022 16:19
@Yehonal
Copy link
Member

Yehonal commented Mar 21, 2022

Hi! I've seen this PR only now. Good Job!!
I was wondering if maybe it would be better to implement the list of the items with a search feature:
There's the possibility to show a text box and trigger an action right after, maybe that can be helpful to filter the list since there will be thousands of items eventually and it's impossible to browse them all

@Helias
Copy link
Member

Helias commented Apr 1, 2022

@ZhengPeiRu21, hey, I was wondering about the Addon, is there a beta release? or did you finish it?
I would like to try it

@ZhengPeiRu21
Copy link
Contributor Author

@ZhengPeiRu21, hey, I was wondering about the Addon, is there a beta release? or did you finish it? I would like to try it

Yes, there is a release I have shared in the Discord #custom-modules channel. (It maybe is hard to find now - it was many days ago!) I do not know if there is a better channel for release it. I could maybe create a GitHub repo for it.

Please let me know if you need guide with the addon. It keeps track separately of equipped items, so sometimes it might be out of sync with actual server data, but this will be corrected when item is equipped again or you manually update cached addon data.

@Helias
Copy link
Member

Helias commented Apr 2, 2022

@ZhengPeiRu21 thanks a lot, I strongly recommend you (and also ask) if you can do the github repository.

If you do it, I will link the repository under the mod-transmog repository and, if you add in your repository the github topic "azerothcore-tools", your repo will be added in the azerothcore catalogue.

Thanks!

@ZhengPeiRu21
Copy link
Contributor Author

@ZhengPeiRu21 thanks a lot, I strongly recommend you (and also ask) if you can do the github repository.

If you do it, I will link the repository under the mod-transmog repository and, if you add in your repository the github topic "azerothcore-tools", your repo will be added in the azerothcore catalogue.

Thanks!

Thank you for the suggestion. I have put the addons in a repository with some instructions to use: https://github.com/ZhengPeiRu21/transmog-addons

@Yehonal
Copy link
Member

Yehonal commented Jun 24, 2022

Hi! I've seen this PR only now. Good Job!! I was wondering if maybe it would be better to implement the list of the items with a search feature: There's the possibility to show a text box and trigger an action right after, maybe that can be helpful to filter the list since there will be thousands of items eventually and it's impossible to browse them all

Any news about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants