Init Folia support

[mani1232]

I'm not sure if it works perfectly, but it works

[MiniDigger]

I have doubts that just replacing the scheduler is enough.
the code has comments stating that its expecting to be ran from the main thread, which you are breaking (because folia doesnt have a main thread)
I havent looked deep enough at stuff to see if those comments are just about concurrency (or concurrent access to PL data structures) or because there other expectations.

furthermore, you shouldn't be reformatting the whole project and that folia check should be placed next to the spigot check in the util class here https://github.com/dmulloy2/ProtocolLib/blob/master/src/main/java/com/comphenix/protocol/utility/Util.java#L24
I would also argue that littering the codebase with if else branches like that is meh and introducing an util method to schedule stuff is both nicer code wise and also easier to optimize for the jvm (not that it matters here).

I am also not sure if raising the target compability is ok.

[dmulloy2]

@MiniDigger my thoughts exactly

[mani1232]

I have doubts that just replacing the scheduler is enough.
the code has comments stating that its expecting to be ran from the main thread, which you are breaking (because folia doesnt have a main thread)
I havent looked deep enough at stuff to see if those comments are just about concurrency (or concurrent access to PL data structures) or because there other expectations.

furthermore, you shouldn't be reformatting the whole project and that folia check should be placed next to the spigot check in the util class here https://github.com/dmulloy2/ProtocolLib/blob/master/src/main/java/com/comphenix/protocol/utility/Util.java#L24
I would also argue that littering the codebase with if else branches like that is meh and introducing an util method to schedule stuff is both nicer code wise and also easier to optimize for the jvm (not that it matters here).

I am also not sure if raising the target compability is ok.

ok, I will improve the code based on your recommendations, but my task was to make the plugin run and work without errors)

[dmulloy2]

i'm not sure what you mean as far as "task." we're not going to merge something done halfway

[mani1232]

i'm not sure what you mean as far as "task." we're not going to merge something done halfway

ok

[dmulloy2]

hard to tell what the actual diff is

[mani1232]

I did not understand what you meant in the last 3 remarks

[dmulloy2]

target compatibility needs to stay 1.8. please undo changes to formatting in the files edited

[lukalt]

So first of all thank you for your contribution! Overall the actual code changes look good to me. However, you completely reformatted all the files you edited leading to a huge diff that is hard to read. While you edited only a small part of some files, Git interprets this as an almost complete rewrite of the file as all the whitespaces and tabs were changed. I would suggest you start from scratch again and apply all your changes without reformatting the file (i.e. dont press CTRL + ALT + SHIT + LIFT in IntelliJ).

Also, as @dmulloy2 mentioned, targetCompatibility = JavaVersion.VERSION_17 // Need for Folia needs to be reverted as ProtocolLib still needs to be compatible with Java 8.

I am a little curious if it makes sense to compile ProtocolLib against folia. Maybe it would be better to use reflections for the global region scheduler access which, apparently, is the only access to the Folia API. What do the others think on this?

Regarding the formatting of the code, are there any plans to completely reformat the project to a consistent code style (especially regarding Tabs vs. Spaces) in a saperate commit? @dmulloy2

[derklaro]

I completely agree here, adding Folia as a dependency has no advantage for us, and raising the Target Java version even less.

I'm curious if there is any advantage at all to using the region scheduler. Anyway for the basic processing of packages it would be enough to run everything in one executor (maybe it has a limit of one thread for the non-async processing part? That would fit the current Bukkit behavior best).

Furthermore I would like to know if the converters that use Entity#getHandle don't work anymore. iirc this method is now protected by a thread check and you can only bypass it by using getHandleRaw (if we even decide that users can bypass this with ProcotolLib).

[dmulloy2]

@mani1232 i've gone through, cleaned up the diff, and changed it to (mostly) use reflection. just need to grab the lastSeen and lastLogin reflectively if we still need those. also would be good to do some testing and see what happens if you try to get an entity in a packet thread

stale

[mani1232]

@mani1232 i've gone through, cleaned up the diff, and changed it to (mostly) use reflection. just need to grab the lastSeen and lastLogin reflectively if we still need those. also would be good to do some testing and see what happens if you try to get an entity in a packet thread

Not working

[20:29:18 INFO]: [ProtocolLib] Enabling ProtocolLib v5.0.0-SNAPSHOT
[20:29:18 INFO]: Error Unable to create packet timeout task. (java.lang.ExceptionInInitializerError) occurred in ProtocolLib v5.0.0-SNAPSHOT.
[20:29:18 ERROR]: [ProtocolLib] INTERNAL ERROR: Unable to create packet timeout task.
If this problem hasn't already been reported, please open a ticket
at https://github.com/dmulloy2/ProtocolLib/issues with the following data:
Stack Trace:
java.lang.ExceptionInInitializerError
at ProtocolLib.jar//com.comphenix.protocol.utility.SchedulerUtil.getInstance(SchedulerUtil.java:17)
at ProtocolLib.jar//com.comphenix.protocol.utility.SchedulerUtil.scheduleSyncRepeatingTask(SchedulerUtil.java:37)
at ProtocolLib.jar//com.comphenix.protocol.ProtocolLib.createPacketTask(ProtocolLib.java:491)
at ProtocolLib.jar//com.comphenix.protocol.ProtocolLib.onEnable(ProtocolLib.java:338)
at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:279)
at io.papermc.paper.plugin.manager.PaperPluginInstanceManager.enablePlugin(PaperPluginInstanceManager.java:192)
at io.papermc.paper.plugin.manager.PaperPluginManagerImpl.enablePlugin(PaperPluginManagerImpl.java:104)
at org.bukkit.plugin.SimplePluginManager.enablePlugin(SimplePluginManager.java:507)
at org.bukkit.craftbukkit.v1_19_R3.CraftServer.enablePlugin(CraftServer.java:631)
at org.bukkit.craftbukkit.v1_19_R3.CraftServer.enablePlugins(CraftServer.java:542)
at net.minecraft.server.dedicated.DedicatedServer.e(DedicatedServer.java:274)
at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:1191)
at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:348)
at java.base/java.lang.Thread.run(Thread.java:1589)
Caused by: java.lang.IllegalArgumentException: Unable to find method runAtFixedRate(interface org.bukkit.plugin.Plugin, interface java.lang.Runnable, long, long) in io.papermc.paper.threadedregions.scheduler.FoliaGlobalRegionScheduler
at ProtocolLib.jar//com.comphenix.protocol.reflect.ExactReflection.getMethod(ExactReflection.java:59)
at ProtocolLib.jar//com.comphenix.protocol.reflect.accessors.Accessors.getMethodAccessor(Accessors.java:109)
at ProtocolLib.jar//com.comphenix.protocol.utility.SchedulerUtil.(SchedulerUtil.java:29)
at ProtocolLib.jar//com.comphenix.protocol.utility.SchedulerUtil.(SchedulerUtil.java:10)
at ProtocolLib.jar//com.comphenix.protocol.utility.SchedulerUtil$Holder.(SchedulerUtil.java:21)
... 14 more
Dump:
manager:
com.comphenix.protocol.injector.PacketFilterManager@d51a166[
plugin=ProtocolLib v5.0.0-SNAPSHOT
server=CraftServer{serverName=Folia,serverVersion=null,minecraftVersion=1.19.4}
reporter=com.comphenix.protocol.ProtocolLib$1@5aad97b4
minecraftVersion=(MC: 1.19.4)
asyncFilterManager=com.comphenix.protocol.async.AsyncFilterManager@1e780d56
pluginVerifier=com.comphenix.protocol.injector.PluginVerifier@1e22c56e
inboundListeners=com.comphenix.protocol.injector.SortedPacketListenerList@136a7fc3
outboundListeners=com.comphenix.protocol.injector.SortedPacketListenerList@254c5a2d
registeredListeners=[]
packetInjector=com.comphenix.protocol.injector.netty.manager.NetworkManagerPacketInjector@7b80f16f
playerInjectionHandler=com.comphenix.protocol.injector.netty.manager.NetworkManagerPlayerInjector@738152c4
networkManagerInjector=com.comphenix.protocol.injector.netty.manager.NetworkManagerInjector@3355bf10
debug=false
closed=false
injected=true
]
Sender:
com.comphenix.protocol.ProtocolLib@8789a12[
statistics=
packetTask=-1
tickCounter=0
configExpectedMod=-1
updater=com.comphenix.protocol.updater.SpigotUpdater@6f2572b6
redirectHandler=com.comphenix.protocol.ProtocolLib$2@6315fd19
commandProtocol=com.comphenix.protocol.CommandProtocol@2f715363
commandPacket=com.comphenix.protocol.CommandPacket@69548380
commandFilter=com.comphenix.protocol.CommandFilter@2d20ecde
packetLogging=com.comphenix.protocol.PacketLogging@61112156
skipDisable=false
isEnabled=true
loader=io.papermc.paper.plugin.manager.DummyBukkitPluginLoader@44de9c5c
server=CraftServer{serverName=Folia,serverVersion=null,minecraftVersion=1.19.4}
file=plugins/ProtocolLib.jar
description=org.bukkit.plugin.PluginDescriptionFile@9db4fc4
pluginMeta=org.bukkit.plugin.PluginDescriptionFile@9db4fc4
dataFolder=plugins/ProtocolLib
classLoader=PluginClassLoader{plugin=ProtocolLib v5.0.0-SNAPSHOT, pluginEnabled=true, url=plugins/ProtocolLib.jar}
naggable=true
newConfig=YamlConfiguration[path='', root='YamlConfiguration']
configFile=plugins/ProtocolLib/config.yml
logger=com.destroystokyo.paper.utils.PaperPluginLogger@4bc49a8c
]
Version:
ProtocolLib v5.0.0-SNAPSHOT
Java Version:
19.0.2
Server:
null (MC: 1.19.4)

[mani1232]

But it shows that it's enabled.

[dmulloy2]

good enough for a first shot at support. we can always fix bugs after some real world use