[Most Important Discussion Ever] Should you store a Player?

Discussion in 'Spigot Plugin Development' started by Bladian, Jun 17, 2015.

  1. Code (Text):
    package me.uhc.spectate;

    import org.bukkit.GameMode;
    import org.bukkit.Location;
    import org.bukkit.Material;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.block.Action;
    import org.bukkit.event.player.PlayerGameModeChangeEvent;
    import org.bukkit.event.player.PlayerInteractEvent;

    import java.util.ArrayList;
    import java.util.HashMap;
    import java.util.List;

    /**
    * Created by maxmigliorini on 17/06/15.
    */
    public class EventSpectate implements Listener {

        public List<Player> getSpectate() {
            return spectate;
        }

        private List<Player> spectate = new ArrayList<Player>();

        private String logo = "§7[§9Galaxy§eUHC§7]§e";

        @EventHandler
        public void onClick(PlayerInteractEvent e) {

            MethodSpectate methodSpectate = new MethodSpectate();

            Player p = e.getPlayer();

            for(Player array : spectate) {
                if (array == p) {
                    if (e.getAction() == Action.RIGHT_CLICK_AIR || e.getAction() == Action.RIGHT_CLICK_BLOCK) {
                        if (e.getItem().getType() == Material.SKULL_ITEM) {
                            if (e.getItem().getItemMeta().getDisplayName() != null) {
                                if (e.getItem().getItemMeta().getDisplayName().equalsIgnoreCase("§e§lPLAYERS")) {

                                    methodSpectate.createSpectate(p);
                                }
                            }
                        }
                    }
                    if (e.getItem().getType() == Material.WATCH) {
                        if (e.getItem().getItemMeta().getDisplayName() != null) {
                            if (e.getItem().getItemMeta().getDisplayName().equalsIgnoreCase("§e§lMAP TELEPORTATION")) {
                                Location location = new Location(p.getWorld(), 0, p.getWorld().getHighestBlockYAt(0, 0) + 20, 0);
                                p.teleport(location);
                                p.sendMessage(logo + "You have been teleported to the §9center §eof the map");

                            }
                        }
                    }
                }
            }
        }

        @EventHandler
        public void onGameMode(PlayerGameModeChangeEvent e) {

            Player p = e.getPlayer();

            if(spectate.contains(p)) {
                p.sendMessage(logo + "You cannot change gamemode when you are in §9spectate mode");
            }
        }
    }
     
    I currently have this code which I'm using for UHC.

    package me.uhc.spectate;

    import org.bukkit.GameMode;
    import org.bukkit.Location;
    import org.bukkit.Material;
    import org.bukkit.entity.Player;
    import org.bukkit.event.EventHandler;
    import org.bukkit.event.Listener;
    import org.bukkit.event.block.Action;
    import org.bukkit.event.player.PlayerGameModeChangeEvent;
    import org.bukkit.event.player.PlayerInteractEvent;

    import java.util.ArrayList;
    import java.util.HashMap;
    import java.util.List;

    /** * Created by maxmigliorini on 17/06/15. */public class EventSpectate implements Listener {

    public List<Player> getSpectate() {
    return spectate;
    }

    private List<Player> spectate = new ArrayList<Player>();

    private String logo = "§7[§9Galaxy§eUHC§7]§e";

    @EventHandlerpublic void onClick(PlayerInteractEvent e) {

    MethodSpectate methodSpectate = new MethodSpectate();

    Player p = e.getPlayer();

    for(Player array : spectate) {
    if (array == p) {
    if (e.getAction() == Action.RIGHT_CLICK_AIR || e.getAction() == Action.RIGHT_CLICK_BLOCK) {
    if (e.getItem().getType() == Material.SKULL_ITEM) {
    if (e.getItem().getItemMeta().getDisplayName() != null) {
    if (e.getItem().getItemMeta().getDisplayName().equalsIgnoreCase("§e§lPLAYERS")) {

    methodSpectate.createSpectate(p);
    }
    }
    }
    }
    if (e.getItem().getType() == Material.WATCH) {
    if (e.getItem().getItemMeta().getDisplayName() != null) {
    if (e.getItem().getItemMeta().getDisplayName().equalsIgnoreCase("§e§lMAP TELEPORTATION")) {
    Location location = new Location(p.getWorld(), 0, p.getWorld().getHighestBlockYAt(0, 0) + 20, 0);
    p.teleport(location);
    p.sendMessage(logo + "You have been teleported to the §9center §eof the map");

    }
    }
    }
    }
    }
    }

    @EventHandlerpublic void onGameMode(PlayerGameModeChangeEvent e) {

    Player p = e.getPlayer();

    if(spectate.contains(p)) {
    p.sendMessage(logo + "You cannot change gamemode when you are in §9spectate mode");
    }
    }
    }

    and

    Code (Text):
    package me.uhc.spectate;

    import org.bukkit.GameMode;
    import org.bukkit.command.Command;
    import org.bukkit.command.CommandExecutor;
    import org.bukkit.command.CommandSender;
    import org.bukkit.entity.Player;

    /**
    * Created by maxmigliorini on 17/06/15.
    */
    public class CommandSpectate implements CommandExecutor {

        @Override
        public boolean onCommand(CommandSender commandSender, Command command, String s, String[] strings) {

            if (!(commandSender instanceof Player)) {
                commandSender.sendMessage("Only a player may execute this command");

            } else {

                Player p = (Player) commandSender;

                if (command.getName().equalsIgnoreCase("spectate")) {

                    if (p.hasPermission("galaxy.spectate")) {

                        if (strings.length == 0) {

                            EventSpectate eventSpectate = new EventSpectate();

                            eventSpectate.getSpectate().add(p);

                            MethodSpectate methodSpectate = new MethodSpectate();
                            methodSpectate.spectateMode(p);

                            p.hidePlayer(p);
                            p.setGameMode(GameMode.CREATIVE);

                            p.sendMessage(String.valueOf(eventSpectate.getSpectate().get(0)));

                        }
                    }
                }

            }
            return false;
        }
    }
     
    The problem is when I check if the arraylist contains an my player, it doesn't do anything, even though the list says it's there.

    I also tried.

    if(spectate.contain(p)) {

    but no use.

    Any help?
     
  2. You are creating a new instance of MethodSpectate, stop doing that and define one as your instance, that instance pass it around your classess
     
    • Like Like x 1
  3. ^ this.

    Code (Text):
    MethodSpectate methodSpectate = new MethodSpectate();
    is not needed, and if anything, will break things.
     
  4. Never save players in a arraylist use uuids/playernames instead
     
    • Funny Funny x 2
    • Agree Agree x 1
  5. Storing player instances is recommended and there isn't any problem with this (it's even better).
    You must ofc remove the instance from the Collection/Map if the player quits, otherwise you've got a memory leak (which is also the case if you'd use names/uuids).

    Storing uuids is only necessary, if the player might log out while you store his name.
     
    • Agree Agree x 2
  6. Okay so instead of making a new instance I should return the instance that is already initialised.

    In addition I can also make the array list static and do this

    MethodSpectate.getSpectate().

    Correct me if I'm wrong please.
     
  7. There's absolutely no need to use static unless you're writing an api.

    You should be initializing the class and, referencing it in your main class. So, You can use:
    Code (Text):
    Player spectator = plugin.getSpectateClass().getSpectator(UUID uuid);
    Player spectator = plugin.getSpectateClass().getSpectatorAtIndex(int arrayIndex);
    if (spectator != null) {
    //TODO
    }
    or:
    plugin.getSpectateClass().setSpectator(UUID uuid);
     
    Example of your main class:
    Code (Text):

    ClassSpectator sClass = new ClassSpectator();

    public ClassSpectator getClassSpectator() {
    return this.sClass;
    }
     
    #7 MidnightShadow, Jun 18, 2015
    Last edited: Jun 18, 2015
  8. If I recall, storing Player objects can cause memory leaks, unless Spigot fixed that.
     
  9. Please organize the code. I can't understand the thing.
    You try to compare players using ==, but you can't do that since its not a primitive type (int, boolean, float, etc.). Use p.getUniqueId().equals(other.getUniqueId()) instead, since (I think) Player class doesn't override equals, also uuids are easier.
     
    #9 NonameSL, Jun 18, 2015
    Last edited: Jun 18, 2015
  10. Its a common misconception due to an old Bukkit developer giving bad advice. It is perfectly fine to store the Player object in memory.
     
    • Informative Informative x 2
    • Friendly Friendly x 1
  11. +1. Creating a list of uuids is only necessary when the players aren't always onnline.
     
  12. There's been a long discussion thread about this a while ago, and the conclusion was that as long as you remove players from the storage when they log off (good practice anyways), you'll have no problems with memory leaks.
     
    • Informative Informative x 1
  13. Thread link? I like to read :p
     
    • Optimistic Optimistic x 2
    • Funny Funny x 1
    • Winner Winner x 1
  14. Let me just change the title...
     
  15. sothatsit

    Patron

    There is nothing wrong with the storage of Player. Storing UUID/Name/Player can all cause memory leaks, Player memory leaks are just more noticeable as more information is stored in Player objects than in UUID or String. As long as you remove all the references to the Player when they log out you should be fine, i usually just like to use UUID out of habit though.
     
  16. It does override equals (check the source code of the server implementation before making assumptions ;))