Fix horse hitbox falses

[483378]

Fixes the hitboxes falses when mounting a horse (mentioned in #1794). When interacting with a horse, the player's rotation is forcefully set to the horse's rotation on the client in 1.13 and below so we don't receive the actual rotation used to interact with the horse, meaning we can't check for hitboxes when interacting with horses. We can still check for reach though.

This fix uses the horseInteractCausedForcedRotation field introduced in #2033 (that PR also includes a more detailed explanation of the exact cause) in the reach check to exempt only the hitbox check for horses when the player has interacted with one in the current tick (this also exempts any attacks in the same tick since these false too). For this to work, I had to move the reset of the boolean field near the end of the onPacketReceive method in CheckManagerListener so we can still access the true value in the reach check.

I have included a detailed comment in the reach check code explaining the exemption, please let me know if I should shorten that and refer here instead.

[SamB440]

Transactions are sent before flying, so we would reset the variable before we receive the new rotation, I don’t think this is the correct way to fix this

[483378]

Transactions are sent before flying, so we would reset the variable before we receive the new rotation, I don’t think this is the correct way to fix this

Yes, the order per client tick is transactions, attacks/interacts, then flying, right?

• The horseInteractCausedForcedRotation is set to true on an interact (in the PacketPlayerAttack listener before all checks).
• The real time reach check (which doesn't false because it doesn't check hitbox) runs on interact.
• The more accurate reach+hitbox check (which falses) runs on the next flying or transaction when this field is still true.
• Only after the flying/transaction has been processed by all packetChecks including Reach etc. (onPacketReceive in CheckManagerListener) we set it back to false

The field only has to be true for a single client tick, starting from the point we receive the interact, ending after we checked reach. Resetting it as early as possible is important (to not allow attack hitbox bypasses in the next tick). Once we receive a flying or a transaction, we can reset it safely because we know a new tick has started (this new tick can already have player influenced rotation again). Also, we're very certain to get a rotation packet directly after interacting with the horse because of the forced rotation change.

The single case where we don't receive a rotation but rather a transaction is in versions without the idle packet when the forced rotation change didn't actually modify the player's rotation (already oriented the same way as the horse!). Then we're at the beginning of the next tick and don't need to exempt anymore since the player could have already made some mouse input. If we need to exempt again the client will let us know by interacting with the horse again.

Please let me know if I misunderstood something or should test this change with any other versions. As far as I can tell from my testing it works as intended on 1.8.9 and 1.13 clients.

[483378]

Tested this again with a vanilla 1.8 client on a 1.8 Paper server and a 1.21.4 Paper server, it still fixes the falses.

[Axionize]

@Cyramek once you're done with #2199 can you take a look at this?

[Cyramek]

looks good overall
after testing, I can confirm this does fix the hitbox false positives

[ManInMyVan]

what if the new rotation is still looking at the horse but is now out of range?

[CLAassistant]

All committers have signed the CLA.

[483378]

what if the new rotation is still looking at the horse but is now out of range?

Ok I would have never thought of that. I tried it, and you're correct. There's still reach falses currently, the exact reproduction steps are:

• Spawn a horse with this command for 1.8 (other rotation values would work too but this makes it easy to test): /summon EntityHorse ~ ~ ~ {Rotation:[25f,20f],NoAI:1}
• Disable vehicle mounting using a plugin
• Right click the horse once to see the target rotation and don't move your mouse
• Walk away from the horse so far that you are still targeting the hitbox without reaching it with this rotation
• Stop walking before you can't reach the closest corner of its hitbox anymore
• Quickly aim from anywhere else (e.g. from above) and right click at the hitbox corner

The highest reach false I got during testing was 4.1 blocks.

As for the fix, I have simply moved the exemption I previously added slightly so that it also covers reach flags. However, when thinking it through I found that it would allow one bypass:

• Flag reach once so that the cancelBuffer is above zero
• Because of that, the simple realtime getMinReachToBox check in isKnownInvalid will not be executed anymore
• The precise check is now exempted for horses when interacting
• For a few hits, reach and hitbox would bypass on horses

To fix that, I execute the simple getMinReachToBox check again when exempting and use its result in place of the accurate check for horses. The simple check does not false as it always finds the optimal rotation.

[ManInMyVan]

is there a reason for not just setting minDistance here? If not this could be moved to where minDistance is calculated as an if else to save performance.

[483378]

Yeah that's better we can save a lot of performance here. I have two suggestions for the implementation:

1. Move the entire existing minDistance calculation into the else block of the horse condition and put the getMinReachToBox call in the if block (this would have less duplicate code for returning the CheckResult)
2. Early exit by moving the existing horse condition block as it is right now before the beginning of the minDistancecalculation (this would have less indention for the minDistance calculation than the first suggestion)

Both have some advantages, which would you prefer?