azerothcore-wotlk
azerothcore-wotlk copied to clipboard
azerothcore
fix(Core/Items): BoP stackable items can be traded among raid members.
Fixes #12400
Issues Addressed:
- Closes #12400
- Closes https://github.com/chromiecraft/chromiecraft/issues/3784
- Closes https://github.com/chromiecraft/chromiecraft/issues/3801
Tests Performed:
- Not tested.
How to Test the Changes:
make a group of 2 or more people. .tele group ZG Kill any boss, loot the Primal Hakkari Token. Have a look to ZG token tooltip.
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.
When an item is added to an existing stack, which rule prevails? Will the old stack be tradeable or will the new itens be impossible to trade?
Tested.
- I can trade quest items after accepting the quest. Such as Head of Ossirian the Unscarred, so many people can take the quest at once. Obviously I can't turn in the quest if it requires the item, but I don't know if the full behaviour is correct or not, or if problems may arise from this.
- If I take 2 of the same item, the trade window refreshes, but if I split the stack, then one item keeps being tradeable while the other loses the ability to do so. See image, they were part of the same stack.
-
If I split the stack, restack them and split it again, the timer of one of the items is refreshed to 2 hours.
-
Also, the quest items aren't tradeable at all. I looked around and found a comment:
You are not able to accept the quest off of the Onyxia head if you have already completed it, we tried, good thing the head was still tradable for 2 hours. (source)
maybe they should become untradable after someone accepts the quest?
Also, may be interesting to note here:
August 7, 2020 WoW Classic
The Qiraji Hilt, Qiraji Drape, and Qiraji Ring quest items are now tradeable and will no longer stack.
Developers’ note: In order to enable the ability to trade to eligible looters within 2 hours of looting the Qiraji hilts, drapes, and rings, we’ve removed the ability to stack multiples of them. Additionally, only one of these items are needed for the non-repeatable quest so there’s no reason for them to stack. (source)
https://www.reddit.com/r/classicwow/comments/i5bran/aq_20_item_tokens_are_not_tradeable/ implies that ZG had the same problem earlier, but it was fixed.
@Gultask Interesting thing. So it looks like these items should not stack and then this PR is not needed :P
Wrong, they shouldn`t be tradeable, the stack removal thing was... wow classic?
From what I researched just now it seems that trading raid items was introduced in wotlk and later in Classic for QoL changes...
At BlizzCon 2018, we talked about how we plan to keep loot trading in World of Warcraft Classic. We added loot trading in Wrath of the Lich King to solve a common problem: a player could accidentally loot an item meant for another player or give it to the wrong person using Master Loot. They would then have to contact Blizzard to get the item moved to the intended recipient, which might take days. We wanted to keep loot trading in WoW Classic because the end result is the same – the correct person gets the item – and it’ll save everyone time. (source)
maybe a SQL query for CC/progressionmod to make them not stack would be better
Trading items was introduced in Wotlk, but it includes itemsfrom previous expansion raids.
Sorry, I got lost after the last researches. I'm not sure how it should work, but it seems to me stackable items shouldnt be tradeable because of the overlaping rules mentioned earlier, so something seems off
Okay. I think the best solution here is just to set these items as single-stack
Sorry for the delay. I tested this before and tried again now. It works, and if you have a full inventory before the query is applied, the previously stacked items disappear from your bags and are returned through Customer Support mail automatically.
But it probably isn't blizzlike in WotLK retail. In Shadowlands the items still stack. But this is a solution that works and even Blizzard themselves decided they shouldn't stack for Classic.
I am not sure this is the correct approach. I think its better to put it in mod-progression instead.
For me it's best option and should be included in core, not in module. But not my decision though.
AQ40 also has stackable loot that I didn't know about. Here are the IDs:
20926, 20927, 20928, 20929, 20930, 20931, 20932, 20933
--
UPDATE `item_template` SET `stackable`=1 WHERE `entry` IN (19716,19717,19718,19719,19720,19721,19722,19723,19724,20888,20884,20889,20885,20890,20886,20926,20927,20928,20929,20930,20931,20932,20933);
https://github.com/azerothcore/azerothcore-wotlk/pull/12415#issuecomment-1200102944
Long story short this will make it the same as WoW Classic but will be different from 'true' Vanilla since these items are still stackable even today in SL. Not blizz-like but it's a very nice QoL improvement.
Reading over the comments i am a bit conflicted with the disposition of this pr. Do we merge, or not merge? last comment was sept 2022 stating its a nice QOL. Do we move forward with this or not?
I believe no as it is a custom implementation, even if QoL
Would it be impossible to implement as a config option? EDIT: Might be completely misunderstanding the PR, disregard me. 🤷
I believe no as it is a custom implementation, even if QoL
Would it be impossible to implement as a config option? EDIT: Might be completely misunderstanding the PR, disregard me. 🤷
Not a possibility. SQLs can’t be set as configs
Considering this is a Classic change the PR should be closed as we shouldn’t merge classic content into the main branch. Perhaps this could be moved to the progression module instead.
I believe no as it is a custom implementation, even if QoL
Would it be impossible to implement as a config option? EDIT: Might be completely misunderstanding the PR, disregard me. 🤷
The closest i ever gotten to sql change via config was matching with client version to database auth version. Long story. Was completely useless. Had to have the sql string mstch the conf. Really was not worth the outcome or the effort to even get it like that.
As most people are more in favour of a transfer to mod-progress, I close this PR in favour of : https://github.com/azerothcore/mod-progression-system/pull/316/files
