From 4b9e7c9fe311f6019e342af958b58c965c710303 Mon Sep 17 00:00:00 2001 From: asofold Date: Wed, 12 Apr 2017 12:44:35 +0200 Subject: [PATCH] Extend PlayerSetBackMethod (allow to skip risk, specify default ids). A NO_RISK flag has been added, to allow skipping workarounds such as packet level ack for skipping a set-back teleport. This remains so-and-so, because in this case the set-back still would stay 'to be done' and moves setting off from elsewhere would get cancelled, leading to re-scheduling it, still: * There could remain potential with micro moves, intentionally getting set back. * There remains an uncertainty with the telported logic initially not having been made for a set-back location getting kept 'to be done' over multiple server side ticks potentially. * Having the ability to turn off this rare (?) case, allows faster reaction, if issues should arise. The default settings can be referenced by their ids: * default.legacy for pre-1.9. * default.cautious for not taking risks (such as packet level workaround disabled, otherwise it's currently post-1.9 handling, working but not optimal on pre-1.9). * default.modern for the latest thing (currently post-1.9). The defaults have been adjusted, according to so far experience: * default.cautious contains flags SCHEDULE and NO_RISK now. * default.modern is the default now, and contains SCHEDULE (but not NO_RISK). This is used if nothing is specified in the configuration. --- .../checks/moving/MovingConfig.java | 34 +++++++++++----- .../moving/player/PlayerSetBackMethod.java | 39 +++++++++++++++++-- .../checks/moving/player/SurvivalFly.java | 5 ++- .../checks/moving/util/MovingUtil.java | 32 +++++++++++---- .../checks/net/model/TeleportQueue.java | 5 ++- 5 files changed, 92 insertions(+), 23 deletions(-) diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/MovingConfig.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/MovingConfig.java index d7a67ae8..f47f2d6c 100644 --- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/MovingConfig.java +++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/MovingConfig.java @@ -48,7 +48,8 @@ import fr.neatmonster.nocheatplus.utilities.ds.prefixtree.SimpleCharPrefixTree; import fr.neatmonster.nocheatplus.utilities.location.PlayerLocation; /** - * Configurations specific for the moving checks. Every world gets one of these assigned to it. + * Configurations specific for the moving checks. Every world gets one of these + * assigned to it. */ public class MovingConfig extends ACheckConfig { @@ -76,8 +77,9 @@ public class MovingConfig extends ACheckConfig { } /** - * Gets the configuration for a specified player . - *
NOTE: Currently only per-world configs are implemented. This method might or might not get removed some day. + * Gets the configuration for a specified player .
+ * NOTE: Currently only per-world configs are implemented. This method might + * or might not get removed some day. * * @param player * the player @@ -89,7 +91,9 @@ public class MovingConfig extends ACheckConfig { /** * Get a per-world config. - * @param worldName Exact case world name. + * + * @param worldName + * Exact case world name. * @return */ public static MovingConfig getConfig(final String worldName) { @@ -123,10 +127,16 @@ public class MovingConfig extends ACheckConfig { public final ActionList morePacketsActions; public final boolean noFallCheck; - /** Deal damage instead of Minecraft, whenever a player is judged to be on ground. */ + /** + * Deal damage instead of Minecraft, whenever a player is judged to be on + * ground. + */ public final boolean noFallDealDamage; public final boolean noFallSkipAllowFlight; - /** Reset data on violation, i.e. a player taking fall damage without being on ground. */ + /** + * Reset data on violation, i.e. a player taking fall damage without being + * on ground. + */ public final boolean noFallViolationReset; /** Reset data on tp. */ public final boolean noFallTpReset; @@ -177,7 +187,10 @@ public class MovingConfig extends ACheckConfig { // Special tolerance values: /** Horizontal buffer (rather sf), after failure leniency. */ public final double hBufMax; - /** Number of moving packets until which a velocity entry must be activated, in order to not be removed.*/ + /** + * Number of moving packets until which a velocity entry must be activated, + * in order to not be removed. + */ public final int velocityActivationCounter; /** Server ticks until invalidating queues velocity. */ public final int velocityActivationTicks; @@ -188,7 +201,7 @@ public class MovingConfig extends ACheckConfig { // General things. /** * If to allow splitting moves, due to player.getLocation reflecting - * soemthing else than from/to. + * something else than from/to. */ public final boolean splitMoves; public final boolean ignoreStance; @@ -347,6 +360,7 @@ public class MovingConfig extends ACheckConfig { final PlayerSetBackMethod playerSetBackMethod = PlayerSetBackMethod.fromString( "extern.fromconfig", config.getString(ConfPaths.MOVING_SETBACK_METHOD)); if (playerSetBackMethod.doesThisMakeSense()) { + // (Might info/warn if legacy is used without setTo and without SCHEDULE and similar?) this.playerSetBackMethod = playerSetBackMethod; } else if (ServerVersion.compareMinecraftVersion("1.9") < 0) { @@ -354,7 +368,7 @@ public class MovingConfig extends ACheckConfig { } else { // Latest. - this.playerSetBackMethod = PlayerSetBackMethod.CAUTIOUS; + this.playerSetBackMethod = PlayerSetBackMethod.MODERN; } traceMaxAge = config.getInt(ConfPaths.MOVING_TRACE_MAXAGE, 200); @@ -389,7 +403,7 @@ public class MovingConfig extends ACheckConfig { catch (IllegalArgumentException e) {} } - + // Messages. msgKickIllegalMove = ColorUtil.replaceColors(config.getString(ConfPaths.MOVING_MESSAGE_ILLEGALPLAYERMOVE)); msgKickIllegalVehicleMove = ColorUtil.replaceColors(config.getString(ConfPaths.MOVING_MESSAGE_ILLEGALVEHICLEMOVE)); diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/PlayerSetBackMethod.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/PlayerSetBackMethod.java index d683266c..16a74dcb 100644 --- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/PlayerSetBackMethod.java +++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/PlayerSetBackMethod.java @@ -36,20 +36,44 @@ public class PlayerSetBackMethod { private static final int UPDATE_FROM = 0x04; /** An additional teleport is scheduled for safety. */ private static final int SCHEDULE = 0x08; + /** Do not use workarounds that pose a risk. */ + private static final int NO_RISK = 0x10; // DEFAULTS + /** + * Pre 1.9 PlayerMoveEvent handling. + */ public static final PlayerSetBackMethod LEGACY = new PlayerSetBackMethod("default.legacy", - SET_TO); + SET_TO | NO_RISK); // NO_RISK for future updates, just in case. + /** + * Option that should work on all supported versions, worse for game-play + * experience on pre-1.9, better for post-1.9, not taking additional risks. + */ public static final PlayerSetBackMethod CAUTIOUS = new PlayerSetBackMethod("default.cautious", - CANCEL | UPDATE_FROM | SCHEDULE); + CANCEL | UPDATE_FROM | SCHEDULE | NO_RISK); + /** + * Meant for the latest thing. + */ public static final PlayerSetBackMethod MODERN = new PlayerSetBackMethod("default.modern", - CANCEL | UPDATE_FROM); + CANCEL | UPDATE_FROM | SCHEDULE); public static final PlayerSetBackMethod fromString(String name, String input) { + input = input.toLowerCase(); + // Check for presets. + if (input.equals(LEGACY.getId())) { + return LEGACY; + } + else if (input.equals(CAUTIOUS.getId())) { + return CAUTIOUS; + } + else if (input.equals(MODERN.getId())) { + return MODERN; + } + // Parse individual flags. // TODO: Perhaps complain for incomplete/wrong content, much later. - input = input.toLowerCase().replaceAll("_", ""); + input = input.replaceAll("_", ""); int flags = 0; if (input.contains("setto")) { flags |= SET_TO; @@ -63,6 +87,9 @@ public class PlayerSetBackMethod { if (input.contains("schedule")) { flags |= SCHEDULE; } + if (input.contains("norisk")) { + flags |= NO_RISK; + } return new PlayerSetBackMethod(name, flags); } @@ -105,4 +132,8 @@ public class PlayerSetBackMethod { return (flags & SCHEDULE) != 0; } + public boolean shouldNoRisk() { + return (flags & NO_RISK) != 0; + } + } diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/SurvivalFly.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/SurvivalFly.java index bdc2ebf5..1ef69004 100644 --- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/SurvivalFly.java +++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/player/SurvivalFly.java @@ -1705,8 +1705,11 @@ public class SurvivalFly extends Check { || thisMove.headObstructed || lastMove.toIsValid && lastMove.headObstructed && lastMove.yDistance <= 0.0 // 1: Hop without y distance increase at moderate h-speed. // TODO: 2nd below: demand next move to jump. Relate to stored past moves. + // TODO: Ensure the gain can only be used once per so and so. || (cc.sfGroundHop || yDistance == 0.0 && !lastMove.touchedGroundWorkaround && !lastMove.from.onGround) - && hDistanceBaseRef > 0.0 && hDistance / hDistanceBaseRef < 1.35 + && hDistanceBaseRef > 0.0 && hDistance / hDistanceBaseRef < 1.5 + && (hDistance / lastMove.hDistance < 1.35 + || hDistance / hDistanceBaseRef < 1.35) ) // 0: Ground + jump phase conditions. && ( diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/util/MovingUtil.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/util/MovingUtil.java index 822d9f0f..baeffe0a 100644 --- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/util/MovingUtil.java +++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/moving/util/MovingUtil.java @@ -44,6 +44,7 @@ import fr.neatmonster.nocheatplus.logging.StaticLog; import fr.neatmonster.nocheatplus.permissions.Permissions; import fr.neatmonster.nocheatplus.players.DataManager; import fr.neatmonster.nocheatplus.utilities.CheckUtils; +import fr.neatmonster.nocheatplus.utilities.location.LocUtil; import fr.neatmonster.nocheatplus.utilities.location.PlayerLocation; import fr.neatmonster.nocheatplus.utilities.location.RichBoundsLocation; import fr.neatmonster.nocheatplus.utilities.location.TrigUtil; @@ -422,7 +423,7 @@ public class MovingUtil { * * @param player * @param debugMessagePrefix - * @return + * @return True, if the teleport has been successful. */ public static boolean processStoredSetBack(final Player player, final String debugMessagePrefix) { final MovingData data = MovingData.getData(player); @@ -433,6 +434,7 @@ public class MovingUtil { return false; } // (teleported is set.). + final Location loc = player.getLocation(useLoc); if (data.isTeleportedPosition(loc)) { // Skip redundant teleport. @@ -445,26 +447,42 @@ public class MovingUtil { } useLoc.setWorld(null); // (player is somewhere else.) + + // Post-1.9 packet level workaround. + final MovingConfig cc = MovingConfig.getConfig(player); // TODO: Consider to skip checking for packet level, if not available (plus optimize access). // TODO: Consider a config flag, so this can be turned off (set back method). - final PlayerSetBackMethod method = MovingConfig.getConfig(player).playerSetBackMethod; - if ((method.shouldCancel() || method.shouldSetTo()) && method.shouldUpdateFrom()) { + final PlayerSetBackMethod method = cc.playerSetBackMethod; + if (!method.shouldNoRisk() + && (method.shouldCancel() || method.shouldSetTo()) && method.shouldUpdateFrom()) { /* - * Another leniency option: Skip, if we have already received an - * ACK for this position on packet level. + * Another leniency option: Skip, if we have already received an ACK + * for this position on packet level - typically the next move would + * confirm the set-back, but a redundant teleport would freeze the + * player for a slightly longer time. This could happen with the set + * back being at the coordinates the player had just been at, but + * between set back and on-tick there has been a micro move (not + * firing a PlayerMoveEvent) - similarly observed on a local test + * server once, HOWEVER there the micro move had been a look-only + * packet, not explaining why the position of the player wasn't + * reflecting the outgoing position. So here remains the uncertainty + * concerning the question if a (silent) Minecraft entity teleport + * always follows a cancelled PlayerMoveEvent (!), and a thinkable + * potential for abuse. */ // (CANCEL + UPDATE_FROM mean a certain teleport to the set back, still could be repeated tp.) final CountableLocation cl = ((NetData) CheckType.NET.getDataFactory().getData(player)).teleportQueue.getLastAck(); if (data.isTeleportedPosition(cl)) { if (data.debug) { - CheckUtils.debug(player, CheckType.MOVING, debugMessagePrefix + "Skip teleport, having received an ACK for the teleport on packet level."); + CheckUtils.debug(player, CheckType.MOVING, debugMessagePrefix + "Skip teleport, having received an ACK for the teleport on packet level. Player is at: " + LocUtil.simpleFormat(loc)); } // Keep teleported in data. Subject to debug logs and/or discussion. return false; } } - // (No ACK received yet.) + + // Attempt to teleport. final Location teleported = data.getTeleported(); // (Data resetting is done during PlayerTeleportEvent handling.) if (player.teleport(teleported, BridgeMisc.TELEPORT_CAUSE_CORRECTION_OF_POSITION)) { diff --git a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/net/model/TeleportQueue.java b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/net/model/TeleportQueue.java index f0dfe4cc..5384bdc6 100644 --- a/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/net/model/TeleportQueue.java +++ b/NCPCore/src/main/java/fr/neatmonster/nocheatplus/checks/net/model/TeleportQueue.java @@ -64,7 +64,10 @@ public class TeleportQueue { } /** - * Call for Bukkit events (expect this packet to be sent). + * Call for Bukkit events (expect this packet to be sent).
+ * TODO: The method name is misleading, as this also should be called with + * expected outgoing packet. + * * @param packetData */ public void onTeleportEvent(final double x, final double y, final double z, final float yaw, final float pitch) {