Qveshn / LightAPI

Bukkit Library for create invisible light source
Other
29 stars 13 forks source link

Added Support for 1.16.5 + Paper/Purpur implementation #26

Closed QuentiumYT closed 3 years ago

QuentiumYT commented 3 years ago

Hi Qveshn, After some time, I'm back with a valid PR now :D Nothing was really needed but I updated the versions and tested everything on Paper and Purpur. Works fine for me and the warning on the console is now gone :)

masonalexone commented 3 years ago

Am I able to download a copy of the 1.16.5/1.16.4 Purpur version of the plugin? Its the only plugin that doesnt work if I transfer from Paper to Purpur.

QuentiumYT commented 3 years ago

Sure Code from PR #26: https://quentium.fr/+Files/mc/LightAPI-3.4.7.jar Code from Original author's new branch: https://quentium.fr/+Files/mc/LightAPI-Bukkit-5.0.0.jar

Qveshn commented 3 years ago

I'm sorry for the delay

1. @QuentiumYT As I understand it, there have been no significant changes. You added paper and purpur just to suppress the startup warning. Right?

2. I am afraid that there are still some bugs with paper 1.16.3 in some cases (or 1.16.4, I don't remember exactly). I will test them again. Perhaps paper has already fixed its server-side code in 1.16.5. The first bug was related to multithreading. Paper did not want to allow some operations and threw exceptions The second bug was related to light deletion at y=15 of each chunk section (16x16x16). The section above did not remove the spreading light if it was completely empty or something.

3.

Am I able to download a copy of the 1.16.5/1.16.4 Purpur version of the plugin? Its the only plugin that doesnt work if I transfer from Paper to Purpur. @masonalexone What exatcly does not work with LightAPI 3.4.6? Any exceptions? Show please log with errors.

QuentiumYT commented 3 years ago

Yes Qveshn, I only bump the code to accept 1.16.5 and disable the warning on Purpur servers :)

I didn't fix any bugs, perhaps the original author is trying to do that :thinking:

Qveshn commented 3 years ago

ok, tonight I will do some tests and then accept PR.

PS. LightAPI-Bukkit-5.0.0.jar and LightAPI-3.4.7.jar are two different plugins LightAPI-3.4.7.jar is not "addon" for LightAPI-Bukkit-5.0.0.jar LightAPI-3.4.7.jar is a fork form original plugin. It is independent plugin. They have different API interfaces, but one name. You should not use two plugins simultaneously. Here some history when API iterface was changed: https://www.spigotmc.org/threads/lightapi-fork.278321/page-4#post-3532402

@QuentiumYT Are you sure that LightAPI-Bukkit-5.0.0.jar is stable and works now? Have you tested it?

QuentiumYT commented 3 years ago

Great!

I know they are 2 different plugins, it is yours or the original one I don't use both of them, in prod, I use your fork, in dev, I tested the new branch from the original author as he pushed some commits a month ago to see if this fixed an issue with blocks emitting light

I'm using it and seems to work apparently, I would not say it's stable and had to remove NMS v1_13_R2 in nms-all tho. I can't really tell about this new version

Qveshn commented 3 years ago

I will accept your PR but paper still has bugs. One of them was fixed in 1.16.5 build #595. But this bug is still present in 1.15.2 and 1.16.1-1.16.4 and papermc doesn't want to fix it. 😟 Here details: https://github.com/PaperMC/Paper/pull/5500#issuecomment-860043894 This means that deleteLight not always deletes light. This happens when you try to delete the light at any position in the first empty section (16x16x16), where all the above and adjacent sections are also empty, but the section below is not empty.

There are also other errors (related to multithreading), but I do not have time to reproduce them, as there is another important PR in the queue. https://github.com/Qveshn/LightAPI/pull/28 (support for 1.17)

Therefore, it would be wrong to say that LightAPI has "implementation" for paper (+purpur) versions like 1.15.2-1.16.4. And I don't really understand why the phrase "Could not find Paper implementation. Trying CraftBukkit instead" is so undesirable and should be suppressed. This only means that LightAPI has no legal (tested and proven) implementation for paper, but it will still use CraftBukkit implementation as "try it on your own risk". Also, this phrase is not red and displayed as INFO and not WARN. Since version 3.4.2 the restriction on only registered servers by the plugin was removed. Now any CraftBukkit-based server can use LightAPI, but with mention about it like that phrase. https://github.com/Qveshn/LightAPI/releases/tag/3.4.2

What do you think?

PS. I will accept both PRs (https://github.com/Qveshn/LightAPI/pull/26 and https://github.com/Qveshn/LightAPI/pull/28) and make release as 3.2.7

QuentiumYT commented 3 years ago

You are right, I thought if it switch to craftbukkit in any case of other server jars (paper / purpur), it could just accept those without that info. This PR wasn't really useful except having 1.16.5 implementation. My opinion is that everybody running on 1.16.x should use the latest 1.16.5 so if paper fixed that since then, it should be fine updating :)

This PR was just some learning for myself after all, to propose some "up to date" content. If it was not really useful, no problem, I don't really mind as I learned. I saw PR #28, this looks really nice and is far above my level!

Anyway, thanks for merging this PR.

Qveshn commented 3 years ago

Hi, I think I found a good solution with 3rd party servers. 😃

  1. I will suppress the warning "Could not find ... implementation. Trying CraftBukkit instead"
  2. I will clean the list of registered servers to a minimum. In any case, if no implementation is found, it will try to use CraftBukkit as usual.

Currently, all implementations are based only on CraftBukkit. If in the future for specific servers their own implementation is required, then it can always be added to this list.

Upd: Done a1c043ce9c0f86d2d4a1d4b39d084128858c885d

Qveshn commented 3 years ago

Hi Release is ready https://github.com/Qveshn/LightAPI/releases/tag/3.5.0 https://www.spigotmc.org/resources/lightapi-fork.48247/ If everything is ok, then the issue can be closed.

PS. Do you have a spigot account?

QuentiumYT commented 3 years ago

Hi, Yeah I already downloaded it :D Got notifications It can be closed, thanks!

Yeah I use "Ynohtna" as my username