This will not check all possibilities, if default=YXZ fails:
* X or Z collision: test YZX as alternative.
* Y collision: test XZY then ZXY as alternatives.
Would need to check client code, possible of multiple client versions,
considering widespread use of protocol support plugins.
New
* Define a couple of default lists for axis order.
* Allow setting axis order via a list.
Removed
* Configuration option to disable ray-tracing.
* Configuration option 'blockchangeonly' - there may be an equivalent
later on, where checking accounts for the entire bounding box(es).
* Legacy code option (things have stabilized and axis order switching
should fix false positives).
Random
* Accidentally commit FastConsume (whitespace).
Currently typically blockbreak.direction will fail, if blockinteract
used a (the first) flying packet.
Other oddities: Block damage followed a cancelled interact event.
* Issues remain with "moved too quickly" interfering (TP loop with
waterfall/alternating, possible to escape).
* Not sure the previous modeling makes sense at all with allowing
friction now.
This probably is too much of a special case (place without interact?),
might rather hit plugin features. Apart from that, consequently, other
checks should test for this too?
[BLEEDING] just to note blockinteract changes may alter things.
Currently it looks like only blockplace.against is using the last move
of BlockInteractData - later other checks are intended to use that too,
in terms of a data source.
* Store complete event result state for interact.
* Store consumed checks as well.
* Still run the speed check, if only using the item in hand is allowed.
* Add a class to loop the flying queue with a block as target (look
only).
* Pass the flyingHandle to sub checks (doesn't necessarily make sense
with reach - should probably re-check reach with the used flying queue
state, but that's more complicated due to the possibility of split pos
vs. look).
* Use the loop class both for visible and direction (not reach).
Likely similar has to be done with BlockPlace and BlockBreak - would be
good to find a skipping heuristic for blockbreak.direction etc., so we
know we have successfully checked that block from that exact position
for this player and nothing has happened between (and so on, or a more
relaxed heuristic).
skipping redundant checking (limiting the number of visited packets
doesn't work - instead 'good candidated' for packet inversion with dig
packets should be stored together, so it's easy to just check vs. the
first/last stored one).
To be done:
* Use yaw and pitch of past packjets for Direction and Reach.
* If block break mathes the last interacted block (+ moving sequence
indicates no change), skip some checks like direction and reach there,
possibly keep track if those were run at all.
If the teleport confirm packet is available, flying packets with
AckResolution.WAITING will be cancelled.
This is real bleeding edge and might need other adjustments not to
freeze players for to be discovered edge cases. The TeleportQueue
already does contain a timeout mechanism and should return
AckResolution.IDLE after some time.
* Count all events/packets regardless of settings.
* MovingListener: Remove pos/look counting for move events.
* MovingFlying: Call the counter method according to primaryThread flag.
This way, you can calculate a more interesting set-back location from
within a hook during violation handling. All you need to do to adjust
the set-back location is to call MovingData.setTeleported(newSetBack)
and NoCheatPlus should recognize this as the location to set back to.
Please do:
* Test if the check in question would set back at all
(IViolationData.wilCancel() returns true).
Please in such a case do not:
* Don't call setTeleported if IViolationData.willCancel() returns false
:).
* Cancel violation processing.
* Teleport the player.
* Do something complicated with MovingData otherwise :).
Store an extra array of items internally and allow fast fetching (for
fastest processing, where necessary). The sortedItemNodes array is kept
for future purpose.
Despite having an interface yet, add things like unregister many objects
(items) and getting all registered items, e.g. for dealing with
sub-registries efficiently (at first code-wise).
Store items in lists by type, sort by RegistrationOrder either lazily on
fetch or upon request. Allow registering an item multiple times, but
only once per type.
Tests, adaption of signatures, additions and variants should follow.
* Add RegistrationOrder for order.
* Add sorting functionality with some tests. (See
RegistrationOrder.AbstractRegisTrationOrderSort.)
* (Except for test cases, all this is not yet in use.)
(Perhaps the sorting should be changed to use an array for output
instead of iterators, this just represents a quick way in.)
Further direction:
* Replace all the registry lists like for INotifyReload and
IDisableListener and so on within registries by a generic storage that
uses this for sorting. This way every such thing will support both
priority-based and tag-based order at the same time.
* To support different order definitions depending on what type an
interface like IRegisterWithOrder might be supported by a registry.
* Later generic event listeners might also use this, simply.
* Reduce details and only send a configurable amount of lines in
notify/chat, default to 5, just state to check logs in the end,
instead of lengthy hints.
* Log detailed output to STATUS (file/s).
* Fix wrong build number printed to set configversion.created to.
* lostGroundAscend: new condition.
* lostGroundStill: some precondition checks moved inside, new condition.
* vdistrel: Extra case with decreased lift-off gain, but second move as
if normal lift off.
* With from being on ground, and last move not having touched ground,
setSetBack(from).
* The construction routine for ActionListS is to by passed the silent
permission already, so getting optimized copies will not add another
'.silent' to the already passed permission. (Correct me if i am
mistaken).
* Clarified javadocs/names.
* Further null permissions are handled, even if not (yet) necessary.
Still to be called with the ordinary check bypass permission:
* ConfigFileWithActions: The convenience methods to obtain
(Optimized)ActionListS from the
configuration.
* AbstractActionFactory.createNewActionList: will then create actual
lists, extending the permission by '.silent', unless the permission is
null.
(Stumbled onto, while looking for optimization potential with log action
execution.)
[BREAKING]
* Due to adding a method to CheckDataFactory.
* Might alter data removal (less is removed, might've overlooked
something).
For MOVING, FIGHT, COMBINED, data removal for sub checks now is
possible, allowing for more precise resetting via commands. Other check
groups may follow on request / randomly.
Data removal for check data is logged now (only CheckData, not
IRemoveData in general), in case the debug flag is set in the data.
Later, a more flexible method may get implemented, accepting a String
id, so not only check type related data, but also any type of special
data can be removed (e.g. moving.locationtrace would not be a check
type, but allow resetting the location trace of a player).
No bypass permission is set. The yawrate part depends on multiple other
checks. Checking for exemption is a hack within feedYawRate, fastest way
to implement.
HashMapLOW
* Add a constructor for using an external lock.
* Add putIfAbsent.
DataManager
* Use playerDataMap.putIfAbsent, return the PlayerData instance that has
been there first.
The per-config-path notifications would keep showing up, even if you
removed the paths, then run 'ncp reload', then alter any of them and run
'ncp reload'.
To fix this, the configversion.created value is set to the current
build, if no config warnings are there - which is the same, as what the
notification suggests as an alternative to removing the paths and
running 'ncp reload'.
To do this, isConfigUpToDate had to be moved from Updates to
ConfigManager, which makes more sense anyway. In addition the 'created'
and saved 'values' are set to the biggest thing found, instead of the
prehistoric static value.
Further a negative 'created' value will not be overridden anymore,
allowing to silence the config notifications forever. Not necessarily
recommended for the general case, but it can be useful/necessary with
maintained blueprints, e.g. with administering multiple servers.
One of the next steps will be to remove the DefaultConfig.buildNumber in
favor of setting a build number for each and every path added. All
provided we don't run into nasty issues here.
Another follow up could be to create an extra registry/config log file
and write all the values there, and only print the first 5 in ingame
chat.
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.
Where it's known that it's the primary thread, that test should be
omitted.
A remaining problem is the Check class, where the convenience methods
all will lead to testing for Bukkit.isPrimaryThread(). This needs to be
done differently.
It'll be hard/impossible to work around, if we have to check permissions
and meta data. For permissions we could do some kind of bulk/context
dependent caching and updating policy and check via PlayerData, but meta
data needs the Bukkit API to state thread safety. Future design could
also do without knowledge of the thread, if permissions are cached and
exemption by meta data is turned off (or also cached, but this only
works if other plugins don't use it for temporary exemption), a lazy
approach could pass on an AlmostBoolean isPrimaryThread.
For now, at least some of the frequently run moving checks use the
optimized approach.
* HashMapLOW for thread-safe access to PlayerData instances.
* PlayerData removal now used the UUID. More changes pending for storing
the UUID for reference rather.
* Use bulk removal for expiration of entries (one time lock).
Can't do much better than being there already. Thinkable trouble could
be with high latency and multiple teleports to different locations in
quick succession, so that cancelling the teleport will lead to the
player violating survivalfly again in the future, which means longer
freezing/rubberbanding than if we teleport now. However, current
assumption is, that it's better not to keep teleporting players around.
Instead of maps for each individual purpose, and the rather expensive
TickListener adding and removing, player specific task will be done via
one PlayerTickListener that can be registered with the TickTask. Thus
PlayerData has the access methods requestUpdateInventory and
requestPlayerSetBack, and so on, later more. For the
DataManager.playerData map it'll be UUID first now.
Consequently some calls have been altered to prefer passing Player or
UUID for PlayerData getting.
Breaks: DataManager.getPlayerData(String, boolean) has been removed, new
methods added to do the same without boolean or with UUID passed extra.
Following changes may repeatedly/randomly break PlayerData and check
data access (unless you use CheckType.getDataFactory), this may not
follow directly, but more or less soon. Even Later, CheckType will get
broken too :), in favor of class instances with dynamic registration
ability.
Basic direction is to concentrate stuff in PlayerData, getting rid of
all the static data stores, but also making access to shared data
more efficient (e.g. store last world id + name and permission cache in
PlayerData). Access will be more thread safe (only for PlayerData,
permissions cache, likely for fetching check data too, however returned
objects may have their own contracts).
* The player is at the coordinates.
* The last received ACK for an outgoing teleport has been received on
packet level. This is experimental, to be confirmed to a) do something
b) not allow abuse.
We do need to fix behavior to move on, so the intention rather is to
react more flexibly towards debugging results, rather than having people
use random configurations early on. Still this does allow for fall-back
configuration, e.g. for live servers.
NOTES:
* Later we will make the configuration set at 'default' adjust to server
mod and version.
* Overriding checks.moving.setback.method with SET_TO can lead to
PlayerTeleportEvents with TeleportCause.PLUGIN on newer versions of
Spigot, which may conflict with other plugins assuming feature-based
teleportation (possibly resulting in /back locations getting overridden
wrongly). For legacy setups this will be similar to restoring the state
of build 1066 for most.
There could still be places where distinction of the used method is
necessary, which would mean bugs.
* Don't unset teleported, if event.getTo is the same position as the
teleported (set back) location.
* Prepare (with) comments.
(Main driver is to be able to adjust quickly without shifting code
to-fro legacy etc., while dealing with much differing side conditions
for server mod + version, client versions with multi protocol support,
and other like bungee or not bungee.)
* MovingListener.prepareSetBack makes more sense than the previous
ambiguous naming.
* On confirming a set back, don't update the setBack field, only set if
null.
The aim is to have a more consistent handling and naming for set back
stages.
Both schedule a set back and update PlayerMoveEvent.getFrom() with the
set back location coordinates. This way, either the next incoming move
or a teleport event can confirm the set back location.
When a set back is scheduled:
* Cancel other teleports early. (x)
* Prevent Portal use. (x)
* Vehicle enter (not on vehicle set back). (x)
* Prevent attacking.
* Interact block. (x)
* Break block.
* Damage block. (x)
* Launch projectile.
* Place Block.
* Interact entity.
* Open inventory. (x)
The list is incomplete and adding/removing items remains subject to
discussion, having differing impact/severity for different actions. As
long as setting back rolls back to last ground, it might be better to
prevent some type of actions. Not all cancelling is logged.
(x) Probably most important for consistency, avoiding some types of
potential abuse.
A common framework
for "prevent types of action" during whatever-handling also is something
to consider.
Optimizations:
* Move handling some rare cases to methods (MovingListener,
PlayerTeleportEvent handling).
When the captain leaves the boat, the vehicle data of a passive player
passenger may be set now, so it won't set them back to where they
entered the vehicle.
Likely other places still need adjusting.
Tested with a pig. It's not nice.
* Vehicle envelope needs a lot of overhaul.
* Force fall set backs may be more nice to have for in-air downwards.
* The set back locations can be from seconds ago, with different
passengers than at the time of the set-back.
Because Spigot changed to fire the teleport following an altered move
end point with TeleportCause.PLUGIN, we have to alter set back handling,
so we can ensure to keep TeleportCause.UNKNOWN for setting back players.
Instead of altering the move end point, the event is just cancelled, and
a teleport is scheduled (with a dedicated TickTask method). Uncancelled
moving events mean removing scheduled teleports.
[BLEEDING]
* Comparably simple change - more places and special cases may still be
uncovered.
[BREAKING]
* Plugins that may rely on the exact sequence of things within NCP, as
it used to be.
Random
* Change "set-back" to "set back" everywhere for simplicity, and to
obfuscate the actual code changes.
* Set backs are now going through MovingListener.onCancelledMove instead
of MovingListener.onMoveMonitorNotCancelled.
* Illegal move handling would still use event.setTo.
* Split log messages: ingame vs. console+file
* Alter flylong to contain tags.
Performance-wise, it's not optimal that flylong is the same as flyfile
at this stage. Not sure if to remove flylong in favor of using flyfile,
to indicate it means details - main objective is to get minimal details
into the violation log, even if people edit it out of the ingame chat
messages.
Players and vehicles:
Instead always use UNKNOWN, as that is what results from
PlayerMoveEvent.setTo(newTo), as we use it for setting back players.
This should break functionality that relies on TeleportCause.PLUGIN
being used, possibly more likely with vehicles.
Internally, the tick is stored with a change id, so we can reuse a
changeId, if the tick is still the same, preserving rough order, as the
changeId should at least increase with the tick.
This isn't the last word on a public API, there likely will be the
following additions.
* One more simplified method with a minimal signature for simple
(non-push) entries (worldId, x, y, z, previousState).
* Optimized methods for adding multiple entries at once. Likely for
adding multiple entries of the same type/data/shape. Plus perhaps
simplified signatures to do without changeId and tick.
(Not sure if there will be need for a method allowing for a collection
of a to be defined class combining x+y+z+previousState.)
This will evolve based on feedback from GitHub issues.
Lots of issues remain with elytra, with and without boost. Selection:
* maxheight will trigger with the rocket feature, naturally. Mendable by
increasing it via configuration
(checks.moving.creativelfly.model.elytra.vertical.maxheight). Not sure
we'll just increase the limit or alter how it's dealt with (e.g. also
for sf: lock to a max / high slope value, independently of the set-back
and world height, alter as necessary).
* All sorts of transitions, e.g. onto ground, into water.
* Loss of boost right after adding (not sure if already fixed).
* What with hover, actually?
* Is the flight duration infinite with power 127?
* Issues with ascending after descending, even without boost?
Changes contain:
* MCAccess.dealFallDamageFiresAnEvent -> true
* Always log basic data on (handled) fall damage events.
* Add a tag for the cancel reason with NoFall. Alter the default alert
message.
* Move 3.0 to Magic.
* Set the skipping flag correctly on allowFlight being set.
Fall damage is adjusted or cancelled, if the Minecraft fall distance is
greater than the distance(s) tracked by NCP (per move diffs, maximum y).
Intention is to prevent (speeding by) self damage by abusing Minecraft
dealing fall for untracked moves.
Issue: https://github.com/NoCheatPlus/Issues/issues/338
Steps done:
* Add more velocity with less preconditions.
* Handle push with the player being just above the block where a slime
block is pushed to.
* Exception for vdistrel.
* Breaking:Move verVelUsed from MovingData to PlayerMoveData.
Gameplay issues remaining:
* Still too often fall damage is dealt.
* Friction envelope gets hidden with past-ground being set too often
(vdistsb).
* Potential for more edge cases.
Missing abuse prevention:
* Reinstate invalidation of past entries (currently turned off to
progress at all, will need another iteration).
* More confined preconditions.
* More/better invalidation conditions for velocity set for bounce
effects.
* Less fall damage.
* Flag all velocity added due to slime bouncing appropriately.
* Experimental concept for splitting up velocity, likely to be altered /
removed.
* Add a flag for (faked) pvp velocity.
NOTE: Invalidation of past entries has been deactivated to progress on detecting the stupid past bouncing at all. This will need another iteration.
Lots of issues remain, but vertical push/bounce with pistons is much
improved. Still there are show stoppers (false positives remain, as well
as occasional fall damage).
Use same/similar entry points, like static/classic bounce checking in
MovingListener. SurvivalFly still keeps one exception with the
after-failure Y_POS block move check.
Also check: https://github.com/NoCheatPlus/Issues/issues/5
There are typical cases to cover:
* Extra falling height.
* Fall damage where a slime block had been.
Thus adding a specialized method for bounce (just foot position) instead
of using the full bounds would be better, preferably just check within
the MovingListener (set bounce / adjust velocity there).
Still incomplete, could contain bugs (endless loops, perhaps).
Invalidation
mechanics may need to be refined.
Not covered:
* Exemption for moves resulting from horizontal push/pull.
*
* In case of non-full bounds + variable, allow ground to be found
underneath. Can concern fences (tested on 1.11).
* There has been a probably misplaced/leftover check assuming the block
above to be passable, without checking for it (iMaxY). Since the
access.getMaxBlockY() has already been checked, this part appears to be
not of use, thus iMaxY has been removed from the signature together with
this exception.
If this causes something, please provide debug logs / circumstances :).
This is unfinished, but gives an outlook on what we may be able to do
with this.
* Double doors are not covered (upper/lower half are).
* Interaction with levers / directly with doors is not covered.
* Only doors + gates (not sure what else there is).
* More fine grained configurability is missing (+ only register
listeners if needed).
* Possibly other things.
Tracking BlockRedstone seems to be more promising than BlockPhysics, as
long as we don't have to inspect neighbour blocks to check for door like
blocks at all.
Currently the oldest available entries are used, as if the player had
the maximum allowed latency always. Later on, at least attempting to hit
a global latency estimate (+window) should be attempted.
We'll still have a (moderate) fast-forward implementation to see what
opportunistic checking can do on live servers (elevators, doors etc.).
Intention is to work towards passing stored IBlockCacheNode instances to
methods in BlockProperties, i.e. to have
isOnGroundInAnOverlyOpportunisticWay implemented.
If you need one of the old BlockProperties signatures (or a more
simplified one), just open a GitHub issue.
This is a slightly peculiar change.
* Passing node and nodeAbove is a little bit odd, despite seemingly
efficient at present.
* Later we might need a BlockCache instance that busies about past
states (currently too complicated to implement).
* Uncertain impact (could perform better, could perform worse).
* Possibly left out a couple of places.
* Might have introduced bugs (fast forward).
On the other hand there may be a lot of other types of changes, if we
ever go for a less opportunistic implementation, which can not be
estimated simply, so this may be about the best that can be done, to get
a quick step in.
Already have the signature use IBlockCacheNode for node+nodeAbove, like
it will be done with other methods like collidesBlock.
This will allow calling this with past block states, once other methods
have been adapted as well.
