jonesdevelopment / sonar

Sonar is a lightweight, effective and easy-to-use anti-bot plugin for Velocity, BungeeCord, and Bukkit.
https://docs.jonesdev.xyz
GNU General Public License v3.0
125 stars 12 forks source link

Bug fixes in `bukkit-support` branch #358

Closed FallenCrystal closed 1 month ago

FallenCrystal commented 2 months ago

Changed:

Tested on 1.8.8 PandaSpigot and 1.21 server.

Compatibility tested:

ByteBuf#release located at FallbackBukkitInboundHandler, has also been deleted. Because there seems to be no sign of a memory overflow.

This was removed because there were of duplicate releases after removing FallbackInboundHandler.

These changes work though. But it looks a little messy. Accept any suggestions on code styles.

Known issues:

jonesdevelopment commented 2 months ago

@FallenCrystal Are you able to test this on 1.12.2 and 1.16.5? 1.8.8 and 1.20.4 seem to work, but @F3F5 I using 1.12.2 (I think), and there've been some errors. I'm not sure if they're fixed yet, but it would be good to check if they are.

FallenCrystal commented 2 months ago

Are you able to test this on 1.12.2 and 1.16.5

I don't have these versions of the server. I'll test that later.

FallenCrystal commented 1 month ago

Unknown disconnect error found on 1.21 bukkit server with sonar (on login state). Set the PR to a draft until the investigation is clear.

jonesdevelopment commented 1 month ago

This resolves https://github.com/jonesdevelopment/sonar/issues/349?

FallenCrystal commented 1 month ago

This resolves #349?

This only fixes the issue where disconnect messages are not sent correctly and the number of online messages is not reduced because of the change of that PR.

FallenCrystal commented 1 month ago

btw can't use viafabric anymore

That issue may not be our main objective. Because Sonar only sends the registry that the vanilla client required for. If install Sonar on a server then cannot join the server (not fallback). It may be that this issue will be taken seriously.

[com.github.retrooper.packetevents.PacketEventsAPI] Could not find encoder/decoder handler in channel pipeline!

Check if the warning message still exists in latest commit.

jonesdevelopment commented 1 month ago

Ugh... why is this all so complicated? I'd really like to just avoid all these lazy-loading mechanisms and make it as simple and straightforward as possible, but I don't think we can pull this off without some "hacks". What do you think @FallenCrystal?

I've tested it and it seems to work. I'd refactor some codestyle stuff after I eventually merge this PR, but - all in all - this looks to be functional.

I love Minecraft. I love Bukkit. I love Spigot. ... I also love all people who are responsible for all those "async optimized mega fast multi threaded server forks"

FallenCrystal commented 1 month ago

What do you think?

Almost everything in this PR is to guarantee that it works. I didn't think much about it. tbh

I removed class Lazy and got ServerConnection directly in the FallbackBukkitInjector#inject method. After reading your comment, I thought it might be better.

About initializeListener located in SonarBukkitPlugin. I don't know what else could replace it. Since I wanted to avoid flooding bots before Sonar was enabled. Maybe we can let method FallbackBukkitInjector#inject to accept a CompletableFuture<?>. It may be neater this way.

jonesdevelopment commented 1 month ago

After reading your comment, I thought it might be better.

My problem is just that it's just too complicated for what we're trying to achieve. I don't have a problem with merging this as it is. I'd just like to clean it up and simplify it as much as possible.

Almost everything in this PR is to guarantee that it works

That is good. I'll also contact some other people for helping me test this.

FallenCrystal commented 1 month ago

My problem is just that it's just too complicated for what we're trying to achieve. I don't have a problem with merging this as it is. I'd just like to clean it up and simplify it as much as possible.

I know. The reply to that paragraph is aimed at:

I'd really like to just avoid all these lazy-loading

jonesdevelopment commented 1 month ago

lgtm