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

[Issue/FeatureRequest] Can't use the physical money to trade with managed villagers - plugin shopkeepers #92

Closed
AdrienFd opened this issue Apr 19, 2020 · 24 comments
Assignees

Comments

@AdrienFd
Copy link

AdrienFd commented Apr 19, 2020

To be clear a big part of this request is enhancement, there is just on part I don't know if it's a bug or a wanted behavior because other than that the plugin works really well !

.

.1. Introduction :
Hi I am starting to develop a semi-virtual economy on my server, running spigot 1.15.20.

This server relies on 2 major plugins Towny and Shopkeepers.

  • Towny is used for the cities and is using virutal economy through town banks.
  • Shopkeepers is used to create villager shop to sell, buy, trade but requires an item based economy which is the reason why I choose it.

It also has minors plugin like timeismoney and jobs who will come later.

At this point my server had only a virtual economy managed by essential and use vault to hook with towny and the minors plugin.

So to make the link between these 2 kind of economy I need a plugin that managed both virtual and physical economy, or an add-on plugin over essential that thus support vault.

This plugin must have functionnalities to deposit/withdraw between player physical/virtual money via command or sign.

Before I steeped accross your plugin I found and tried 2 plugins :

  • gringots but it's out of the way it's not up to date
  • the piggy bank 1.2.7 which kinda work, the witdraw/deposit par was working with shopkeepers I used the default coin system which is a basic dead bush named with the coin value and 2 lores saying always the same thing, however it didn't not hook with towny.

.

.2. Steps to reproduce/problems :
So that led to your plugin which performs really well with towny, now I am trying to use your plugin with shopkeepers. And here the problems begins :

  • If I use the types SKULL or ITEM I can't do a trade with the shopkeeper, when I try to drag and drop the skull_bagofgold or gold_nuggets it looks like your plugin doesn't accept it the item goes back to the inventory. (To be sure it wasn't Shopkeepers the issue i tried to do it with normal villager, by just simply drag and drop the item in villager the trade input but it doesn't work too).
  • On the other end when i use type GRINGOTS_STYLE it work perfectly with shopkeepers, but i don't like it the way it's developped (cause it use normal item so if i use emerald or gold it's subject to duplication and fortune enchantement).

.

.3. Explanation of the request :
I tried to understand how Shopkeepers work when they test the expected trade input with what the player input and here are my result :

  • If expected input is a normal item (not renamed and no lores) then it will accept the same item even it's renamed and/or it has as lores, any other item will be rejected.
  • If expected input is a renamed item then an item with the same exact name is expected. If color are used in the name of the item then the item name of the one provided by the player should have the same color (however if a player tries to imitate the color using color code and an anvil/nametag this will not work cause the color code will not be translated into the color and will be left as text)
  • If expected input is a **renamed item with lores ** then an item with the exact same name and lores is expected. For the color that the same behavior than described before.

So this gave me an idea for options to add in your plugin (this idea aim to reproduce the behavior used by piggy bank that's work perfectly with shopkeepers), I see two way of implementing if you wouldn't mind the trouble :) :
A) First way that comes to mind to solve the problem with shopkeepers is to add these 2 options in the config:

  • one to disable random uuid on skulls (and items)
  • (heads being stackable like any other item if the named and lores are the same) another option to use 2,3 or more skulls (or items) size like 1,10,100 using specific names or lores for each size type rather than specifying the max a skull can contain.

B - If this isn't possible then I have think about that :

  • adding the possibility to give specific custom name and lores with color for each gringotts like item (without adding them random lores).
  • your plugin kinda already manage the second part of my idea that is to allow multiple money value store in the name and lores of the item like a deadbush for example.
  • finally an option to disable the ability to place on the ground gringotts like item that are named and that have lores.

I tried to be as clear as possible in my request.
If you could implement one of this idea or something like that it would be really nice from you, thank you !

@AdrienFd AdrienFd changed the title Issue with the plugin shopkeepers can't use the physical money to trade [Issue/Feature Request] with the plugin shopkeepers can't use the physical money to trade Apr 19, 2020
@AdrienFd AdrienFd changed the title [Issue/Feature Request] with the plugin shopkeepers can't use the physical money to trade [Issue/FeatureRequest] Can't use the physical money to trade with managed villagers - plugin shopkeepers Apr 19, 2020
@Rocologo
Copy link
Owner

You will have to ask the developer of Shopkeepers to support an itembased economy like BagOfGold, its not something I can do.

If he wants to add support for BagOfGold I will happily help him finding the methods he need to do so.

@blablubbabc
Copy link

I just happend to come across this ticket and had a very brief look into the plugin.
It seems like the BagOfGold plugin adds a random uuid to every bag of gold item. Not sure what these uuids are used for, but as outlined by the OP, this obviously breaks any kind of trading for these items if the trade compares items by their data (since the required and offered items never match if they have individual uuids).
This not only applies to Shopkeepers, but regular minecraft villagers as well. A solution that would enable trading these bag of gold items in regular minecraft villagers would very likely allow them to be tradable in shops created by the Shopkeepers plugin as well.
There are currently no plans from my side to add any plugin-specific special cases to the Shopkeepers plugin.

@Rocologo
Copy link
Owner

Rocologo commented May 7, 2020

@blablubbabc To be honest I cant remember why I had to add the random uuid. I think it was added when I was trying to make sure that the players didn't try to change the Lores and this way changing the value. I will investigate if it is possible remove the random uuid to simplify the rewards.

But even if this is possible, it is not possible to handle the items as all other normal items. The reason for this is that the Bags ARE the money, so if a player is trading bags it need to affect the players balance.

For all normal operations drop, pickup, place,break I have events to catch and make the updates to the balance and much more. But if is another plugin like shopkeeper which use the Bags Im not able to catch any event.

One thing alot of people forget when they talk about the bags and shops they do forget that you cant not sell a bag, because the bag are the money. You do not sell money for money. That does not make sense.

@Rocologo
Copy link
Owner

Rocologo commented May 8, 2020

@blablubbabc I have studied your plugin as well now and seen a couple of videos.

I might be able to remove the random uuid, but I dont see how that would help your plugin?

In the default configuration you support to items (emerald and emarald_block) and they have the values (1,9) but BagOfGold uses a Player_Head and only a playerhead. But one playerhead have have different values. I can handle a player head with a value of 1 and a another playerhead with a value of 9, and if they are merged the player will have only one player_head with a value of 10. (Not two player heads)

I dont think integration to BagOfGold will not be possible unless you choose to handle the BagOfGold items as a special item not like a ordinary emarald or iron_ingot.

I understand why dont want to add special cases (its a big work), but if you change you mind, I would help you finding the methods you would need to adjust the player balance, getting the correct BagOfGold item with the value you want and so on.

@blablubbabc
Copy link

blablubbabc commented May 8, 2020

so if a player is trading bags it need to affect the players balance.

So this plugin is live updating the player's economy balance for the bag of gold items in his inventory? This is indeed tricky to get working in general with all the plugins out there which might perform inventory modifications at their will.. without the BagOfGold plugin noticing.
So far I thought that the BagOfGold plugin would only convert between the virtual currency to bag of gold items back and forth under specific conditions (eg. the player using a command to withdraw or deposit money, or interacting with the item in some way or similar).
But if you are live updating player balances, you certainly must already detect a player moving a bag of gold item from his inventory to some other inventory (eg. a chest or similar)? If that is the case, you could do the same when the player inserts the items into the villager trading menu (treating it like any other external inventory).
But reacting to any bag of gold items being added to the player's inventory might be tricky, since Shopkeepers manually re-implements the Minecraft trading logic (in order to implement restrictions regarding when the trade can take place). So the Shopkeepers plugin manually adds the trade's result items into the player's cursor and inventory. The only general solution for the BagOfGold items to detect this and other plugin's inventory changes would be to periodically scan the player's inventory..

Edit: Actually, if you only scan the player's inventory when the Vault economy is asked for the player's current balance then everything would work fine. You plugin would not need to handle inventory changes on the fly then. I believe this is how Gringotts works as well.

I might be able to remove the random uuid, but I dont see how that would help your plugin?

Shopkeepers is not restricted to the configured currency items (emerald and emerald block by default), but also allows trading of arbitrary items. As I see it the trading would handle these BagOfGold items like regular items. For example, the shop owner could setup a trade for a BagOfGold item with value 10 (represented inside the item's lore) and then either receive or give away some other arbitrary item in exchange for that BagOfGold item. The Shopkeepers plugin does not actually need to know about the 'value' of that BagOfGold item. All it cares is that the item provided by a buyer matchers the item required by the trade. If the trade takes place, the buyer either receives a new copy of the BagOfGold item (with all its internal data copied, including the lore marking it as 'value of 10'), or if the buyer is giving away a BagOfGold item he loses his item and a copy of it gets added to the shop owner's shop chest where the shop owner can later pick it up.

@Rocologo
Copy link
Owner

Rocologo commented May 8, 2020

So this plugin is live updating the player's economy balance for the bag of gold items in his inventory?

Yes it does this live and it handle ofllinePlayers as well. And economy bank functions.

It IS pretty tricky and yes I am handling alot of events including inventory events for ordinary inventories. And this is exactly why most Inventory-Shops like your is not working with BagOfGold items.

So far I thought that the BagOfGold plugin would only convert between the virtual currency.

No BagOfGold is working like any virtual currency and it support both Vault and The NewEConomy plugins. The only difference it that the amount of BagOfGold in the inventory is equal to balance in the virtual economy
f.ex. i you use Essentials commands /bal you will get the amount of BagOfGold in the Player Inventory of if you use /Eco give {playername} {amount} the player will get the bags added to his inventory and the virtual balance will be updated at the same time.

Shopkeepers is not restricted to the configured currency items

Okay I will make a new branch and see if it possible to remove the random UUID and then we have to see. I think I made this long ago trying to detect if players was trying to cheat by editing the lores, but this is handled differently now, but calculation a hash value hidden in the last Lore. Today BagOfGold supports more simple shops like ChestShop or sign shops and even the inventory shop called Shop, but I would very much like to support your plugin as well. It is not first time I got this request.

@blablubbabc
Copy link

Also see my edit regarding how live updates of the economy balance could be avoided:

Edit: Actually, if you only scan the player's inventory when the Vault economy is asked for the player's current balance then everything would work fine. You plugin would not need to handle inventory changes on the fly then. I believe this is how Gringotts works as well.

Not sure if you would be up for this, or if your plugin already works this way. But I believe this would be a lot easier to implement and maintain compared to detecting and reacting to all kinds of inventory changes (which is impossible to do efficiently (without periodic scanning) if other plugins are involved..)

@blablubbabc
Copy link

blablubbabc commented May 8, 2020

but this is handled differently now, but calculation a hash value hidden in the last Lore

I am not completely sure how this would prevent item duplications, but if that 'hash value' is unique for the specific item instance then this would effectively be just like the random uuid preventing trading (since two different items would not match if they have different hash values).

@Rocologo
Copy link
Owner

Rocologo commented May 8, 2020

Also see my edit regarding how live updates of the economy balance could be avoided:

Unfortunately this is not that simple and Gringotts is alot easier to handle because Gringotts are 100% normal items and therefor following normal rules for the itemds. where the BagOfGold following other rules. Ex Mergeing two itemstacks (each 1 amount) will result in only 1 itemstack

The first versions (v1.x) of BagOfGold did not keep track of the virtual balance, but that gave me alot of problems. So for V2.x + I decided to keep track of the the virtual balance at the same time.

I am not completely sure how this would prevent item duplications

Correct the random uuid, does not prevent duplications. If the player are able to duplicate a bag, he will double the value, but I try to keep track on that by checking the events. F.ex. I check if the player is in creative mode, where duplication is possible.

For a start I will try to remove the random uuid and then lets see if what happens.

@Rocologo
Copy link
Owner

@AdrienFd @blablubbabc I have removed the random number from the BagOgGold items and (MobHunting rewards)

@AdrienFd could I ask you to see if it is possible to use the items now?
https://fractal.lindegaard.one:8181/job/BagOfGold/415/

@Rocologo
Copy link
Owner

@AdrienFd are your there?

@blablubbabc
Copy link

blablubbabc commented May 18, 2020

I just tried to quickly test this and have run into a few issues:

  • When I give myself money via essentials' pay command this does seem to get translated into bagofgold items, however it does not correctly merge with the bagofgold items that I already have: After giving myself 1000$ twice I end up with only a single bagofgold item of value 1000. However, if I use BagOfGold's commands for giving myself money the merging works fine.
  • I am sometimes/mostly not able to insert this bag of gold item into the trading slots of a minecraft villager. It behaves as if something is cancelling the corresponding InventoryClickEvent. Turning the debug mode on inside the config prints a message saying that 'blablubbabc is not allowed to use BagOfGold item in a CRAFTING slot'. Inserting via dragging works. However, the result item of the trade still doesn't show up and I am also not able to pickup the bagofgold item again.

Regarding that auto-merging of bagofgold items: I previously assumed that players could hold and split up their gold into separate bag of gold items. (I never used your plugin before so correct me if there is a way for players to split up a bagofgold item into multiple items of lower values, that can be moved around separetely)

I previously thought this plugin would be similar to Gringotts, with the difference being that it uses the item's displayname/lore to store and represent the value of the currency item, rather than using the item's stack size (and therefore not being limited to Minecraft's 64 stack size limit and only discrete values). However, not being able to split up the bagofgold items and moving them around separately makes it rather clumsy to use this item in item based shops (like Minecraft's villager trading interface).

With further changes it might maybe be possible for players to transfer 10 gold from their virtual bank balance into an item and then use that for trading. However, right now these bagofgold items seem to behave more like an item-based visualization of the player's vault-based virtual currency balance.

For the above reasons I might agree on your earlier statement that it might not make much sense to support trading with bagofgold items in villager trades.. unless you someday decide to change how the plugin works in that it becomes possible to use these bagofgold items more like actual minecraft items.

(One idea could be to allow players to right-click the bagofgold item inside their inventory and by that pick up a fixed amount of gold each time from the clicked bagofgold item into a new bagofgolditem on their cursor. Shift-clicking could adjust the amount that is being transfered between the items. The resulting bag of gold items could then be moved around independently. When the player left clicks a bagofgold item while holding another bagofgold item on the cursor, the items could merge. Basically mimicking some of Minecraft's item pickup, split, move and merging actions)

Supporting this in Shopkeepers would be close to supporting vault-based currencies.. which there are currently no plans for doing that (Shopkeepers is a purely item-based shop plugin that tries to stick to Minecraft's vanilla trading as closely as possible).

@Rocologo
Copy link
Owner

I have actually been working on this and V4.5.0 has Shopkeeper BETA features. This means that you must enable the Shopkeeper integration in the config.yml and the you are able to SELL items and get BagOfGold. Buying items for BagOfGold does not work yet.

@Rocologo Rocologo self-assigned this Nov 24, 2022
@blablubbabc
Copy link

blablubbabc commented Nov 24, 2022

I have actually been working on this and V4.5.0 has Shopkeeper BETA features. This means that you must enable the Shopkeeper integration in the config.yml and the you are able to SELL items and get BagOfGold. Buying items for BagOfGold does not work yet.

I very briefly looked into those changes (3487ea2). But I am not yet completely sure how this will resolve the issue (maybe this explains why buying BagOfGold items does not yet work in your tests).

The code appears to remove some special data from the lore of items inside the packets that are sent to players. So on the client, items that might be different on the server might then appear to match. However, on the server these items will still be different.

Even if you were to dynamically modify the items on the server as well while they are inside the merchant inventory, this will likely not work either: The Shopkeepers plugin has some additional checks in place to only handle trades if the traded items match the items that are configured inside the shopkeeper that is being traded with. So if the items inside the shopkeeper have this random hidden lore entries, any items used in trades are required to match that lore.
This is required in order to account for other plugins that might (sometimes accidentally, sometimes intentionally) modify the items inside the merchant inventory. The shopkeepers plugin will intentionally abort and not handle any trades if it detects mismatching items.

Edit: Maybe an option could be for your extension to dynamically modify these shopkeeper trading recipes and remove any special lore/item data there already. Not sure.

I can't comment on your solution yet, but at least from my past experience, plugins dynamically modifying items inside the merchant inventory (directly, or via packet listeners) most of the time resulted in weird glitches and hard to debug issues (e.g. because items appear to be the same on the client, while they are actually different on the server; or because the items on the server got modified as well and then no longer matched what the shopkeeper's configuration).

@Rocologo
Copy link
Owner

I didnt say i was easy :-) but I have not given up. I would really like to make this work.

@Rocologo
Copy link
Owner

In release BagOfGold 4.5.1 + CustomItemsLib you can both buy and sell BagOfGold. Its still BETA and there are still errors but Im a lot closer now :-)

I found a serious bug which has been there for years, but its fixed now. I strongly recommend to upgrade to 4.5.1 no matter if you want to test the Shopkeeper integration or not.

@Rocologo
Copy link
Owner

BagOfGold 4.5.1 + CustomItemsLib 1.0.4-SNAPSHOT

https://jenkins.lindegaard.one/job/CustomItemsLib/

Now seems to bee working, but I need some server owners to help me with the testing.

I have tested on these combinations
image

But I have never played much with the Merchant Inventory, so there might be some actions I dont know.

If you find any bugs, please explain in detail how I can reproduce the bug.

@blablubbabc
Copy link

I briefly looked over the changes:

  • What happens if the user does not have the balance you try to withdraw for the selected trading receipt? Currently you seem to insert the item anyway.
  • When the user switches between trades: Wouldn't you need to depost the current items from the inventory again and freshly withdraw items to match the newly selected trading recipet (in item data and amount)?
  • Is the user able to move the automatically withdrawn bag of gold item into their inventory? If yes, wouldn't this be a problem, since the player could then end up with bag of golds items with matching item data (which would apparently be an issue, otherwise the unique item data wouldn't be required).

@Rocologo
Copy link
Owner

  1. I have forgotten to check what happens if the player do not have the balance. :-( I will do that immediately
  2. I have tested what happens when the player chooses difference and I think I have handled that
  3. Yes the player is able to withdraw/deposit the bag into the recipe slots but this is tested too. When bag is "moved" from the PlayerInventory to the MerchantIventory it is actually removed from the PlayerBalance. So I think I handle this too.

Their might be some actions where the players can duplicate/loose money, actions which Im not aware of yet.

@Rocologo
Copy link
Owner

Its not until now that I realize that you are the developer of Shopkeepers :-)

@Rocologo
Copy link
Owner

  1. There is a problem if the player do not have enough money :-( If will find a fix immediately

@blablubbabc
Copy link

Its not until now that I realize that you are the developer of Shopkeepers :-)

Hence why I am very much interested in there being some more alternatives available when it comes to item-based economy plugins that can be used together with shopkeepers, ideally without requiring any kind of dedicated integration that needs to be maintained. Although I am not a user of this plugin, I would very much appreciate the BagOfGold plugin being another option I can recommend to users that are looking for an item-based economy plugin that integrates with Vault and can be used together with shopkeepers. :)

@Rocologo
Copy link
Owner

Its not until now that I realize that you are the developer of Shopkeepers :-)

Hence why I am very much interested in there being some more alternatives available when it comes to item-based economy plugins that can be used together with shopkeepers, ideally without requiring any kind of dedicated integration that needs to be maintained. Although I am not a user of this plugin, I would very much appreciate the BagOfGold plugin being another option I can recommend to users that are looking for an item-based economy plugin that integrates with Vault and can be used together with shopkeepers. :)

I only know my plugin and Gringotts which are item-based. I has been a pain to make this work with basically PLAYER_HEADs, because I have to follow ALL actions the players might do with the bag (PLAYER_HEAD)

@Rocologo
Copy link
Owner

The V4.5.2-SNAPSHOT on my jenkins server https://jenkins.lindegaard.one/job/BagOfGold/

Handle when the player do not have enough money to choose the recipe.

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

No branches or pull requests

3 participants