cf47c45e20
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. By: blablubbabc <lukas@wirsindwir.de> |
||
---|---|---|
.. | ||
nms-patches | ||
src | ||
.gitignore | ||
applyPatches.sh | ||
checkstyle.xml | ||
CONTRIBUTING.md | ||
LGPL.txt | ||
LICENCE.txt | ||
makePatches.sh | ||
pom.xml | ||
README.md |
CraftBukkit
An implementation of the Bukkit plugin API for Minecraft servers, currently maintained by SpigotMC.
Index
Bug Reporting
The development team is very open to both bug and feature requests / suggestions. You can submit these on the JIRA Issue Tracker.
Compilation
CraftBukkit is a Java program which uses Maven 3 for compilation. To compile fresh from Git, simply perform the following steps:
- Install Git using your preferred installation methods.
- Download and run BuildTools
Some IDEs such as NetBeans can perform these steps for you. Any Maven capable Java IDE can be used to develop with CraftBukkit, however the current team's personal preference is to use NetBeans.
Contributing
Contributions of all sorts are welcome. To manage community contributions, we use the pull request functionality of Stash. In to gain access to Stash and create a pull request, you will first need to perform the following steps:
- Create an account on JIRA.
- Fill in the SpigotMC CLA and wait up to 24 hours for your Stash account to be activated. Please ensure that your username and email addresses match.
- Log into Stash using your JIRA credentials.
Once you have performed these steps you can create a fork, push your code changes, and then submit it for review.
If you submit a PR involving both Bukkit and CraftBukkit, each PR should link the other.
Although the minimum requirement for compilation & usage is Java 8, we prefer all contributions to be written in Java 7 style code unless there is a compelling reason otherwise.
Code Requirements
- For the most part, CraftBukkit and Bukkit use the Sun/Oracle coding standards.
- No tabs; use 4 spaces instead.
- Empty lines should contain no spaces.
- No trailing whitespaces.
- No 80 character column limit, or 'weird' mid-statement newlines unless absolutely necessary.
- The 80 character column limit still applies to documentation.
- No one-line methods.
- All major additions should have documentation.
- Try to follow test driven development where available.
- All code should be free of magic values. If this is not possible, it should be marked with a TODO comment indicating it should be addressed in the future.
- If magic values are absolutely necessary for your change, what those values represent should be documented in the code as well as an explanation in the Pull Request description on why those values are necessary.
- No unnecessary code changes. Look through all your changes before you submit it.
- Do not attempt to fix multiple problems with a single patch or pull request.
- Do not submit your personal changes to configuration files.
- Avoid moving or renaming classes.
Bukkit/CraftBukkit employs JUnit 4 for testing. Pull Requests(PR) should attempt to integrate within that framework as appropriate. Bukkit is a large project and what seems simple to a PR author at the time of writing may easily be overlooked by other authors and updates. Including unit tests with your PR will help to ensure the PR can be easily maintained over time and encourage the Spigot team to pull the PR.
- There needs to be a new line at the end of every file.
- Imports should be organised in a logical manner.
- Do not group packages
- All new imports should be within existing CraftBukkit comments if any are present. If not, make them.
- Absolutely no wildcard imports outside of tests.
- If you only use an import once, don't import it. Use the fully qualified name.
Any questions about these requirements can be asked in #spigot-dev in IRC.
Applying Patches
Any time new patches are created and/or updated in CraftBukkit, you need to apply them to your development environment.
- Pull changes from CraftBukkit repo.
- Run the
applyPatches.sh
script in the CraftBukkit directory.- This script requires a decompile directory from BuildTools. (e.g. /work/decompile-XXXXXX)
- Your development environment is now up to date with the latest CraftBukkit patches!
Making Changes to Minecraft
Importing new NMS classes to CraftBukkit is actually very simple.
- Find the
work/decompile-XXXXXX
folder in your BuildTools folder. - Find the class you want to add in the
net/minecraft/server
folder and copy it. - Copy the selected file to the
src/main/java/net/minecraft/server
folder in CraftBukkit. - Implement changes.
- Run
makePatches.sh
to create a new patch for the new class.- Be sure that Git recognizes the new file. This might require manually adding the file.
- Commit new patch.
Done! You have added a new patch for CraftBukkit!
Making Changes to NMS Classes
Bukkit/CB employs a Minimal Diff policy to help guide when changes should be changed to Minecraft and what those changes should be.
This is to ensure that any changes have the smallest impact possible on the update process whenever a new Minecraft version is released.
As well as the Minimal Diff Policy, every change made to a Minecraft class must be marked with the appropriate CraftBukkit comment.
At no point should you rename an existing/obfuscated field or method. All references to existing/obfusacted fields/methods should be marked with the // PAIL rename
comment.
Mapping of new fields/methods are done when there are enough changes to warrant the work. (Any questions can be asked in #spigot-dev in IRC)
Key Points:
- All additions to patches must be accompanied by an appropriate comment.
- To avoid large patches, avoid adding methods where possible. We prefer making fields public over adding methods in patches.
- If you have to add a method to a patch, please explain why in the Pull Request description.
- Never rename an existing field or method. If you want something renamed, include a
PAIL rename
comment - Converting a method/class from one access level to another (i.e. private to public) is fine as long as that method is not overridden in subclasses.
- If a method is overridden in its' subclasses, create a new method that calls that method instead (along with appropriate CraftBukkit comments).
- If you can use a field to accomplish something, use that over creating a new method.
Minimal Diff Policy
The Minimal Diff Policy is key to any changes made within Minecraft classes. When people think of the phrase "minimal diffs", they often take it to the extreme - they go completely out of their way to abstract the changes they are trying to make away from editing Minecraft's classes as much as possible. However, this is not what is meant by "minimal diffs". Instead, when trying to understand this policy, it helps to keep in mind its goal: to reduce the impact of changes we make to Minecraft's internals have on our update process.
To put it simply, the Minimal Diffs Policy simply means to make the smallest change in a Minecraft class possible without duplicating logic.
Here are a few tips you should keep in mind, or common areas you should focus on:
- Try to avoid duplicating logic or code when making changes.
- Try to keep your changes easily discernible - don't nest or group several unrelated changes together.
- All changes must be surrounded by CraftBukkit comments.
- If you only use an import once within a class, don't import it and use a fully qualified name instead.
- Try to employ "short-circuiting" of logic if at all possible. This means you should force a conditional to be the value needed to side step the code block to achieve your desired effect.
For example, to short circuit this:
if (!this.world.isClientSide && !this.isDead && (d0*d0) + d1 + (d2*d2) > 0.0D) {
this.die();
this.h();
}
You would do this:
if (false && !this.world.isClientSide && !this.isDead && (d0*d0) + d1 + (d2*d2) > 0.0D) {
this.die();
this.h();
}
- When adding a validation check, this applies everywhere not just in Minecraft classes, see if the Validate package has a better, more concise method you can use instead.
- The Preconditions package works just as well. Either are acceptable; though these are, by no means, the only accepted validation strategies.
For example, you should use:
Validate.notNull(sender, "Sender cannot be null");
Instead of:
if (sender == null) {
throw new IllegalArgumentException("Sender cannot be null");
}
- When the change you are trying to make involves removing code, or delegating it somewhere else, instead of removing it, you should comment it out.
For example:
// CraftBukkit start - special case dropping so we can get info from the tile entity
public void dropNaturally(World world, int i, int j, int k, int l, float f, int i1) {
if (world.random.nextFloat() < f) {
ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);
if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
itemstack.setTag(new NBTTagCompound());
itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
}
this.b(world, i, j, k, itemstack);
}
}
// CraftBukkit end
public void remove(World world, int i, int j, int k, int l, int i1) {
if (!world.isStatic) {
/* CraftBukkit start - drop item in code above, not here
if ((i1 & 8) == 0) {
ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);
if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
itemstack.setTag(new NBTTagCompound());
itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
}
this.b(world, i, j, k, itemstack);
}
// CraftBukkit end */
super.remove(world, i, j, k, l, i1);
}
}
CraftBukkit Comments
Changes to a Minecraft class should be clearly marked using CraftBukkit comments.
- All CraftBukkit comments should be capitalised appropriately. (i.e. CraftBukkit, not CB/craftBukkit, etc.)
- If the change only affects one line of code, use an end of line CraftBukkit comment
Examples:
If the change is obvious, then you need a simple end of line comment.
if (true || minecraftserver.getAllowNether()) { // CraftBukkit
Every reference to an obfuscated field/method in NMS should be marked with:
// PAIL rename newName
All PAIL rename comments must include a new name.
If, however, the change is something important to note or difficult to discern, you should include a reason at the end of the comment
public int fireTicks; // PAIL private -> public
Changing access levels must include a PAIL comment indicating the previous access level and the new access level.
If adding the CraftBukkit comment negatively affects the readability of the code, then you should place the comment on a new line above the change you made.
// CraftBukkit
if (!isEffect && !world.isStatic && world.difficulty >= 2 && world.areChunksLoaded(MathHelper.floor(d0), MathHelper.floor(d1), MathHelper.floor(d2), 10)) {
- If the change affects more than one line, you should use a multi-line CraftBukkit comment.
Example:
The majority of the time, multi-line changes should be accompanied by a reason since they're usually much more complicated than a single line change. If the change is something important to note or difficult to discern, you should include a reason at the end of line CraftBukkit comment.
// CraftBukkit start - special case dropping so we can get info from the tile entity
public void dropNaturally(World world, int i, int j, int k, int l, float f, int i1) {
if (world.random.nextFloat() < f) {
ItemStack itemstack = new ItemStack(Item.SKULL, 1, this.getDropData(world, i, j, k));
TileEntitySkull tileentityskull = (TileEntitySkull) world.getTileEntity(i, j, k);
if (tileentityskull.getSkullType() == 3 && tileentityskull.getExtraType() != null && tileentityskull.getExtraType().length() > 0) {
itemstack.setTag(new NBTTagCompound());
itemstack.getTag().setString("SkullOwner", tileentityskull.getExtraType());
}
this.b(world, i, j, k, itemstack);
}
}
// CraftBukkit end
Otherwise, if the change is obvious, such as firing an event, then you can simply use a multi-line comment.
// CraftBukkit start
BlockIgniteEvent event = new BlockIgniteEvent(this.cworld.getBlockAt(i, j, k), BlockIgniteEvent.IgniteCause.LIGHTNING, null);
world.getServer().getPluginManager().callEvent(event);
if (!event.isCancelled()) {
world.setTypeIdUpdate(i, j, k, Block.FIRE);
}
// CraftBukkit end
- All CraftBukkit comments should be on the same indentation level the code block it is in.
Imports in Minecraft Classes
- Do not remove unused imports if they are not marked by CraftBukkit comments.
Creating Pull Requests
To learn what Spigot expects of a Pull Request please view the Contributing guidelines