Cleanup to the /player command (#1690)

* Small cleanup to `/player` command

There should be no changes to users

* Use extracted `source` variable

* Inline another single-use small function

* Inline another trivial helper

* Import inner classes of the ActionPack

* Rename `literalargumentbuilder` variable

* Compact unnecessary three lines into one

* Inline `source.getServer` and pass `Vec3` to `createFake`

This gets outside of "just" the command, but it's fine

* Unpick name length constant

* Pass `Vec3` directly to `BlockPos.containing`

src/main/java/carpet/commands/PlayerCommand.java

@@ -1,6 +1,8 @@
 package carpet.commands;
 
 import carpet.helpers.EntityPlayerActionPack;
+import carpet.helpers.EntityPlayerActionPack.Action;
+import carpet.helpers.EntityPlayerActionPack.ActionType;
 import carpet.CarpetSettings;
 import carpet.fakes.ServerPlayerInterface;
 import carpet.patches.EntityPlayerMPFake;
@@ -17,6 +19,7 @@ import com.mojang.brigadier.exceptions.CommandSyntaxException;
 import net.minecraft.commands.CommandBuildContext;
 import net.minecraft.commands.CommandSourceStack;
 import net.minecraft.commands.arguments.DimensionArgument;
+import net.minecraft.commands.arguments.GameModeArgument;
 import net.minecraft.commands.arguments.coordinates.RotationArgument;
 import net.minecraft.commands.arguments.coordinates.Vec3Argument;
 import net.minecraft.core.BlockPos;
@@ -31,7 +34,6 @@ import net.minecraft.world.level.GameType;
 import net.minecraft.world.level.Level;
 import net.minecraft.world.phys.Vec2;
 import net.minecraft.world.phys.Vec3;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -44,26 +46,22 @@ import static net.minecraft.commands.SharedSuggestionProvider.suggest;
 
 public class PlayerCommand
 {
-
     // TODO: allow any order like execute
     public static void register(CommandDispatcher<CommandSourceStack> dispatcher, CommandBuildContext commandBuildContext)
     {
-        final String[] gamemodeStrings = Arrays.stream(GameType.values())
+        LiteralArgumentBuilder<CommandSourceStack> command = literal("player")
-                .map(GameType::getName)
-                .toArray(String[]::new);
-        LiteralArgumentBuilder<CommandSourceStack> literalargumentbuilder = literal("player")
                 .requires((player) -> CommandHelper.canUseCommand(player, CarpetSettings.commandPlayer))
                 .then(argument("player", StringArgumentType.word())
-                        .suggests( (c, b) -> suggest(getPlayers(c.getSource()), b))
+                        .suggests((c, b) -> suggest(getPlayerSuggestions(c.getSource()), b))
-                        .then(literal("stop").executes(PlayerCommand::stop))
+                        .then(literal("stop").executes(manipulation(EntityPlayerActionPack::stopAll)))
-                        .then(makeActionCommand("use", EntityPlayerActionPack.ActionType.USE))
+                        .then(makeActionCommand("use", ActionType.USE))
-                        .then(makeActionCommand("jump", EntityPlayerActionPack.ActionType.JUMP))
+                        .then(makeActionCommand("jump", ActionType.JUMP))
-                        .then(makeActionCommand("attack", EntityPlayerActionPack.ActionType.ATTACK))
+                        .then(makeActionCommand("attack", ActionType.ATTACK))
-                        .then(makeActionCommand("drop", EntityPlayerActionPack.ActionType.DROP_ITEM))
+                        .then(makeActionCommand("drop", ActionType.DROP_ITEM))
                         .then(makeDropCommand("drop", false))
-                        .then(makeActionCommand("dropStack", EntityPlayerActionPack.ActionType.DROP_STACK))
+                        .then(makeActionCommand("dropStack", ActionType.DROP_STACK))
                         .then(makeDropCommand("dropStack", true))
-                        .then(makeActionCommand("swapHands", EntityPlayerActionPack.ActionType.SWAP_HANDS))
+                        .then(makeActionCommand("swapHands", ActionType.SWAP_HANDS))
                         .then(literal("hotbar")
                                 .then(argument("slot", IntegerArgumentType.integer(1, 9))
                                         .executes(c -> manipulate(c, ap -> ap.setSlot(IntegerArgumentType.getInteger(c, "slot"))))))
@@ -83,63 +81,60 @@ public class PlayerCommand
                                 .then(literal("west").executes(manipulation(ap -> ap.look(Direction.WEST))))
                                 .then(literal("up").executes(manipulation(ap -> ap.look(Direction.UP))))
                                 .then(literal("down").executes(manipulation(ap -> ap.look(Direction.DOWN))))
-                                .then(literal("at").then(argument("position", Vec3Argument.vec3()).executes(PlayerCommand::lookAt)))
+                                .then(literal("at").then(argument("position", Vec3Argument.vec3())
+                                        .executes(c -> manipulate(c, ap -> ap.lookAt(Vec3Argument.getVec3(c, "position"))))))
                                 .then(argument("direction", RotationArgument.rotation())
                                         .executes(c -> manipulate(c, ap -> ap.look(RotationArgument.getRotation(c, "direction").getRotation(c.getSource())))))
                         ).then(literal("turn")
-                                .then(literal("left").executes(c -> manipulate(c, ap -> ap.turn(-90, 0))))
+                                .then(literal("left").executes(manipulation(ap -> ap.turn(-90, 0))))
-                                .then(literal("right").executes(c -> manipulate(c, ap -> ap.turn(90, 0))))
+                                .then(literal("right").executes(manipulation(ap -> ap.turn(90, 0))))
-                                .then(literal("back").executes(c -> manipulate(c, ap -> ap.turn(180, 0))))
+                                .then(literal("back").executes(manipulation(ap -> ap.turn(180, 0))))
                                 .then(argument("rotation", RotationArgument.rotation())
                                         .executes(c -> manipulate(c, ap -> ap.turn(RotationArgument.getRotation(c, "rotation").getRotation(c.getSource())))))
-                        ).then(literal("move").executes(c -> manipulate(c, EntityPlayerActionPack::stopMovement))
+                        ).then(literal("move").executes(manipulation(EntityPlayerActionPack::stopMovement))
-                                .then(literal("forward").executes(c -> manipulate(c, ap -> ap.setForward(1))))
+                                .then(literal("forward").executes(manipulation(ap -> ap.setForward(1))))
-                                .then(literal("backward").executes(c -> manipulate(c, ap -> ap.setForward(-1))))
+                                .then(literal("backward").executes(manipulation(ap -> ap.setForward(-1))))
-                                .then(literal("left").executes(c -> manipulate(c, ap -> ap.setStrafing(1))))
+                                .then(literal("left").executes(manipulation(ap -> ap.setStrafing(1))))
-                                .then(literal("right").executes(c -> manipulate(c, ap -> ap.setStrafing(-1))))
+                                .then(literal("right").executes(manipulation(ap -> ap.setStrafing(-1))))
                         ).then(literal("spawn").executes(PlayerCommand::spawn)
                                 .then(literal("in").requires((player) -> player.hasPermission(2))
-                                        .then(argument("gamemode", StringArgumentType.word())
+                                        .then(argument("gamemode", GameModeArgument.gameMode())
-                                                .suggests( (c, b) -> suggest(gamemodeStrings, b))
                                         .executes(PlayerCommand::spawn)))
                                 .then(literal("at").then(argument("position", Vec3Argument.vec3()).executes(PlayerCommand::spawn)
                                         .then(literal("facing").then(argument("direction", RotationArgument.rotation()).executes(PlayerCommand::spawn)
                                                 .then(literal("in").then(argument("dimension", DimensionArgument.dimension()).executes(PlayerCommand::spawn)
                                                         .then(literal("in").requires((player) -> player.hasPermission(2))
-                                                                .then(argument("gamemode", StringArgumentType.word()).suggests( (c, b) -> suggest(gamemodeStrings, b))
+                                                                .then(argument("gamemode", GameModeArgument.gameMode())
                                                                 .executes(PlayerCommand::spawn)
                                                         )))
                                         )))
                                 ))
                         )
                 );
-        dispatcher.register(literalargumentbuilder);
+        dispatcher.register(command);
     }
 
-    private static LiteralArgumentBuilder<CommandSourceStack> makeActionCommand(String actionName, EntityPlayerActionPack.ActionType type)
+    private static LiteralArgumentBuilder<CommandSourceStack> makeActionCommand(String actionName, ActionType type)
     {
         return literal(actionName)
-                .executes(c -> action(c, type, EntityPlayerActionPack.Action.once()))
+                .executes(manipulation(ap -> ap.start(type, Action.once())))
-                .then(literal("once").executes(c -> action(c, type, EntityPlayerActionPack.Action.once())))
+                .then(literal("once").executes(manipulation(ap -> ap.start(type, Action.once()))))
-                .then(literal("continuous").executes(c -> action(c, type, EntityPlayerActionPack.Action.continuous())))
+                .then(literal("continuous").executes(manipulation(ap -> ap.start(type, Action.continuous()))))
                 .then(literal("interval").then(argument("ticks", IntegerArgumentType.integer(1))
-                        .executes(c -> action(c, type, EntityPlayerActionPack.Action.interval(IntegerArgumentType.getInteger(c, "ticks"))))));
+                        .executes(c -> manipulate(c, ap -> ap.start(type, Action.interval(IntegerArgumentType.getInteger(c, "ticks")))))));
     }
 
     private static LiteralArgumentBuilder<CommandSourceStack> makeDropCommand(String actionName, boolean dropAll)
     {
         return literal(actionName)
-                .then(literal("all").executes(c ->manipulate(c, ap -> ap.drop(-2,dropAll))))
+                .then(literal("all").executes(manipulation(ap -> ap.drop(-2, dropAll))))
-                .then(literal("mainhand").executes(c ->manipulate(c, ap -> ap.drop(-1,dropAll))))
+                .then(literal("mainhand").executes(manipulation(ap -> ap.drop(-1, dropAll))))
-                .then(literal("offhand").executes(c ->manipulate(c, ap -> ap.drop(40,dropAll))))
+                .then(literal("offhand").executes(manipulation(ap -> ap.drop(40, dropAll))))
                 .then(argument("slot", IntegerArgumentType.integer(0, 40)).
-                        executes(c ->manipulate(c, ap -> ap.drop(
+                        executes(c -> manipulate(c, ap -> ap.drop(IntegerArgumentType.getInteger(c, "slot"), dropAll))));
-                                IntegerArgumentType.getInteger(c,"slot"),
-                                dropAll
-                        ))));
     }
 
-    private static Collection<String> getPlayers(CommandSourceStack source)
+    private static Collection<String> getPlayerSuggestions(CommandSourceStack source)
     {
         Set<String> players = new LinkedHashSet<>(List.of("Steve", "Alex"));
         players.addAll(source.getOnlinePlayerNames());
@@ -156,26 +151,23 @@ public class PlayerCommand
     private static boolean cantManipulate(CommandContext<CommandSourceStack> context)
     {
         Player player = getPlayer(context);
+        CommandSourceStack source = context.getSource();
         if (player == null)
         {
-            Messenger.m(context.getSource(), "r Can only manipulate existing players");
+            Messenger.m(source, "r Can only manipulate existing players");
             return true;
         }
-        Player sendingPlayer;
+        Player sender = source.getPlayer();
-        try
+        if (sender == null)
-        {
-            sendingPlayer = context.getSource().getPlayerOrException();
-        }
-        catch (CommandSyntaxException e)
         {
             return false;
         }
 
-        if (!context.getSource().getServer().getPlayerList().isOp(sendingPlayer.getGameProfile()))
+        if (!source.getServer().getPlayerList().isOp(sender.getGameProfile()))
         {
-            if (sendingPlayer != player && !(player instanceof EntityPlayerMPFake))
+            if (sender != player && !(player instanceof EntityPlayerMPFake))
             {
-                Messenger.m(context.getSource(), "r Non OP players can't control other real players");
+                Messenger.m(source, "r Non OP players can't control other real players");
                 return true;
             }
         }
@@ -196,8 +188,8 @@ public class PlayerCommand
         String playerName = StringArgumentType.getString(context, "player");
         MinecraftServer server = context.getSource().getServer();
         PlayerList manager = server.getPlayerList();
-        Player player = manager.getPlayerByName(playerName);
+
-        if (player != null)
+        if (manager.getPlayerByName(playerName) != null)
         {
             Messenger.m(context.getSource(), "r Player ", "rb " + playerName, "r  is already logged on");
             return true;
@@ -234,109 +226,86 @@ public class PlayerCommand
         return 1;
     }
 
-    private static int lookAt(CommandContext<CommandSourceStack> context)
-    {
-        return manipulate(context, ap -> {
-            //try {
-                ap.lookAt(Vec3Argument.getVec3(context, "position"));
-            //} catch (CommandSyntaxException ignored) {}
-        });
-    }
-
     @FunctionalInterface
-    interface SupplierWithCommandSyntaxException<T>
+    interface SupplierWithCSE<T>
     {
         T get() throws CommandSyntaxException;
     }
 
-    private static <T> T tryGetArg(SupplierWithCommandSyntaxException<T> a, SupplierWithCommandSyntaxException<T> b) throws CommandSyntaxException
+    private static <T> T getArgOrDefault(SupplierWithCSE<T> getter, T defaultValue) throws CommandSyntaxException
     {
         try
         {
-            return a.get();
+            return getter.get();
         }
         catch (IllegalArgumentException e)
         {
-            return b.get();
+            return defaultValue;
         }
     }
 
     private static int spawn(CommandContext<CommandSourceStack> context) throws CommandSyntaxException
     {
         if (cantSpawn(context)) return 0;
+
         CommandSourceStack source = context.getSource();
-        Vec3 pos = tryGetArg(
+        Vec3 pos = getArgOrDefault(
                 () -> Vec3Argument.getVec3(context, "position"),
-                source::getPosition
+                source.getPosition()
         );
-        Vec2 facing = tryGetArg(
+        Vec2 facing = getArgOrDefault(
-                () -> RotationArgument.getRotation(context, "direction").getRotation(context.getSource()),
+                () -> RotationArgument.getRotation(context, "direction").getRotation(source),
-                source::getRotation
+                source.getRotation()
         );
-        ResourceKey<Level> dimType = tryGetArg(
+        ResourceKey<Level> dimType = getArgOrDefault(
                 () -> DimensionArgument.getDimension(context, "dimension").dimension(),
-                () -> source.getLevel().dimension() // dimension.getType()
+                source.getLevel().dimension()
         );
         GameType mode = GameType.CREATIVE;
         boolean flying = false;
-        try
+        if (source.getEntity() instanceof ServerPlayer sender)
         {
-            ServerPlayer player = context.getSource().getPlayerOrException();
+            mode = sender.gameMode.getGameModeForPlayer();
-            mode = player.gameMode.getGameModeForPlayer();
+            flying = sender.getAbilities().flying;
-            flying = player.getAbilities().flying;
         }
-        catch (CommandSyntaxException ignored) {}
         try {
-            String opGameMode = StringArgumentType.getString(context, "gamemode");
+            mode = GameModeArgument.getGameMode(context, "gamemode");
-            mode = GameType.byName(opGameMode, null);
+        } catch (IllegalArgumentException notPresent) {}
-            if(mode == null)
+
-            {
+        if (mode == GameType.SPECTATOR)
-                Messenger.m(context.getSource(), "rb Invalid game mode: "+opGameMode+".");
-                return 0;
-            }
-        } catch (IllegalArgumentException ignored) {}
-        if(mode == GameType.SPECTATOR)
         {
             // Force override flying to true for spectator players, or they will fell out of the world.
             flying = true;
-        } else if(mode.isSurvival()){
+        } else if (mode.isSurvival())
+        {
             // Force override flying to false for survival-like players, or they will fly too
             flying = false;
         }
         String playerName = StringArgumentType.getString(context, "player");
-        if (playerName.length()>maxPlayerLength(source.getServer()))
+        if (playerName.length() > maxNameLength(source.getServer()))
         {
-            Messenger.m(context.getSource(), "rb Player name: "+playerName+" is too long");
+            Messenger.m(source, "rb Player name: " + playerName + " is too long");
             return 0;
         }
 
-        MinecraftServer server = source.getServer();
+        if (!Level.isInSpawnableBounds(BlockPos.containing(pos)))
-        if (!Level.isInSpawnableBounds(BlockPos.containing(pos.x, pos.y, pos.z)))
         {
-            Messenger.m(context.getSource(), "rb Player "+playerName+" cannot be placed outside of the world");
+            Messenger.m(source, "rb Player " + playerName + " cannot be placed outside of the world");
             return 0;
         }
-        Player player = EntityPlayerMPFake.createFake(playerName, server, pos.x, pos.y, pos.z, facing.y, facing.x, dimType, mode, flying);
+        Player player = EntityPlayerMPFake.createFake(playerName, source.getServer(), pos, facing.y, facing.x, dimType, mode, flying);
         if (player == null)
         {
-            Messenger.m(context.getSource(), "rb Player " + StringArgumentType.getString(context, "player") + " doesn't exist " +
+            Messenger.m(source, "rb Player " + playerName + " doesn't exist and cannot spawn in online mode. " +
-                    "and cannot spawn in online mode. Turn the server offline to spawn non-existing players");
+                    "Turn the server offline to spawn non-existing players");
             return 0;
         }
         return 1;
     }
 
-    private static int maxPlayerLength(MinecraftServer server)
+    private static int maxNameLength(MinecraftServer server)
     {
-        return server.getPort() >= 0 ? 16 : 40;
+        return server.getPort() >= 0 ? Player.MAX_NAME_LENGTH : 40;
-    }
-
-    private static int stop(CommandContext<CommandSourceStack> context)
-    {
-        if (cantManipulate(context)) return 0;
-        ServerPlayer player = getPlayer(context);
-        ((ServerPlayerInterface) player).getActionPack().stopAll();
-        return 1;
     }
 
     private static int manipulate(CommandContext<CommandSourceStack> context, Consumer<EntityPlayerActionPack> action)
@@ -352,13 +321,10 @@ public class PlayerCommand
         return c -> manipulate(c, action);
     }
 
-    private static int action(CommandContext<CommandSourceStack> context, EntityPlayerActionPack.ActionType type, EntityPlayerActionPack.Action action)
-    {
-        return manipulate(context, ap -> ap.start(type, action));
-    }
-
     private static int shadow(CommandContext<CommandSourceStack> context)
     {
+        if (cantManipulate(context)) return 0;
+
         ServerPlayer player = getPlayer(context);
         if (player instanceof EntityPlayerMPFake)
         {
@@ -369,14 +335,7 @@ public class PlayerCommand
             Messenger.m(context.getSource(), "r Cannot shadow single-player server owner");
             return 0;
         }
-        ServerPlayer sendingPlayer = null;
-        try
-        {
-            sendingPlayer = context.getSource().getPlayerOrException();
-        }
-        catch (CommandSyntaxException ignored) { }
 
-        if (sendingPlayer!=player && cantManipulate(context)) return 0;
         EntityPlayerMPFake.createShadow(player.server, player);
         return 1;
     }

src/main/java/carpet/patches/EntityPlayerMPFake.java

@@ -26,6 +26,7 @@ import net.minecraft.world.level.GameType;
 import net.minecraft.world.level.Level;
 import net.minecraft.world.level.block.entity.SkullBlockEntity;
 import net.minecraft.world.level.block.state.BlockState;
+import net.minecraft.world.phys.Vec3;
 import carpet.fakes.ServerPlayerInterface;
 import carpet.utils.Messenger;
 
@@ -37,7 +38,7 @@ public class EntityPlayerMPFake extends ServerPlayer
     public Runnable fixStartingPosition = () -> {};
     public boolean isAShadow;
 
-    public static EntityPlayerMPFake createFake(String username, MinecraftServer server, double d0, double d1, double d2, double yaw, double pitch, ResourceKey<Level> dimensionId, GameType gamemode, boolean flying)
+    public static EntityPlayerMPFake createFake(String username, MinecraftServer server, Vec3 pos, double yaw, double pitch, ResourceKey<Level> dimensionId, GameType gamemode, boolean flying)
     {
         //prolly half of that crap is not necessary, but it works
         ServerLevel worldIn = server.getLevel(dimensionId);
@@ -65,9 +66,9 @@ public class EntityPlayerMPFake extends ServerPlayer
             gameprofile = result.get();
         }
         EntityPlayerMPFake instance = new EntityPlayerMPFake(server, worldIn, gameprofile, false);
-        instance.fixStartingPosition = () -> instance.moveTo(d0, d1, d2, (float) yaw, (float) pitch);
+        instance.fixStartingPosition = () -> instance.moveTo(pos.x, pos.y, pos.z, (float) yaw, (float) pitch);
         server.getPlayerList().placeNewPlayer(new FakeClientConnection(PacketFlow.SERVERBOUND), instance);
-        instance.teleportTo(worldIn, d0, d1, d2, (float)yaw, (float)pitch);
+        instance.teleportTo(worldIn, pos.x, pos.y, pos.z, (float) yaw, (float) pitch);
         instance.setHealth(20.0F);
         instance.unsetRemoved();
         instance.setMaxUpStep(0.6F);