Issues resolved by this:
* SPIGOT-5063: Internal text representation of ItemStacks changes during ItemStack serialization. This issue was initially primarily concerned with the conversion between color text attributes to legacy color codes.
* SPIGOT-5304: Internal text representation of ItemStacks changes when opening the inventory (in creative mode). In particularly, this issue is also concerned with the conversion between plain text representations to non-plain ones.
* SPIGOT-5656: Internal text representation of ItemStacks changes during ItemStack serialization. This issue is particularly concerned with reordering of text attributes in the text's Json representation.
* SPIGOT-3206: Internal text representation of book pages changes during ItemStack serialization.
* SPIGOT-5350: Any non-plain text features are stripped from books during ItemStack serialization.
* SPIGOT-5980: Written books are marked as 'resolved' during ItemStack serialization and on various inventory interactions, even though they aren't, and thereby breaking any non-resolved page contents.
* SPIGOT-4672: Since item display names are serialized in their internal Json representation, any translatable components get properly persisted as well now.
---------
Minecraft uses text components to represent text. Internally Minecraft stores these components as Json formatted Strings and dynamically parses the text components from this Json representation whenever required.
In some cases Minecraft will create the text components and then convert them to Json itself for the internal storage. In other cases the Json representation is specified by users (eg. in Minecraft give commands, loot tables, mob equipment specified via Minecraft's summon commands, etc.).
There are many different ways in which the same text components can be represented in Json. When Minecraft compares objects which store this textual information, it takes the exact Json representation into account to determine whether the objects are considered equal. For example, ItemStacks will not match (and therefore not stack) if there is a difference in this internal Json representation for at least one if the item's text attributes (such as display name, lore, book pages, etc.). And when specifying nbt data in command selectors (eg. to only match entities/players which hold an item with specific name), the selector compares the raw Json representation as well.
As long as the Json representation is valid and can be parsed, Minecraft will not modify or normalize it.
However, under various circumstances Spigot converts this text information from the internal Json representation to text components (and in some cases even to plain text with legacy color codes) and then later tries to convert the text from these representations back to text components in the Json representation. Because this backwards conversion is in many cases not able to reproduce the original Json representation, the internal data of some affected Minecraft objects (ItemStacks, TileEntities, Entities, etc.) will in some cases get modified.
One especially notable situation in which this issue can come up is Bukkit's configuration serialization of ItemStacks: When a plugin serializes and later deserializes ItemStacks with display name, localized name, lore, or book pages of signed books, Spigot would convert these textual ItemStack attributes to plain text with legacy color codes and later try to convert those back to chat components in the Json representation. If the reconstructed Json representation does not match the original representation, the deserialized ItemStacks would no longer match nor stack with any original ItemStacks.
This case is particularly common if the original ItemStacks are created by users via some vanilla Minecraft mechanism (eg. Minecraft's give command, loot tables, mob equipment specified via Minecraft's summon command, etc.) and the used internal text representation for the created ItemStacks does not match the text representation produced by Spigot. This is also quite likely to be case, because the internal text representation produced by Spigot can sometimes be slightly verbose and, until recently, contained legacy color codes which cannot be used in Minecraft commands in-game.
However, this issue is not limited to items created by users, but affects items created by Minecraft itself as well.
Other cases in which Spigot itself (without any plugins involved) will convert between these text representations include dragging items around inside the inventory or opening the inventory while in creative mode. In these cases Spigot creates Bukkit representations of the affected items for use in Bukkit events and then, after the events have been handled, converts these Bukkit representations back to Minecraft items. See for example SPIGOT-5656 and SPIGOT-5304.
The idea of these changes is to avoid this back and forth conversion between the internal Json representation and the text component or plain text representations in various situations in which it is not actually required:
* CraftMetaItem stores the raw original Json representation for the display name, localized name, lore and pages of signed books now. As long as no plugin modifies these text attributes via the API, they can be reapplied in their original form to an ItemStack.
* The configuration serialization will serialize the original Json representation for these text attributes now so that it can also be restored during deserialization.
* However, in order to still be able to deserialize previously serialized items, and in order to allow users to specify text in the more simple plain representation in configuration files, we also still accept plain text during deserialization. Our approach is to check if the serialized text contains legacy color codes, in which case we convert it to chat components using our own converter and then to Json. Otherwise we try to parse it via Minecraft's Json parser. If the parsing fails due to the text not being valid Json, we interpret the text as plain text and convert it via our own converter as well.
* Various duplicated code has been removed from CraftMetaBookSigned and instead the base CraftMetaBook class allows sub classes to override the relevant aspects of how pages are parsed, serialized and deserialized.
* The BlockStates for command blocks and signs preemptively retrieved the custom name and sign line components, converted them to plain text and later converted them back to text components when applying the BlockState. We now only perform this conversion if a plugin has explicitly modified these texts.
Other changes:
* Minor: We also retrieve, convert and update a few other BlockState attributes directly from the underlying snapshot and only when requested by plugins now.
* SPIGOT-5980: Written books did not properly persist their 'resolved' attribute, resulting in unresolved book pages not getting resolved.
* There are methods to get and set the resolved value for books. However, these are not yet exposed in Bukkit.
* Minor fix: CraftMetaBook#isBookEmpty did not check some of the book attributes. This is probably a minor issue, but for consistency reasons there are checks for the missing attribute(s) now.
----
Covered cases
---
* By remembering the raw original String data, we can persist the exact text representation (eg. the ordering of elements within the Json text object (SPIGOT-5656), used style of escaping quotes (single quotes, escaped double quotes, etc.), use of plain texts (SPIGOT-5304), used boolean style, modern text component features such as translatable texts (SPIGOT-4672), etc.). All of these differences would otherwise cause the ItemStack to no longer be considered equal to its original.
* An empty String in the serialized config data results in no display name rather than an empty display name, like before. An item with explicitly empty display name (`{display: {Name: '""'}}`) is saved as `'""'` and can also be loaded from that representation again.
* Any plain texts, with or without color codes, which don't parse as Json (eg. `display-name: 'Bla'`) are still getting run through Spigot's text to components converter, like before.
* We can now also persist empty but explicitly present lore (`{display:{Lore:[]}}`). Previously this would get removed when the ItemMeta gets reapplied to the item. And ItemMeta#equals would return true for items with and without this empty lore data, even though Minecraft considers them to be different. For plugins using the API there should be no change: #hasLore still checks whether the lore is both present and not empty, and #getLore returns an empty list instead of null in this case (however, it previously already returned an empty list in this case). And setting the lore to an empty list via #setLore will still result in an item with no lore.
* Similarly, we can also persist explicitly specified but empty lists of book pages now.
----
Cases that are not covered (i.e. which may lead to changes in items), but were already not covered previously:
----
* NBT data for text that is not actually of type String.
* Empty or unexpected entries within the display compound.
* Variations in the NBT data representation in item features other than the above mentioned ones.
* Texts containing color codes. During deserialization these texts get interpreted as plain text and converted to a text component representation. This will break the serialization of any ItemStacks which actually use a text component representation with embedded color codes for some reason. Usually the likelihood for encountering such items in practice would probably be small. However, in the past (pre MC 1.16) Spigot would actually produce such items during ItemStack deserialization or when plugins created ItemStack via the Bukkit API. However, Spigot has changed the text representation it produces in MC 1.16, so any previously created and still existing items with this text representation are already problematic anyways now. See SPIGOT-5964. A fix for this linked issue (eg. the automatic conversion of these items) would probably resolve this deficit here as well.
* Spigot's String to text components converter produces quite verbose components since 1.16. See SPIGOT-5964 as well. However, this applies regardless of the changes of this PR.
* Book ItemStacks with more pages than 100 pages or oversized pages are truncated (like before) and may therefore change.
hange.
* Actually capture all the data of TileEntities. This is done by creating a copy of the TileEntity. The methods of BlockState which currently directly access the TileEntity reference will modify the data of that TileEntity-snapshot instead.
* With the call to BlockState.update, the captured TileEntity data gets applied to the current TileEntity in the world.
* Methods which trigger block specific actions will use the current TileEntity from the world.
* CraftBlockState does not hand out the wrapped or the snapshot TileEntity directly. Instead, it provides an applyTo method to copy the data to a given TileEntity and a method to directly get a copy of the TileEntity NBT data represented by the BlockState. CraftMetaBlockState was updated to make use of that.
* Added #getSnapshotInventory() to bukkit which allows modifiying the captured inventory snapshots of containers.
* Tried to clarify which methods only work if the BlockState is placed, which methods require the block in the world to still be of the same type (methods which trigger actions), and that .getInventory() directly modifies the inventory of the block in the world if the BlockState is placed and becomes invalid if the block type is changed.
Backwards compatibility
* If the BlockState acts as InventoryHolder, getInventory() will still return the inventory directly backed by the TileEntity in the world (like before), and not the snapshot inventory. This compromise should reduce the potential of these changes to break existing plugins, or craftbukkit's own use of BlockState.
* The snapshot's inventory can be accessed by a new method getSnapshotInventory()
* In case the BlockState is not placed (if it was retrieved from the MetaBlockState of an item), the getInventory() method will however return the snapshot inventory. So that when the BlockState gets applied back to the item, the inventory changes are properly included.
* With the changes to CraftMetaBlockState it is no longer required to call the update method before passing a modified BlockState to the CraftMetaBlockState. For backwards compatibility the update method will simply return true for a non-placed BlockState, without actually doing anything.
Impact on plugins
* Restoring blocks now actually works as expected, properly restoring the TileEntity data, reglardless if the block changed its type in the meantime.
* Plugins are now consistently required to call the update method in order to apply changes to blocks. Though, regarding the Javadoc they should have been required to do so anyways.
* New feature: Plugins can take and modify inventory snapshots.
* Breaking change: If a plugin gets the BlockState of a block in the world, modifies the inventory returned by .getInventory(), and then tries to use the same BlockState to apply the TileEntity data to an ItemStack block meta, the ItemStack will use the snapshot inventory, disregarding the changes made to the inventory returned by .getInventory(). This is the compromise of .getInventory() returning the inventory directly backed by the TileEntity in the world.
Other fixes related to BlockState:
* TileEntityContainer#getLocation() will run into a NPE if the TileEntity is non-placed (ex. when getting the BlockState from a CraftMetaBlockState).
* Beacon.getEntitiesInRange() would previously throw a NPE if called for a non-placed BlockState. It was changed to now require to be placed and use the current TileEntity in the world. If the TileEntity in the world is no longer a beacon, it will return an empty list.
* EndGateway now supports setting and getting the exit location even for non-placed EndGateways (inside BlockStateMeta) by using / returning a location with world being null.
When a block placement happens we currently update physics on the
attempted placement and update again if the placement is cancelled.
To correct the first one we simply set the block without applying
physics. To correct the second we have to add a new method to
BlockState that lets us update without applying physics and use
this method method when putting the block back.
- Removed the useless world field.
- Made it so changes to a CraftSign (which is a Block*State*) no longer reflect into the world without calling sign.update().
