Skip to content
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

Jump to bottom

SpectateLogin implementation #2552

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

SpectateLogin implementation #2552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8692637
Add SpectateLogin implementation
invalid-email-address Jun 2, 2022
e51fbdf
Change according to the style guide
invalid-email-address Jun 3, 2022
b9bb7bd
Code improvements
invalid-email-address Jun 5, 2022
208790e
Update pom.xml
mrchefran Jun 5, 2022
6de4a9c
Fix some Checkstyle issues
invalid-email-address Jun 5, 2022
37dd1b0
Merge branch 'stand-spectating' of https://github.com/mrchefran/AuthM…
invalid-email-address Jun 5, 2022
7cd7bdb
Remove stands on plugin shutdown
invalid-email-address Jun 5, 2022
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
27 changes: 27 additions & 0 deletions src/main/java/fr/xephi/authme/listener/PlayerListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import fr.xephi.authme.service.AntiBotService;
import fr.xephi.authme.service.BukkitService;
import fr.xephi.authme.service.JoinMessageService;
import fr.xephi.authme.service.SpectateLoginService;
import fr.xephi.authme.service.TeleportationService;
import fr.xephi.authme.service.ValidationService;
import fr.xephi.authme.settings.Settings;
Expand All @@ -19,6 +20,7 @@
import fr.xephi.authme.settings.properties.RegistrationSettings;
import fr.xephi.authme.settings.properties.RestrictionSettings;
import org.bukkit.ChatColor;
import org.bukkit.GameMode;
import org.bukkit.Location;
import org.bukkit.entity.HumanEntity;
import org.bukkit.entity.Player;
Expand Down Expand Up @@ -49,6 +51,8 @@
import org.bukkit.event.player.PlayerQuitEvent;
import org.bukkit.event.player.PlayerRespawnEvent;
import org.bukkit.event.player.PlayerShearEntityEvent;
import org.bukkit.event.player.PlayerTeleportEvent;
import org.bukkit.event.player.PlayerToggleSneakEvent;
import org.bukkit.inventory.InventoryView;

import javax.inject.Inject;
Expand Down Expand Up @@ -90,6 +94,8 @@ public class PlayerListener implements Listener {
private PermissionsManager permissionsManager;
@Inject
private QuickCommandsProtectionManager quickCommandsProtectionManager;
@Inject
private SpectateLoginService spectateLoginService;

// Lowest priority to apply fast protection checks
@EventHandler(priority = EventPriority.LOWEST)
Expand Down Expand Up @@ -508,4 +514,25 @@ public void onPlayerInventoryClick(InventoryClickEvent event) {
event.setCancelled(true);
}
}

@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onToggleSneak(PlayerToggleSneakEvent event) {
if (listenerService.shouldCancelEvent(event.getPlayer())
&& (settings.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)
|| spectateLoginService.hasStand(event.getPlayer()))) {
event.setCancelled(true);
}
}

@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onTeleport(PlayerTeleportEvent event) {
if (listenerService.shouldCancelEvent(event.getPlayer())
&& event.getCause() == PlayerTeleportEvent.TeleportCause.SPECTATE
&& event.getPlayer().getGameMode() == GameMode.SPECTATOR
&& (settings.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)
|| spectateLoginService.hasStand(event.getPlayer()))) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this checks out since listenerService.shouldCancelEvent(event.getPlayer()) will be false if the player is logged in, right? In other words, even when we enable a feature, players can still teleport to other players in spectate mode once they've logged in

Copy link
Author

Choose a reason for hiding this comment

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

These checks are designed to prevent unauthorized players from teleporting (using hotbar) while they are in spectator gamemode

spectateLoginService.updateTarget(event.getPlayer());
event.setCancelled(true);
}
}
}
13 changes: 13 additions & 0 deletions src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import fr.xephi.authme.service.CommonService;
import fr.xephi.authme.service.PluginHookService;
import fr.xephi.authme.service.SessionService;
import fr.xephi.authme.service.SpectateLoginService;
import fr.xephi.authme.service.ValidationService;
import fr.xephi.authme.service.bungeecord.BungeeSender;
import fr.xephi.authme.service.bungeecord.MessageType;
Expand Down Expand Up @@ -81,6 +82,9 @@ public class AsynchronousJoin implements AsynchronousProcess {
@Inject
private ProxySessionManager proxySessionManager;

@Inject
private SpectateLoginService spectateLoginService;

AsynchronousJoin() {
}

Expand Down Expand Up @@ -191,6 +195,15 @@ private void processJoinSync(Player player, boolean isAuthAvailable) {
int blindTimeOut = (registrationTimeout <= 0) ? 99999 : registrationTimeout;
player.addPotionEffect(new PotionEffect(PotionEffectType.BLINDNESS, blindTimeOut, 2));
}

if (service.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)) {
// The delay is necessary in order to make sure that the player is teleported to spawn
Copy link
Member

Choose a reason for hiding this comment

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

What about listening at the player spawn/respawn event?

Copy link
Member

Choose a reason for hiding this comment

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

+ doesn't the armor stand have to be deleted if the player disconnects as well?

Copy link
Author

Choose a reason for hiding this comment

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

What about listening at the player spawn/respawn event?

Looks like this will also require a delay, because at this stage it's impossible to change the player gamemode

Copy link
Author

Choose a reason for hiding this comment

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

  • doesn't the armor stand have to be deleted if the player disconnects as well?

Armorstands are removed if an unauthorized player leaves the server

// and after authorization appears in the same place
bukkitService.runTaskLater(() -> {
spectateLoginService.createStand(player);
}, 1);
}

commandManager.runCommandsOnJoin(player);
});
}
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
import fr.xephi.authme.service.BukkitService;
import fr.xephi.authme.service.CommonService;
import fr.xephi.authme.service.JoinMessageService;
import fr.xephi.authme.service.SpectateLoginService;
import fr.xephi.authme.service.TeleportationService;
import fr.xephi.authme.service.bungeecord.BungeeSender;
import fr.xephi.authme.settings.WelcomeMessageConfiguration;
import fr.xephi.authme.settings.commandconfig.CommandManager;
import fr.xephi.authme.settings.properties.RegistrationSettings;
import fr.xephi.authme.settings.properties.RestrictionSettings;
import org.bukkit.entity.Player;
import org.bukkit.potion.PotionEffectType;

Expand Down Expand Up @@ -57,6 +59,9 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
@Inject
private PermissionsManager permissionsManager;

@Inject
private SpectateLoginService spectateLoginService;

ProcessSyncPlayerLogin() {
}

Expand Down Expand Up @@ -98,6 +103,11 @@ public void processPlayerLogin(Player player, boolean isFirstLogin, List<String>
player.removePotionEffect(PotionEffectType.BLINDNESS);
}

if (commonService.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)
|| spectateLoginService.hasStand(player)) {
spectateLoginService.removeStand(player);
}

// The Login event now fires (as intended) after everything is processed
bukkitService.callEvent(new LoginEvent(player));

Expand Down
8 changes: 8 additions & 0 deletions src/main/java/fr/xephi/authme/process/logout/ProcessSyncPlayerLogout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import fr.xephi.authme.process.SynchronousProcess;
import fr.xephi.authme.service.BukkitService;
import fr.xephi.authme.service.CommonService;
import fr.xephi.authme.service.SpectateLoginService;
import fr.xephi.authme.service.TeleportationService;
import fr.xephi.authme.settings.commandconfig.CommandManager;
import fr.xephi.authme.settings.properties.RegistrationSettings;
Expand Down Expand Up @@ -44,6 +45,9 @@ public class ProcessSyncPlayerLogout implements SynchronousProcess {
@Inject
private CommandManager commandManager;

@Inject
private SpectateLoginService spectateLoginService;

ProcessSyncPlayerLogout() {
}

Expand All @@ -65,6 +69,10 @@ public void processSyncLogout(Player player) {

service.send(player, MessageKey.LOGOUT_SUCCESS);
logger.info(player.getName() + " logged out");

if (service.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)) {
spectateLoginService.createStand(player);
}
}

private void applyLogoutEffect(Player player) {
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/fr/xephi/authme/process/quit/ProcessSyncPlayerQuit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,29 @@

import fr.xephi.authme.data.limbo.LimboService;
import fr.xephi.authme.process.SynchronousProcess;
import fr.xephi.authme.service.CommonService;
import fr.xephi.authme.service.SpectateLoginService;
import fr.xephi.authme.settings.commandconfig.CommandManager;
import fr.xephi.authme.settings.properties.RestrictionSettings;
import org.bukkit.entity.Player;

import javax.inject.Inject;


public class ProcessSyncPlayerQuit implements SynchronousProcess {

@Inject
private CommonService service;

@Inject
private LimboService limboService;

@Inject
private CommandManager commandManager;

@Inject
private SpectateLoginService spectateLoginService;

/**
* Processes a player having quit.
*
Expand All @@ -26,6 +35,11 @@ public void processSyncQuit(Player player, boolean wasLoggedIn) {
if (wasLoggedIn) {
commandManager.runCommandsOnLogout(player);
} else {
if (service.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)
|| spectateLoginService.hasStand(player)) {
spectateLoginService.removeStand(player);
}

limboService.restoreData(player);
player.saveData(); // #1238: Speed is sometimes not restored properly
}
Expand Down
86 changes: 86 additions & 0 deletions src/main/java/fr/xephi/authme/service/SpectateLoginService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package fr.xephi.authme.service;

import fr.xephi.authme.settings.properties.RestrictionSettings;
import org.bukkit.GameMode;
import org.bukkit.Location;
import org.bukkit.entity.ArmorStand;
import org.bukkit.entity.Player;

import javax.inject.Inject;
import java.util.HashMap;
import java.util.Map;

public class SpectateLoginService {
Copy link
Member

Choose a reason for hiding this comment

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

Good name! Allows us to remember that this will set players to SPECTATOR game mode :)

Copy link
Author

Choose a reason for hiding this comment

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

Added a description of the class behavior


private Map<Player, ArmorStand> armorStands = new HashMap<>();
private Map<Player, GameMode> gameModeMap = new HashMap<>();

@Inject
private CommonService service;

/**
* Creates a stand for the player
*
* @param player the player
*/
public void createStand(Player player) {
Location location = player.getLocation();
ArmorStand stand = spawnStand(location);

armorStands.put(player, stand);
gameModeMap.put(player, player.getGameMode());
Copy link
Member

Choose a reason for hiding this comment

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

What if the server shuts down while players are not logged in? I guess they'll be saved with the spectator game mode, and next time when they'll join, they'll already have spectator game mode and won't be able to leave it. I think this is quite relevant because it's not only an inconvenience to a player, it's giving them access to a game mode that many servers don't want to give to players...
I could imagine trying to make use of the limbo player stuff or to maybe have an additional setting that will set players to survival instead of spectator when the code would want to. It's ugly but we've had to add a few of those failsafe properties in the past and people seem happy with it

Copy link
Author

Choose a reason for hiding this comment

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

Apparently, when the plugin disabled, I can call a function that will remove all armorstands and return the players to their previous state. Is it possible to implement it this way?

Copy link
Author

Choose a reason for hiding this comment

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

Could you please tell me if I can call SpectateLoginService in onDisable directly to remove armorstands, or is this not a good practice?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply.
I don't see any better way. I think you could get the SpecateLoginService with injector.getIfAvailable(SpectateLoginService.class) in AuthMe#onDisable. But still, when a server unexpectedly shuts down (if it crashes or its process is terminated abruptly) we have the problem that players are potentially in spectator mode, and there are armor stands that no one tracks anymore. I don't know if the armor stands are an issue (I have zero experience with the way you spawn them).
Wondering if we should have a property that specifies what game mode a player should change to if after authentication, we would put him into spectate game mode and/or to add the game mode to the LimboPlayer. I can elaborate on the whole limbo player thing if it helps

Copy link
Author

Choose a reason for hiding this comment

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

Untraceable armorstand is not as critical as players in Spectator gamemode. It would be really cool if it could be done with LimboPlayer

Copy link
Member

Choose a reason for hiding this comment

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

What about adding a "limboArmorStandUUID" field to LimboPlayer?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean adding a way to keep track of whether a player is assigned an armorstand?

Copy link
Member

Choose a reason for hiding this comment

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

@mrchefran yes, exactly


player.setGameMode(GameMode.SPECTATOR);
player.setSpectatorTarget(stand);
}

/**
* Updates spectator target for the player
*
* @param player the player
*/
public void updateTarget(Player player) {
ArmorStand stand = armorStands.get(player);
if (stand != null) {
player.setSpectatorTarget(stand);
}
}

/**
* Removes the player's stand and deletes effects
*
* @param player the player
*/
public void removeStand(Player player) {
ArmorStand stand = armorStands.get(player);
if (stand != null) {

stand.remove();
player.setSpectatorTarget(null);
player.setGameMode(gameModeMap.get(player));

gameModeMap.remove(player);
armorStands.remove(player);
}
}

public boolean hasStand(Player player) {
return armorStands.get(player) != null;
Copy link
Member

Choose a reason for hiding this comment

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

can be armorStands.containsKey(player)

Copy link
Author

Choose a reason for hiding this comment

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

Changed to it

}

private ArmorStand spawnStand(Location loc) {
double pitch = service.getProperty(RestrictionSettings.HEAD_PITCH);
double yaw = service.getProperty(RestrictionSettings.HEAD_YAW);
Location location = new Location(loc.getWorld(), loc.getX(), loc.getY(), loc.getBlockZ(),
(float) yaw, (float) pitch);

ArmorStand stand = location.getWorld().spawn(location, ArmorStand.class);
Copy link
Member

Choose a reason for hiding this comment

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

Does this cause issues if two players have the same location?

Copy link
Author

Choose a reason for hiding this comment

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

This does not affect the position of the stand in any way. The only thing is that players can see the stands of others if their view is directed low enough. The players don't see each other

Copy link
Member

Choose a reason for hiding this comment

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

So the first armor stand doesn't get replaced by the second one when the same location is used?

Copy link
Author

Choose a reason for hiding this comment

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

No, it won't be replaced


stand.setGravity(false);
stand.setAI(false);
stand.setInvisible(true);

return stand;
}

}
16 changes: 16 additions & 0 deletions src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,22 @@ public final class RestrictionSettings implements SettingsHolder {
public static final Property<Set<String>> UNRESTRICTED_INVENTORIES =
newLowercaseStringSetProperty("settings.unrestrictions.UnrestrictedInventories");

@Comment({
"While in unregistered/logged out state, should players be set to spectator mode and",
"forced to spectate within a spawn point invisible armor stand, for 0 movement and head",
"pitch + yaw? may be more effective than 'allowMovement' at locking the player in place."
})
public static final Property<Boolean> SPECTATE_STAND_LOGIN =
newProperty("settings.restrictions.spectateStandLogin", true);
mrchefran marked this conversation as resolved.
Show resolved Hide resolved

@Comment("Head Yaw position for 'spectateStandLogin'.")
public static final Property<Double> HEAD_YAW =
newProperty("settings.restrictions.headYaw", 0.0f);

@Comment("Head Pitch position for 'spectateStandLogin'.")
public static final Property<Double> HEAD_PITCH =
newProperty("settings.restrictions.headPitch", 0.0f);
Copy link
Member

@ljacqu ljacqu Jun 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to put the new properties under a dedicated path? In the sense of settings.restrictions.spectateStandLogin.pitch (or .headPitch). With that naming, settings.restrictions.spectateStandLogin would have to become something like "settings.restrictions.spectateStandLogin.enabled"

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it would. It looks much better this way. I put the properties in a separate path


private RestrictionSettings() {
}

Expand Down