Rocologo / BagOfGold

Economy plugin for Minecraft Servers
13 stars 6 forks source link

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

Closed AdrienFd closed 1 year ago

AdrienFd commented 4 years ago

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.

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 :

.

.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 :

.

.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 :

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:

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

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 !

Rocologo commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@AdrienFd are your there?

blablubbabc commented 4 years ago

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

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 commented 1 year ago

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.

blablubbabc commented 1 year ago

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 (https://github.com/Rocologo/BagOfGold/commit/3487ea21c2f7015365675143c58df3a6722c226e). 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 commented 1 year ago

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

Rocologo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I briefly looked over the changes:

Rocologo commented 1 year ago

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 commented 1 year ago

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

Rocologo commented 1 year ago

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

blablubbabc commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.