Fix left click on air being ignored and right click on block being handled twice

[Yeregorix]

Left click on air

Bukkit: Left click on air was already detected properly.
Sponge: Added a listener for InteractItemEvent.Primary.
Forge and Fabric: The click is detected using the swing arm packet. Sponge and Spigot are using this same technique. Sponge and Spigot are doing raytracing to ensure it's a click on air and not on a block but in our case we don't need to do raytracing, just deduplication.

There was a workaround on Forge where the mod if installed on the client would notify the server about the click. This workaround is no longer needed and has been removed.

Right click on block

The right click on a block was handled twice because a RightClickBlock event is always followed by a RightClickAir event on all platforms. To fix this, Event handlers store the tick number during which the last interaction occured and ignore air interactions that are too close (tick delta <= 1).

This mechanism is active for both left and righ clicks.

[me4502]

Has the performance impact of this been measured? I remember previously doing this for CraftBook in the past, purely with sign clicks that would sometimes be duplicated and found that there was actually a measurable impact on some servers.

[Yeregorix]

I haven't measured the performance yet.

The deduplication is just a map, a specialized fastutil map, so it should have a really low cpu and memory footprint. In fact, the deduplication, would probably avoid some extra processing of double clicks so it will slightly improve performance on this side.

About the swing arm packet to detect a left click on air, this is what has been used by Bukkit and Sponge for years and afaik no one has ever reported a performance issue on this part. Moreover, the implementation here is much simpler and lighter (no raytracing needed).

Just in case, I will do some measures in-game with Spark's profiler but I doubt I will see anything noticeable.

[Yeregorix]

I rebased on latest version/7.2.x commit.

I did some benchmarks in-game using Spark and an auto-clicker.
World: superflat (to avoid measuring worldgen lag).
Click speed: 100 clicks / second (theoretical click speed is limited to 20c/s but it doesn't hurt to ensure extra packets will not cause overload).
Number of clicks: 3000 left clicks then 3000 right clicks.

Results:
Sponge: https://spark.lucko.me/ku7MJdFyW8
Fabric: https://spark.lucko.me/lxoD5XGsYX
Forge: https://spark.lucko.me/iiRgGkyhr1
Spigot: https://spark.lucko.me/OPUxcJbsqN

As expected the performance impact is negligible on all platforms. The benchmark procedure is manual and not rigorous but I doubt we would see a different result with another approach.

[octylFractal]

Overall the way the debounce is handled is fine, but I don't like having the code repeated in every platform. Why not do this in we.handleXClick instead?

I also think that the fix for LCA in Forge/Fabric should be separated from this PR.

[Yeregorix]

Overall the way the debounce is handled is fine, but I don't like having the code repeated in every platform. Why not do this in we.handleXClick instead?

There is some platform specific stuff related to deduplication.

LCA -> always deduplicated.
RCA -> always deduplicated.
LCB -> never deduplicated.
RCB -> deduplicated only for Bukkit. This is because the events fire in a slightly different order in Bukkit than other platforms.

It's been a while since I've worked on this, I will probably check again if the Bukkit-specific stuff is really needed.

I also think that the fix for LCA in Forge/Fabric should be separated from this PR.

LCA is detected using the swing arm packet which is also present for some other click scenarios. In this case, LCA always happen last, so we must deduplicate it. Since LCA is heavily relying on deduplication I don't think we should move it into another PR. LCA without deduplication would trigger a lot of false positives.

[Yeregorix]

I applied your suggestions!

• Rebased.
• Factorized the debouncing mechanism. *
• Cache the event result when debouncing.
• Modern switch expression.
• nanoTime

* I added a getTickCount method into Platform. I'm not sure whether this is the right place for it or not.
I have also discovered that field server was always null inside ForgePlatform so I fixed it.

[me4502]

Would it be possible to please rebase this due to conflicts? (involves 1.20 update too)

[Yeregorix]

Sure I will do ASAP but I'm really busy this week

[Yeregorix]

Rebased and retested.

[me4502]

Thanks for this :)