Paper
Paper copied to clipboard
PaperMC
Fix sendBlockDamage 0 progress
Method Player#sendBlockDamage work wrongly. The javadoc says that if the progress is 0 then it means that the block has no destruction, while if you pass 0 then the player will see the destruction. To reset the destruction, the player needs to send any number other than 0-9, this is written on wiki.vg So I decided to handle a separate case with 0 progress
This is really confusing for the user. There are actually 11 states, 1 undamaged state, and 10 damaged states, 0-9. Using a float to set any of those 11 states seems very wrong. I feel like we should find some other way to represent those 11 states.
Like with these changes, no damage is 0, the 0 state is (0,0.1111], the 1 state is (0.11111, 0.22222], and so on until 1 is the only way you get the 10th state.
If there isn't a better way than using a float, then I think the documentation needs to be updated to reflect what values of the float does. It might seem like an impl detail, but I feel like its just some random value otherwise.
As Owen already approved it (the logic looks fine) I only want to add my comment to what MM said above. I would also agree the javadocs might be in need of a bit of an improvement here. While technically understandable with the line of thinking:
It is only unbroken on exactly 0 and only completely broken on exactly 1,
That is only somewhat clear from the javadocs for me.
A specific definition here for the two special values 0 and 1 and how they exclusively represent the levels while the other values in the float are ranges for single states would be nice. Additionally, you could use the fact that we'd have javadoc API here to add a jetbrains @Range annotation, which would also improve the workflow with the method for a lot of developers.
At which point (if you change javadocs) I'd also suggest merging this patch with https://github.com/PaperMC/Paper/blob/master/patches/server/0901-Add-custom-destroyerIdentity-to-sendBlockDamage.patch and your API addition into its api additions. If you want to do so, make sure to add yourself as a Co-Author so people still know you are responsible for the cool fix :blush:
@lynxplay I did not fully understand how to work with commits inside the repository correctly. Should I add new ones, or change old ones, and what do you mean by merge, because a merge is usually two different branches
Beyond the https://github.com/PaperMC/Paper/blob/master/CONTRIBUTING.md file I can try my best to elaborate. The Paper-API and Paper-Server folders are their own git repositories. Every patch on the root repository becomes a normal git commit in the Paper-API or Paper-Server folder respectively.
Right now, you added a new commit to the Paper-Server directory.
What I am suggesting (if you are going to add javadocs, again I don't think they are 100% needed, just a preference) I was suggesting to merge your changes into the existing patches linked in my previous comment. This is done by "editing" the commit the patch you want to edit is turned into in Paper-API and Paper-Server. So you can re-use the patches
- https://github.com/PaperMC/Paper/blob/master/patches/api/0391-Add-custom-destroyerIdentity-to-sendBlockDamage.patch
- https://github.com/PaperMC/Paper/blob/master/patches/server/0901-Add-custom-destroyerIdentity-to-sendBlockDamage.patch
and edit them (through editing the commits they produce in the API and Server folder) instead of creating a new patch. I'd rename the patch to "Improve sendBlockDamage API" to include your changes tho.
How you edit a commit in git is explained in the previously mentioned CONTRIBUTING.md file. If you have any questions, feel free to ask in our discord :) Someone will be around to help you for sure.
Resolved by upstream in https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/2f8e5bc7c07299cdcd275c86c605133107fa164e#src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
Sorry that we didn't get to your PR 😦