SignShop v4

SignShop v4

Bukkit Plugins
992k Downloads

Buy signs for written books break on plugin reload

RoyCurtis opened this issue · 18 comments

commented

Summary

If a SignShop is created to sell written books with NBT data (e.g. book author, title, contents), it is able to buy those books and determine it is in stock in a chest until the plugin is reloaded. After which, it incorrectly determines the books are out of stock and appears to lose the extra information associated with what it is buying.

(Note: this bug may also extend to sell signs, or other items that may hold data such as enchanted tools)

Details

Reproduction

  1. Create a chest and a sign with [Buy] on the first line
  2. Obtain a book and quill, open it, write random text inside, sign it with a random title
  3. Use an Essentials command such as /more to obtain a stack of the signed books
  4. Add a single copy of the book into the chest and link to the buy sign with redstone. Observe it correctly identifies the title and author.
  5. Insert the entire stack of books into the chest and punch the sign. Observe it correctly identifies the stock
  6. Execute /signshop reload or restart the server
  7. Punch the sign. Observe it now reports the shop as empty (Unusually, this contradicts the reporting player's screenshot where the author and title of the books had gone missing. In my reproduction, the data is still in the message but the stock is wrong).
commented

We are having a similar issue but on a different scale with a totally un-supported server/client configuration, I will try to explain and you can tell me if this is related:

Server: Cauldron 1.7.10
Mod: Thaumcraft
Item: Crystal Essence
For Discussion purposes: ItemID: 5038:0 (unique to our server)
What I know about this issue: If you make a sign shop with these types of thaumcraft items everything works fine until server is stopped / started. From what I understand these items and other items within a Cauldron environment stores a bunch of information in NBT that apparently isn't surviving the server restarts.

Surprisingly your latest build for 1.8.1 works on our Cauldron server without any issue. The item configuration stored within our config files is as below:

-1205/66/-1000/asgard:
owner: c1e27065-6684-4a76-ae1d-d23d9f2fcff5
activatables: []
containables:
- -1203/74/-992/asgard
shopworld: asgard
sign: -1205/66/-1000/asgard
items:
- 32&5038&0&0&&&39&

I realize that we are using this in a totally un-supported configuration but any thoughts you could share would be much appreciated.

Here's hoping you folks migrate to the Sponge server which just went Beta last night.

commented

That's just an issue with SignShop not supporting NBT items. Not related
to this issue.

On Fri, Jan 1, 2016 at 12:07 PM, Dockter [email protected] wrote:

We are having a similar issue but on a different scale with a totally
un-supported server/client configuration, I will try to explain and you can
tell me if this is related:

Server: Cauldron 1.7.10
Mod: Thaumcraft
Item: Crystal Essence
For Discussion purposes: ItemID: 5038:0 (unique to our server)
What I know about this issue: If you make a sign shop with these types of
thaumcraft items everything works fine until server is stopped / started.
From what I understand these items and other items within a Cauldron
environment stores a bunch of information in NBT that apparently isn't
surviving the server restarts.

Surprisingly your latest build for 1.8.1 works on our Cauldron server
without any issue. The item configuration stored within our config files is
as below:

-1205/66/-1000/asgard:
owner: c1e27065-6684-4a76-ae1d-d23d9f2fcff5
activatables: []
containables:

  • -1203/74/-992/asgard
    shopworld: asgard
    sign: -1205/66/-1000/asgard
    items:
  • 32&5038&0&0&&&39&

I realize that we are using this in a totally un-supported configuration
but any thoughts you could share would be much appreciated.

Here's hoping you folks migrate to the Sponge server which just went Beta
last night.

—
Reply to this email directly or view it on GitHub
#21 (comment).

commented

Sorry for the very slow response to this issue,

I think this issue is properly resolved once and for all. I made the following PRs to Spigot in response to this issue, which were successfully merged:

  • SPIGOT-783: BookMeta API methods for getting/setting book generation data
    • This adds API for plugins to get and set the generation data of written books. This means that plugins (e.g. SignShop) no longer need to use unreliable reflection to handle this data.
  • Improvements to BookMeta API
    • An important fix in this PR is CraftBukkit's comparison of book metadata. It had not been taking book generation into account. This affected hasItems() in VirtualInventory.java, which was failing to differentiate between books of differing generations.

This means that for this issue to be resolved, players must use Spigot 1.9.4 or higher. However, I also made a pull-request for SignShop to use the new book generation API instead of reflection.

I've tested all of the above on PaperSpigot 1.10.2 b837. It appears that buying/selling written books no longer have any issues on SignShop

commented

Just to keep you up-to-date, we have a possible fix for this issue but it's pending some testing on a server that extensively uses written books. The reason it needs testing is because if the feature were to misbehave, it could damage existing books both in your inventory and in the chest. Local testing has resulted in no such thing so odds are, if there's a bug, it will only occur in very rare cases.

The reason this is taking so long is because it's not a simple bug in SignShop, it turned out to be an inconsistency in Minecraft/Spigot/Bukkit: https://hub.spigotmc.org/jira/browse/SPIGOT-734
And we're now basically creating a catch-all fix for that inconsistency.

commented

Thanks! This is good information to know regardless.

commented

I have confirmed the reproduction on this. Thanks for the detailed report. We'll look into it.

commented

also I found a bug, if you have more than one book then it only gives the first one

commented

This doesn't work for maps, same issue :O

commented

Ah, I didn't test with more than one book! Good catch. Can you describe the map issue as well? Does it just go blank on restarts or something?

commented

It just says the map isn't in my inventory.

commented

Works.

commented

I tested it in my server and our book came out with the xp, so the lore is getting carried over. We make our lore with plugins though and as long as the lore reads the same it doesn't bother us.

commented

Thanks again for testing pande 👍
The fix is available from our build server here.
It will allow you to use written books (with or without lore, custom names, enchantments) as you normally would.
In case of issues, just switch the EnableWrittenBookFix boolean in the config to false and let us know about the problem.

commented

I have only two test cases on my server, but they work! However, books in SignShop inventories will lose their "generation" metadata as a result of this, which may be a dealbreaker for some vanilla players.

I did some digging and found the Bukkit API has no methods for dealing with book generation data, so I filed a feature suggestion for this. If this gets implemented, I will file another bug here or may submit a PR.

Regardless, thank you very much for solving this issue!

commented

I'll make a note of the "generation" issue when we officially release this fix.

commented

@RoyCurtis just to let you know, I just built in some support for the generation flag with 89aa2bb. It supports a shop selling a copy of a book and will persist that flag all the way to the user's inventory. However, when a shop is buying from a user then it won't, due to limitations in Bukkit/Spigot, ensure the user is selling the right book. So if the shop were to only buy the original of a book, a user could still sell a copy (or copy of a copy) to the shop.
We're not planning on fixing that at this point though. We'll look into it as soon as a real use case comes along.
Thanks for creating the ticket btw 👍

commented

Only just updated today; works nicely! Thank you very much again, and np.