Update some comments and Javadocs

Remove references to ViaVersion's input emulation. It just tries to guess which input the player is moving with; totally unreliable.

Author: Lysandr0

NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/combined/CombinedData.java

@@ -18,7 +18,6 @@
 
 import fr.neatmonster.nocheatplus.checks.CheckType;
 import fr.neatmonster.nocheatplus.checks.access.ACheckData;
-import fr.neatmonster.nocheatplus.checks.moving.MovingData;
 import fr.neatmonster.nocheatplus.components.data.IDataOnRemoveSubCheckData;
 import fr.neatmonster.nocheatplus.players.PlayerData;
 import fr.neatmonster.nocheatplus.utilities.PenaltyTime;
@@ -35,12 +34,10 @@ public class CombinedData extends ACheckData implements IDataOnRemoveSubCheckDat
 
     /** 
      * Last sprinting state of the player as set by {@link PlayerData#isSprinting()}. Set in SurvivalFly.
-     * If the server and client are capable of reading and sending inputs respectively, {@link MovingData#input}should be used instead.
      */
     public boolean wasSprinting;
      /** 
       * Last shift state of the player as set by {@link PlayerData#isShiftKeyPressed()}. Set in SurvivalFly.
-      * If the server and client are capable of reading and sending inputs respectively, {@link MovingData#input}should be used instead.
       */
     public boolean wasPressingShift;
 

NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/model/MoveData.java

@@ -45,22 +45,34 @@ public class MoveData {
 
     /**
      * Start point of a move, or a static location (join/teleport).
+     * 
+     * <p>Quick note on semantics: these fields (from/to) follow Bukkit's PlayerMoveEvent semantics.<br>
+     * {@code from} is the server's last known position for the entity. <br>
+     * For example, if the player was at (1,1,1) and the client sends an update to (2,1,1),
+     * Bukkit will fire a move event with {@code from}=(1,1,1) and {@code to}=(2,1,1).<br>
+     * From the server's perspective, the {@code from} location is the current/last-known position
+     * and {@code to} one is the destination <i>about</i> to be applied that's been reported by the client; however, from the client's perspective they
+     * already are at the {@code to} position when the packet is sent.
      */
     public final LocationData from = new LocationData();
 
     /**
      * Indicate if coordinates for a move end-point and distances are present.
      * Always set on setPositions call.
      */
-    public boolean toIsValid = false; // Must initialize.
+    public boolean toIsValid = false; // Must initialize here as false.
 
 
     /////////////////////////////////////////////////////////////////////
     // Only set if a move end-point is set, i.e. toIsValid set to true.
     /////////////////////////////////////////////////////////////////////
     // Coordinates and distances.
     /**
-     * End point of a move.
+     * End point (destination) of a move.
+     *
+     * Essentially represents the destination reported by the client and about to be applied by the server. Example: client updates to
+     * (2,1,1) => {@code to}=(2,1,1) (the client may already be at that position).
+     * See {@link #from} for more details on semantics.
      */
     public final LocationData to = new LocationData();
 

NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/model/PlayerKeyboardInput.java

@@ -21,6 +21,7 @@
 
 /**
  * Carry information regarding the player's key presses (WASD, space bar, shift, sprint).
+ * To not be conflated with the status of the player (isSprinting, isSneaking, etc).<br>
  */
 public class PlayerKeyboardInput implements Cloneable {
 

NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/SurvivalFly.java

@@ -1125,7 +1125,7 @@ else if (lastMove.from.inLava) {
             // NOTE: this is after the riptiding propelling force.
             // NOTE: here the game uses isShiftKeyDown (so this is shifting not sneaking, using Bukkit's isShift is correct)
             if (!player.isFlying() && pData.isShiftKeyPressed() && from.isAboveGround() && thisMove.yDistance <= 0.0) {
-                Vector backOff = from.maybeBackOffFromEdge(new Vector(thisMove.xAllowedDistance, thisMove.yDistance, thisMove.zAllowedDistance));
+                Vector backOff = from.maybeBackOffFromEdge(new Vector(thisMove.xAllowedDistance, yDistanceBeforeCollide, thisMove.zAllowedDistance));
                 thisMove.xAllowedDistance = backOff.getX();
                 thisMove.zAllowedDistance = backOff.getZ();
             }
@@ -1234,7 +1234,7 @@ else if (lastMove.from.inLava) {
         if (!player.isFlying() && pData.isShiftKeyPressed() && from.isAboveGround() && thisMove.yDistance <= 0.0) {
             for (i = 0; i < 9; i++) {
                 // TODO: Optimize. Brute forcing collisions with all 9 speed combinations will tank performance.
-                Vector backOff = from.maybeBackOffFromEdge(new Vector(xTheoreticalDistance[i], thisMove.yDistance, zTheoreticalDistance[i]));
+                Vector backOff = from.maybeBackOffFromEdge(new Vector(xTheoreticalDistance[i], yDistanceBeforeCollide, zTheoreticalDistance[i]));
                 xTheoreticalDistance[i] = backOff.getX();
                 zTheoreticalDistance[i] = backOff.getZ();
             }
@@ -1508,7 +1508,7 @@ private double[] vDistRel(final Player player, final PlayerLocation from,
         // NOTE: pressing space bar on a bouncy block will override the bounce (in that case, vdistrel will fall back to the jump check above).
         // updateEntityAfterFallOn(), this function is called on the next move
         if (pData.isShiftKeyPressed() && lastMove.collideY) { 
-            if (lastMove.yAllowedDistance < 0.0) { // NOTE: Must be the allowed distance, not the actual one (exploit)
+            if (thisMove.yAllowedDistance < 0.0) { // NOTE: Must be the allowed distance, not the actual one (exploit)
                 if (lastMove.to.onBouncyBlock) {
                     // The effect works by inverting the distance.
                     // Beds have a weaker bounce effect (BedBlock.java).
@@ -1532,13 +1532,13 @@ private double[] vDistRel(final Player player, final PlayerLocation from,
                 thisMove.yAllowedDistance = 0.0;
             }
         }
-        // *----------Finalization of handleRelativeFrictionAndCalculateMovement; this check/condition is called after having called the move(). The former method is called only when the player is traveling in air, thus the liquid and gliding checks ----------*
+        // *----------Finalization of handleRelativeFrictionAndCalculateMovement; this check/condition is called after having called the move() function. The former method is called only when the player is traveling in air, thus the liquid and gliding checks ----------*
         if (!lastMove.from.inLiquid && !lastMove.isGliding) {
             // TODO: Which condition is correct ??? Check for past versions to see when this check changed... Fun. 
             // if ((this.horizontalCollision || this.jumping) && (this.onClimbable() || this.getInBlockState().is(Blocks.POWDER_SNOW) && PowderSnowBlock.canEntityWalkOnPowderSnow(this))) {
             // if ((this.horizontalCollision || this.jumping) && (this.onClimbable() || this.wasInPowderSnow && PowderSnowBlock.canEntityWalkOnPowderSnow(this))) {
             // TODO: We have to loop the jumping state for 1.21.1 and below... No other way to put it unfortunately. This will make the code an ugly mess than it already is.
-            final boolean jumpedOrCollided = lastMove.collidesHorizontally || data.input.isSpaceBarPressed() && BridgeMisc.isSpaceBarImpulseKnown(player);
+            final boolean jumpedOrCollided = lastMove.collidesHorizontally || data.input.wasSpaceBarPressed() && BridgeMisc.isSpaceBarImpulseKnown(player);
             if (jumpedOrCollided && (lastMove.from.onClimbable || lastMove.from.inPowderSnow && BridgeMisc.canStandOnPowderSnow(player))) { // this.wasInPowderSnow. The living entity field already checks for the past state, does that mean we need to check for the second last move?
                 thisMove.yAllowedDistance = 0.2;
             }
@@ -1838,6 +1838,7 @@ private double[] hDistAfterFailure(final Player player,
          *  Because this move is not sent by the client and cannot be predicted through normal means, we have to brute force it.
          */
         if (hDistanceAboveLimit > 0.0 && lastMove.isPossibleStoppingMotion(pData.getClientVersion())) {
+            tags.add("stop_motion");
             double[] res = prepareSpeedEstimation(from, to, pData, player, data, thisMove, lastMove, fromOnGround, toOnGround, debug, isNormalOrPacketSplitMove, false, false);
             hAllowedDistance = res[0];
             hDistanceAboveLimit = res[1];
@@ -1846,6 +1847,7 @@ private double[] hDistAfterFailure(final Player player,
          * 2: Undetectable jump (must brute force here): player failed with the onGround flag, lets try with off-ground then.
          */
         if (PhysicsEnvelope.isVerticallyConstricted(from, to, pData) && hDistanceAboveLimit > 0.0) {
+            tags.add("vert_constricted");
             double[] res = prepareSpeedEstimation(from, to, pData, player, data, thisMove, lastMove, fromOnGround, toOnGround, debug, isNormalOrPacketSplitMove, false, true);
             hAllowedDistance = res[0];
             hDistanceAboveLimit = res[1];

NCPCore/src/main/java/fr/neatmonster/nocheatplus/compat/BridgeMisc.java

@@ -35,7 +35,6 @@
 import org.bukkit.inventory.meta.ItemMeta;
 
 import fr.neatmonster.nocheatplus.checks.moving.MovingConfig;
-import fr.neatmonster.nocheatplus.checks.moving.MovingData;
 import fr.neatmonster.nocheatplus.compat.bukkit.BridgeMaterial;
 import fr.neatmonster.nocheatplus.compat.versions.ClientVersion;
 import fr.neatmonster.nocheatplus.players.DataManager;
@@ -104,9 +103,8 @@ public static boolean hasAnyUsingItemMethod() {
      * Test if the player's horizontal impulse (WASD presses) is either known or emulated by ViaVersion.
      * 
      * @param player The player whose input is being checked.
-     * @return <b>Always</b><code>true</code>, if both client and server are on a version that supports impulse sending and reading respectively (1.21.2 and above).<br>
-     *         <b>Always</b><code>false</code>, if the server is unable to read inputs at all (legacy, pre 1.21.2). <br>
-     *         <b>Note</b>: currently, we cannot rely on ViaVersion's input emulation for older clients on newer servers (1.21.2+); it seems to be way off from the actual input.
+     * @return True if both client and server are on a version that supports impulse reading and sending respectively (1.21.2 and above).<br>
+     *         Always false, if the server or the client is unable to read inputs at all (legacy, pre 1.21.2). <br>
      * <hr>
      * Check {@link BridgeMisc#isSpaceBarImpulseKnown(Player)} for the vertical impulse.
      *
@@ -117,43 +115,24 @@ public static boolean isWASDImpulseKnown(final Player player) {
             // TODO: What about modern clients (1.21.2+) on legacy servers (1.21.2-)? Is there a way to intercept and translate the sent input by ourselves?
             return false;
         }
-        if (DataManager.getPlayerData(player).getClientVersion().isAtLeast(ClientVersion.V_1_21_2)) {
-            // Client sends impulses and server can read them. Inputs are always known, regardless of any other condition.
-            return true;
-        }
-        //final IPlayerData pData =  DataManager.getPlayerData(player);
-        //final MovingData data = pData.getGenericInstance(MovingData.class);
-        // A legacy client (which doesn't natively send inputs) is connected to a server that supports impulse-reading.
-        // Connection is enabled by ViaVersion, which emulates the PLAYER_INPUT packet.
-        // TODO: Check how exactly ViaVersion is emulating the packet. It would be useful to have the logic within NCP.
-        //return !data.hasActiveHorVel();
-        return false;
+        // Client sends impulses and server can read them. Inputs are always known, regardless of any other condition.
+        return DataManager.getPlayerData(player).getClientVersion().isAtLeast(ClientVersion.V_1_21_2);
     }
 
     /**
      * Test if the player's vertical impulse (space bar presses) is either known or emulated by ViaVersion.
      *      
      * @param player The player whose input is being checked.
-     * @return <b>Always</b> <code>true</code> if both client and server are on a version that supports impulse reading and sending respectively (1.21.2 and above).<br>
-     *         <b>Always</b> <code>false</code>, if the server is unable to read inputs at all (legacy, pre 1.21.2). <br>
-     *         <code>true</code>, if the server supports impulse reading, but the client does not natively support impulse-sending, which is instead enabled by ViaVersion
-     *         through emulation of the PLAYER_INPUT packet. Because the packet is emulated, the actual impulse may not always reflect what the player actually pressed; particularly if the 
-     *         player's speed was affected by external velocity sources. In which case, this will return <code>false</code>.
+     * @return True if both client and server are on a version that supports impulse reading and sending respectively (1.21.2 and above).<br>
+     *         Always false, if the server or the client is unable to read inputs at all (legacy, pre 1.21.2). <br>
      * <hr>
      * Check {@link BridgeMisc#isWASDImpulseKnown(Player)} for the horizontal impulse.  
      */
     public static boolean isSpaceBarImpulseKnown(final Player player) {
         if (!hasInputGetterMethod) {
             return false;
         }
-        if (DataManager.getPlayerData(player).getClientVersion().isAtLeast(ClientVersion.V_1_21_2)) {
-            return true;
-        }
-        //final IPlayerData pData =  DataManager.getPlayerData(player);
-        //final MovingData data = pData.getGenericInstance(MovingData.class);
-        // TODO: getOrUseVerticalVelocity or peekVerticalVelocity?
-        // TODO: How exactly does ViaVersion provide?
-        return false;
+        return DataManager.getPlayerData(player).getClientVersion().isAtLeast(ClientVersion.V_1_21_2);
     }
 
     /**

NCPCore/src/main/java/fr/neatmonster/nocheatplus/utilities/location/RichBoundsLocation.java

@@ -785,9 +785,7 @@ public boolean isInPowderSnow() {
             if (blockFlags != null && (blockFlags & BlockFlags.F_POWDER_SNOW) == 0) {
                 inPowderSnow = false;
             }
-            else {
-                inPowderSnow = isInside(BlockFlags.F_POWDER_SNOW);
-            }
+            else inPowderSnow = isInside(BlockFlags.F_POWDER_SNOW);
         }
         return inPowderSnow;
     }