SpectateLogin implementation #2552

mrchefran · 2022-06-03T20:20:10Z

Implementation of the idea proposed in #2503

Feature description: puts the player in an invisible armorstand for the time of authorization and fixes the direction of the view. After authorization is completed, the stand disappears

sgdc3 · 2022-06-03T20:40:57Z

Looks cool! I'll take a look asap, in the meantime @ljacqu @games647 @hex3l what do you think about this concept+implementation?

sgdc3 · 2022-06-04T00:05:36Z

src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java

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


What about listening at the player spawn/respawn event?

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

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

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

ljacqu

Code looks great and you've found a great way to incorporate the changes into the existing codebase! Some unit tests would be nice :)

src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java

ljacqu · 2022-06-04T19:28:06Z

src/main/java/fr/xephi/authme/service/SpectateLoginService.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class SpectateLoginService {


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

Added a description of the class behavior

ljacqu · 2022-06-04T19:29:00Z

src/main/java/fr/xephi/authme/service/SpectateLoginService.java

+    }
+
+    public boolean hasStand(Player player) {
+        return armorStands.get(player) != null;


can be armorStands.containsKey(player)

Changed to it

ljacqu · 2022-06-04T19:33:47Z

src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java

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


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"

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

ljacqu · 2022-06-04T19:37:42Z

src/main/java/fr/xephi/authme/listener/PlayerListener.java

+            && event.getCause() == PlayerTeleportEvent.TeleportCause.SPECTATE
+            && event.getPlayer().getGameMode() == GameMode.SPECTATOR
+            && (settings.getProperty(RestrictionSettings.SPECTATE_STAND_LOGIN)
+            || spectateLoginService.hasStand(event.getPlayer()))) {


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

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

ljacqu · 2022-06-04T19:39:19Z

src/main/java/fr/xephi/authme/service/SpectateLoginService.java

+        Location location = new Location(loc.getWorld(), loc.getX(), loc.getY(), loc.getBlockZ(),
+            (float) yaw, (float) pitch);
+
+        ArmorStand stand = location.getWorld().spawn(location, ArmorStand.class);


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

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

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

No, it won't be replaced

ljacqu · 2022-06-04T19:40:15Z

src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java

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


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

ljacqu · 2022-06-04T19:46:41Z

src/main/java/fr/xephi/authme/service/SpectateLoginService.java

+        ArmorStand stand = spawnStand(location);
+
+        armorStands.put(player, stand);
+        gameModeMap.put(player, player.getGameMode());


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

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?

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

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

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

What about adding a "limboArmorStandUUID" field to LimboPlayer?

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

@mrchefran yes, exactly

…eReloaded into stand-spectating

BuildTools added 2 commits June 2, 2022 14:13

Add SpectateLogin implementation

8692637

Change according to the style guide

e51fbdf

sgdc3 requested review from ljacqu, hex3l, sgdc3 and games647 June 4, 2022 00:02

sgdc3 requested changes Jun 4, 2022

View reviewed changes

ljacqu reviewed Jun 4, 2022

View reviewed changes

BuildTools and others added 5 commits June 5, 2022 13:44

Code improvements

b9bb7bd

Update pom.xml

208790e

Fix some Checkstyle issues

6de4a9c

Merge branch 'stand-spectating' of https://github.com/mrchefran/AuthM…

37dd1b0

…eReloaded into stand-spectating

Remove stands on plugin shutdown

7cd7bdb

mrchefran requested a review from sgdc3 June 5, 2022 21:31

This was referenced Aug 31, 2023

Review Spectator Pull Request #2744

Closed

SpectateLogin implementation #2745

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpectateLogin implementation #2552

SpectateLogin implementation #2552

mrchefran commented Jun 3, 2022

sgdc3 commented Jun 3, 2022

sgdc3 Jun 4, 2022

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

mrchefran Jun 4, 2022

ljacqu left a comment

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

ljacqu Jun 4, 2022 •

edited

mrchefran Jun 4, 2022

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

ljacqu Jun 6, 2022

mrchefran Jun 6, 2022

ljacqu Jun 4, 2022

ljacqu Jun 4, 2022

mrchefran Jun 4, 2022

mrchefran Jun 5, 2022

ljacqu Jun 6, 2022

mrchefran Jun 6, 2022

sgdc3 Jun 12, 2022

mrchefran Jun 17, 2022

sgdc3 Jul 8, 2022

SpectateLogin implementation #2552

Are you sure you want to change the base?

SpectateLogin implementation #2552

Conversation

mrchefran commented Jun 3, 2022

sgdc3 commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacqu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacqu Jun 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacqu Jun 4, 2022 •

edited