Skip to content

GH-371 Fix/knockback roof teleport #371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MassiveLag wants to merge 2 commits into
base: master
Choose a base branch
from
Open

GH-371 Fix/knockback roof teleport #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acdd2da
- Changed old methods to new worldguard methods
MassiveLag Apr 9, 2026
9bbd126
fix(knockback): prevent roof teleport exploit & improve safe knockbac…
MassiveLag Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
258 changes: 229 additions & 29 deletions ...lcombat-plugin/src/main/java/com/eternalcode/combat/fight/knockback/KnockbackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,28 @@
package com.eternalcode.combat.fight.knockback;

import com.eternalcode.combat.config.implementation.PluginConfig;
import com.eternalcode.combat.region.Point;
import com.eternalcode.combat.region.Region;
import com.eternalcode.combat.region.RegionProvider;
import com.eternalcode.commons.bukkit.scheduler.MinecraftScheduler;
import com.eternalcode.commons.scheduler.Task;
import io.papermc.lib.PaperLib;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import org.bukkit.Location;
import org.bukkit.Material;
import org.bukkit.entity.Player;
import org.bukkit.event.player.PlayerTeleportEvent.TeleportCause;
import org.bukkit.util.Vector;

import java.time.Duration;
import java.util.*;

public final class KnockbackService {

private final PluginConfig config;
private final MinecraftScheduler scheduler;
private final RegionProvider regionProvider;

private final Map<UUID, Region> insideRegion = new HashMap<>();
private final Set<UUID> fallbackActive = new HashSet<>();

public KnockbackService(PluginConfig config, MinecraftScheduler scheduler, RegionProvider regionProvider) {
this.config = config;
Expand All @@ -31,57 +31,257 @@ public KnockbackService(PluginConfig config, MinecraftScheduler scheduler, Regio
}

public void knockbackLater(Region region, Player player, Duration duration) {
this.scheduler.runLater(() -> this.knockback(region, player), duration);
scheduler.runLater(() -> this.knockback(region, player), duration);
}

public void knockback(Region region, Player player) {

if (player.isInsideVehicle()) {
player.leaveVehicle();
}

Location loc = player.getLocation();
Vector direction = getDirectionToEdge(region, loc);

if (config.knockback.dampenVelocity) {
player.setVelocity(player.getVelocity().multiply(config.knockback.dampenFactor));
}

boolean onGround = Math.abs(player.getVelocity().getY()) < 0.08;

double y = onGround
? config.knockback.vertical
: Math.min(player.getVelocity().getY(), config.knockback.maxAirVertical);

Vector velocity = direction.multiply(config.knockback.multiplier).setY(y);

player.setFallDistance(0);
player.setVelocity(velocity);


if (config.knockback.useTeleport) {
scheduleSmartFallback(player, region);
}
}

public void forceKnockbackLater(Player player, Region region) {
if (insideRegion.containsKey(player.getUniqueId())) {
UUID uuid = player.getUniqueId();

if (insideRegion.containsKey(uuid)) {
return;
}

insideRegion.put(player.getUniqueId(), region);
insideRegion.put(uuid, region);

scheduler.runLater(player.getLocation(), () -> {
insideRegion.remove(player.getUniqueId());

Location playerLocation = player.getLocation();
if (!region.contains(playerLocation) && !regionProvider.isInRegion(playerLocation)) {
insideRegion.remove(uuid);

Location loc = player.getLocation();
double velocity = player.getVelocity().lengthSquared();

if (velocity > 0.02) {
return;
}

if (player.isInsideVehicle()) {
player.leaveVehicle();
if (!region.contains(loc)) {
return;
}

double distanceToEdge = getDistanceToEdge(region, loc);

if (distanceToEdge > 1.5) {
return;
}
Comment on lines +83 to +95
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The values 0.02 (on line 83) and 1.5 (on line 93) are magic numbers. They make the code harder to understand and maintain. These should be extracted into named constants with descriptive names (e.g., MIN_VELOCITY_FOR_TELEPORT, MAX_DISTANCE_FOR_TELEPORT) or, even better, made configurable in KnockbackSettings to allow server administrators to tweak them.

The value 0.02 is also used in scheduleSmartFallback (line 140), which reinforces the need for a constant.


if (fallbackActive.contains(uuid)) {
return;
}

Location generated = generate(
player.getLocation(),
Point2D.from(region.getMin()),
Point2D.from(region.getMax()),
0
);

Location safe = makeSafe(generated);
if (safe == null || safe.getWorld() == null) {
return;
}

Location location = generate(playerLocation, Point2D.from(region.getMin()), Point2D.from(region.getMax()));
PaperLib.teleportAsync(player, safe.clone(), TeleportCause.PLUGIN);

PaperLib.teleportAsync(player, location, TeleportCause.PLUGIN);
}, this.config.knockback.forceDelay);
}, config.knockback.forceDelay);
}

private Location generate(Location playerLocation, Point2D minX, Point2D maxX) {
private void scheduleSmartFallback(Player player, Region region) {
UUID uuid = player.getUniqueId();

if (fallbackActive.contains(uuid)) {
return;
}

fallbackActive.add(uuid);

final Task[] taskRef = new Task[1];

taskRef[0] = scheduler.timer(() -> {

Location check = player.getLocation();
double velocity = player.getVelocity().lengthSquared();

if (!region.contains(check)) {
fallbackActive.remove(uuid);
taskRef[0].cancel();
return;
}

if (velocity > 0.02) {
return;
}

Location generated = generate(
player.getLocation(),
Point2D.from(region.getMin()),
Point2D.from(region.getMax()),
0
);

Location safe = makeSafe(generated);
if (safe == null || safe.getWorld() == null) {
fallbackActive.remove(uuid);
taskRef[0].cancel();
return;
}

PaperLib.teleportAsync(player, safe.clone(), TeleportCause.PLUGIN);

fallbackActive.remove(uuid);
taskRef[0].cancel();

},
Comment on lines +129 to +163
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic to remove the player's UUID from fallbackActive and cancel the task is repeated three times within this lambda. This duplicated code can be extracted into a private helper method to improve readability and maintainability. For example:

private void cancelSmartFallback(UUID uuid, Task task) {
    this.fallbackActive.remove(uuid);
    if (task != null) {
        task.cancel();
    }
}

You could then call this.cancelSmartFallback(uuid, taskRef[0]); where needed.

Duration.ofMillis(100),
Duration.ofMillis(100));
}

private Location makeSafe(Location loc) {
if (loc == null || loc.getWorld() == null) return loc;

return config.knockback.safeGroundCheck
? findSafeGround(loc)
: loc.getWorld().getHighestBlockAt(loc).getLocation().add(0, config.knockback.groundOffset, 0);
}

private Location findSafeGround(Location loc) {

if (loc.getWorld() == null) return loc;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The findSafeGround method checks if loc.getWorld() is null, but it doesn't check if loc itself is null. This could lead to a NullPointerException if the method is ever called with a null location. For better robustness, you should add a check for loc == null at the beginning of the method, similar to what's done in getSafeHighest and makeSafe.

Suggested change
if (loc.getWorld() == null) return loc;
if (loc == null || loc.getWorld() == null) return loc;


Location check = loc.clone();
int minY = loc.getWorld().getMinHeight();

for (int y = check.getBlockY(); y > minY; y--) {
check.setY(y);

Material type = check.getBlock().getType();
Material above = check.clone().add(0, 1, 0).getBlock().getType();
Material above2 = check.clone().add(0, 2, 0).getBlock().getType();

if (type.isSolid()
&& !config.knockback.unsafeGroundBlocks.contains(type)
&& above.isAir()
&& above2.isAir()) {

return check.clone().add(0, config.knockback.groundOffset, 0);
}
Comment on lines +190 to +196
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic to check if a location is safe to stand on is duplicated in findSafeGround and getSafeHighest. This logic could be extracted into a private helper method to avoid code duplication and make it easier to maintain. For example:

private boolean isSafeStandable(Location location) {
    Material type = location.getBlock().getType();
    Material above = location.clone().add(0, 1, 0).getBlock().getType();
    Material above2 = location.clone().add(0, 2, 0).getBlock().getType();

    return type.isSolid()
        && !this.config.knockback.unsafeGroundBlocks.contains(type)
        && above.isAir()
        && above2.isAir();
}

Then you could simplify the loops in both methods by calling if (isSafeStandable(check)) { ... }.

}

return getSafeHighest(loc);
}

private Location getSafeHighest(Location loc) {
if (loc == null || loc.getWorld() == null) return loc;

if (!config.knockback.safeHighestFallback) {
return loc.getWorld()
.getHighestBlockAt(loc)
.getLocation()
.add(0, config.knockback.groundOffset, 0);
}

Location highest = loc.getWorld().getHighestBlockAt(loc).getLocation();
int minY = loc.getWorld().getMinHeight();

int startY = highest.getBlockY();
int maxScan = config.knockback.safeHighestMaxScan;

int endY = (maxScan < 0)
? minY
: Math.max(minY, startY - maxScan);

for (int y = startY; y > endY; y--) {
highest.setY(y);

Material type = highest.getBlock().getType();
Material above = highest.clone().add(0, 1, 0).getBlock().getType();
Material above2 = highest.clone().add(0, 2, 0).getBlock().getType();

if (type.isSolid()
&& !config.knockback.unsafeGroundBlocks.contains(type)
&& above.isAir()
&& above2.isAir()) {

return highest.clone().add(0, config.knockback.groundOffset, 0);
}
}

return config.knockback.cancelIfNoSafeGround ? null : loc;
}

private Location generate(Location playerLocation, Point2D minX, Point2D maxX, int attempts) {
if (attempts >= config.knockback.maxAttempts) {
return playerLocation;
}

Location location = KnockbackOutsideRegionGenerator.generate(minX, maxX, playerLocation);

Optional<Region> otherRegion = regionProvider.getRegion(location);
if (otherRegion.isPresent()) {

Region region = otherRegion.get();
return generate(playerLocation, minX.min(region.getMin()), maxX.max(region.getMax()));

return generate(
playerLocation,
minX.min(region.getMin()),
maxX.max(region.getMax()),
attempts + 1
);
}

return location;
}

public void knockback(Region region, Player player) {
if (player.isInsideVehicle()) {
player.leaveVehicle();
}
private Vector getDirectionToEdge(Region region, Location loc) {
double dxMin = loc.getX() - region.getMin().getX();
double dxMax = region.getMax().getX() - loc.getX();
double dzMin = loc.getZ() - region.getMin().getZ();
double dzMax = region.getMax().getZ() - loc.getZ();

double min = Math.min(Math.min(dxMin, dxMax), Math.min(dzMin, dzMax));

Point point = region.getCenter();
Location subtract = player.getLocation().subtract(point.x(), 0, point.z());
if (Math.abs(min - dxMin) < 1e-6) return new Vector(-1, 0, 0);
if (Math.abs(min - dxMax) < 1e-6) return new Vector(1, 0, 0);
if (Math.abs(min - dzMin) < 1e-6) return new Vector(0, 0, -1);
Comment on lines +272 to +274
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The value 1e-6 is a magic number used as an epsilon for floating-point comparisons. It's a good practice to define this as a named constant at the class level, such as private static final double EPSILON = 1e-6;, to improve readability and make it clear what the value represents.


return new Vector(0, 0, 1);
}

Vector knockbackVector = new Vector(subtract.getX(), 0, subtract.getZ()).normalize();
double multiplier = this.config.knockback.multiplier;
Vector configuredVector = new Vector(multiplier, 0.5, multiplier);
private double getDistanceToEdge(Region region, Location loc) {
double dxMin = loc.getX() - region.getMin().getX();
double dxMax = region.getMax().getX() - loc.getX();
double dzMin = loc.getZ() - region.getMin().getZ();
double dzMax = region.getMax().getZ() - loc.getZ();

player.setVelocity(knockbackVector.multiply(configuredVector));
return Math.min(Math.min(dxMin, dxMax), Math.min(dzMin, dzMax));
}
}
Loading