Commands, and why you shouldn't just return if != instanceof Player

Discussion in 'Plugin Development' started by Dinnerbone, Feb 2, 2011.

Thread Status:
Not open for further replies.
  1. Offline

    Dinnerbone Bukkit Team Member

    With the new introduction of onCommand being the central place of all methods of commands, I've seen some plugins just instantly check if it's a player and return without bothering to process it if not. This isn't very nice to the user, it's not very nice to other plugins, and it degrades the overall quality of the plugin. This should be done depending on the command they enter and its usage.

    Let's say a plugin defined the command "health", which checks and tells you the health of a given player. If you don't pass a player as an argument, it uses the player who typed the command. Simple, right? What if I wanted to use this from IRC using a plugin that interfaces irc->game. Naturally I'd have to give a player as an argument, but that should certainly still work.

    The plugin should check; "they didn't define a player, *now* let's see if they're a player or not". It shouldn't pre-emptively say no just because it came from somewhere else.


    That is all.
     
    ferrybig and rmsy like this.
  2. Offline

    NathanWolf

    Great note!

    It's so easy to "just get it working" with the new command system and stop there, but it only takes a little bit more effort, in most cases, to really take full advantage of this great new system- and it's so worth it!

    I know, for me, I've got a plugin where the player in the command event was really only used to send result/status messages to- the commands the player is performing are more "system level", and don't have to do with the player itself.

    So, this really works out well in that case! A little bit of refactoring to use a CommandSender instead of player, and suddenly all my commands work as server commands too- it's pretty freakin great! I can talk to my plugin now without ever even having to login to Minecraft...
     
  3. Offline

    Firestar

    why not just spoof the player class by making a fake player, and have getName return console, and send message be sent to console, that would make this entire discussion irrelevant.
     
  4. Offline

    Dinnerbone Bukkit Team Member

    Because it's not a player, performing actions on it like "teleport" will just make no sense and we'd have to make a method on player called "isReallyAPlayer"... Not a good design structure.
     
  5. Offline

    NathanWolf

    The instanceof operator really is a nice, Java way to handle just this sort of thing- it lets you use CommandSender directly if you don't need a player, and cast it down when you do- no need for hokey isPlayer() type functions- it's built in to the language :)
    --- merged: Feb 2, 2011 6:12 PM ---
    Dinnerbone, random comment- It's super awesome to be able to use console commands in the server console, but it seems like output is wrapped to an arbitrary column limit (the same width as the in-game console, maybe?). This is probably a known thing, but I thought I'd mention it.
     
  6. Offline

    Dinnerbone Bukkit Team Member

    Yeah we're working on it
     
  7. Offline

    Firestar

    so isplayer couldn't exist with a fake player?
     
  8. Offline

    NathanWolf

    Thanks very much!
     
  9. Offline

    Dinnerbone Bukkit Team Member

    It could. But isPlayer is the biggest sign of a flawed architecture. A console isn't a player, why should it extend Player?
     
  10. Offline

    Firestar

    for compatibility, otherwise plugins would need just a bit more code, (more code per plugins, larger size, more checks greater resource usage per plugin)

    also do not understand why you make every command usable through console, not every command includes "target player"
     
  11. Offline

    NathanWolf

    I think it makes total sense- instanceof is built into Java, why not use it?

    As an example case, I have one plugin that is mainly admin-oriented. None of its commands require a player- converting this one took a bit of time (ok, mostly search+replace) but now it's capable of being used from the server console, which is amazing.

    However, I also have a "magic spells" plugin- this is an entirely gameplay oriented plugin, none of its commands make any sense outside the context of a player. So, it took me all of 30 seconds to convert this one, I just moved the body of onPlayerCommand to onCommand, and added this at the top:
    Code:
    if (!(sender instanceof Player)) return false;
    Player player = (Player)sender;
    
    And, done. In this case, it was the appropriate thing to do because none of the commands should be used outside of the game. And it took all of two extra lines of code. (Actually, it takes less code because I was able to get rid of the PlayerListener and its registration.... )

    I think that's well worth it for the added flexibility!
     
  12. Offline

    Firestar

    idk why you continued to use onplayerlistener for commands still (oncommand has been in there since 66 build or earlier) it has been oncommand(Player player, blah blah,blah)
    now it is a new class CommandSender which makes no sense and adds pointless code to plugins.
     
  13. Offline

    dag10

    Just because something doesn't make sense to you doesn't mean it doesn't make sense. And it's not "pointless code". Dinnerbone is correct: The Console should not be considered a Player--this is improper. A player is a player, not just a command-giving entity. However, both the Player and Console can send commands, right? That's why they implement the CommandSender interface. They are both CommandSenders. It makes perfect sense, and this is the exact reason why Interfaces exist. This is how Interfaces should be used.
     
  14. Offline

    Crash

    I'm confused about why you use onCommand when you have onPlayerCommand..
    Is it for commands sent from the console ?
     
  15. Offline

    darknesschaos

    out of curiosity, what are the possible sources for commands. The only ones that come to mind would be players and the console.
    --- merged: Feb 2, 2011 8:52 PM ---
    Please, lets try to not incite war. I understand he doesn't have much to back himself up, but still his point is valid, there is no point in getting angry and calling people on low experience, for all we know, he could have multiple private plugins.
     
  16. Offline

    NathanWolf

    After some use, I realize this is not quite the right thing to do- if you return false for a command you're responsible for, it seems like Bukkit spews out your help text by default. This is nice- but not what you want when an in-game command is invoked from the server console.

    So, the above should probably return true, and be moved to after you've already decided that you're responsible for this command.

    Still the same amount of modification, just a different arrangement.
    --- merged: Feb 2, 2011 9:00 PM ---
    Beta APIs are going to change as you use them, I'm sure you're used to this working with Bukkit plugins!

    I don't want to be a jerk, but I think you could have easily converted however many plugins you have over by now in the time you've spent complaining in here. :\

    It's really not that big of a change- just a couple of extra lines of code, especially if you're already been using onCommand for so long.
    --- merged: Feb 2, 2011 9:03 PM ---
    @Dinnerbone - I've got a plugin made for invoking console commands. I've modified it to support the new interface by calling CraftWorld.dispatchCommand directly- I fallback to the old method of calling a player command event if that fails, for backwards compatibility.

    So, my questions are:
    1. Is this safe / approved? I don't want to write a plugin that does badness.
    2. Is a return value of false a good indicator that no command was found? Or does it also mean that the command was found, but returned false?
    3. Will dispatchCommand be added to the Bukkit World API, or some equivalent, eventually?
    Thanks again for all your hard work and patience.--- merged: Feb 2, 2011 9:07 PM ---
    I think that's true for now, but I can imagine lots of cool possibilities: commands from an IRC interface, commands from a web interface, etc.

    All of these different sources may have their own user system with their own permissions, etc- so plugins could react differently to commands sent from different sources.

    So, for instance, maybe you could cast down CommandSender to IRCPlayer, and then grab an in-game player reference from that (assuming there is some system that binds IRC users to in-game users, etc). Same thing with a web client, it could use web authentication to bind clients with in-game players, allowing you to do some really cool stuff.

    How about commands that source from dynmap? You could log in, click somewhere on the map, and your player tp's there.

    There are so many creative uses for this system... It's really all very elegant :)
     
  17. Offline

    dag10

    Way to come off as a douche. I've seen many forums, but never have I seen a Moderator say something like that so abruptly.

    My "plugin count" is not required to make my point anymore valid; I am speaking factually. Interfaces were created for this sole purpose.

    And furthermore, darknesschaos is correct in that I have made multiple private plugins for my server. A live example of one such plugin can be observed here: http://emagination.minipenguin.com. View the user list, live chat (AJAX), and chat logs. Both the front-end and back-end have been coded by me. Also, view the website with Chrome, Safari, or Firefox. The few people who I have intended this site use Chrome, so I couldn't be bothered to create Internet Explorer compatibility.

    Additionally, I am currently working on a public plugin. I don't want to spoil the surprise for you guys, so I will withhold from saying more. G'day.
     
  18. Offline

    NathanWolf

    Wow, that is a nice website! If it had dynmap integration, it'd be perfect... Please tell me those are live in-game screenshots in the background? Or is that asking too much? :)

    Sorry, didn't mean to go off-topic there.
     
  19. Offline

    dag10

    Not live. It's interesting that you say that however, for this morning I was considering to create a somewhat-live screenshot system. It's just be a Minecraft client running on my web server, which would take a screenshot every two minutes and upload it to the web directory. I'd also have to make a plugin to hide that user and username from all other clients.

    As for DynMap, I was strongly considering it. However, my private plugin that powers the chat logs, live user list, and live chat uses a Java HttpServer, and DynMap also runs its own web server. So the most optimal way to integrate it would be to combine their web servers. I may look into that once I finish the public plugin I'm working on.
     
  20. Offline

    NathanWolf

    I'm interested, but going to take this to PM since it's horribly off-topic :)
     
  21. Offline

    Acru

    I have an issue with onCommand, in that its splitting the 'arguments' of a command, when I want to get a plaintext string. (Editing signs and other text, spaces matter~) So I was using the PLAYER_COMMAND event instead, but I just found out that you removed it? How can I get the arguments not split() now?

    Edit: I wanted to use onCommand originally, but I didn't for the above reason.
     
  22. Offline

    Firestar

    you can merge them back together, you did not seem very interested when I brought it up almost a month ago...
     
  23. Offline

    Acru

    Me personally you mean? My first plugin is only two weeks old. XD

    split(String regex) will remove trailing empty strings (loosing trailing spaces in sign text formatting), and splitting a string just to rejoin it is suboptimal in any case.

    I switched over to PLAYER_COMMAND_PREPROCESS for now, which is probably not desirable.

    Edit: I'm not insisting as I can think of ways around this, like using a non-space char in place of spaces, but I would like to see an onCommand method with non-split args. (This could either replace or be in addition to the current method.)
     
  24. Offline

    Firestar

    not going to happen.
     
  25. Offline

    Dinnerbone Bukkit Team Member

    Actually, it probably will; just not in the same design you're thinking of. I'm working on it, just have faith :)
     
  26. Offline

    Acru

    Fair enough :3
     
  27. Offline

    xZise

    A nice, I already waiting for this so I could get rid of the listener thing and also the console could use my parser.

    Fabian
     
  28. Offline

    daemonfire

    Why don't we make something like ircUser and Player implementing an interface, or abstract class.
    Because basically both are "players" of the server.
    One object (the Player Object) has all the additions it already has, and one object the ircUser also a person who "indirectly plays/accesses" the server as user(=player) has all the irc methods etc it had before plus things like .getName().

    This wouldn't break the system.
    It would enhance the object relation, since I tried to point out, that both objects are "Users/Players".
     
  29. Offline

    xZise

    I don't get your problem, but you can easily create a new class (e.g. IRCUser) and implement the CommandSender interface. The interface doesn't restrict you in adding a “getName” method. Instead it is a bad idea to let IRCUser implement Player. First the Player class have thousands of methods and second, some of them are in-game specific, like the location or teleportation.

    If you want, that also other plugins easily could use this “getName” method, you maybe discuss with other developers (of chat plugins etc.) so you could e.g. create an interface “Nameable” which only supports “Nameable.getName()”. So other plugin developers only have to check if the CommandSender either is a Player object or a Nameable. Also other plugins could use this interface to declare there CommandSender as Nameable, so that the chat plugin hasn't to differ between different “getName()” implementations.

    Maybe it is also possible to use relfection to avoid an additional interface. But this normally should be more complex.

    I also implemented something like this in my xWarp plugin. Before I added this feature the CommandSender instance had to be a Player object. But now it is also possible to create own CommandSender classes which implement Warpable (Warpable btw is already extending CommandSender). So it could be possible e.g. for a mapper plugin to create a Warpable class (which is a CommandSender class) and use my plugin. The code is on github.

    Fabian
     
  30. Offline

    Tagette

    In my Template I try to make use of this. Please let me know if I correctly did this and if not point out where I went wrong.
    Click! <- Click That

    Heres a link to the source of a example command.
    Click!

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 8, 2016
Thread Status:
Not open for further replies.

Share This Page