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.
This commit is contained in:
asofold 2017-04-12 12:44:35 +02:00
parent ea5a064132
commit 4b9e7c9fe3
5 changed files with 92 additions and 23 deletions

View File

@ -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 .
* <br>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 . <br>
* 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));

View File

@ -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;
}
}

View File

@ -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.
&& (

View File

@ -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)) {

View File

@ -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).<br>
* 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) {