Reverted pull requests, duplicate fields, strange additions to formatting: What's going on?

[ashquarky]

Hiya!

So, I'm really happy that my PR (#18) was accepted. However, pretty much immediately after, some... weird stuff started happening.

First, a few more normal commits were made. Stuff such as extra functions and the like. Weirdly, it seemed like TheMCPEGamer forgot to sync the files on his PC or something, as some of the stuff he edited (see 8f96a33630a8dd2187da62c89d39e0965481025f) was removed by me. Just check the green side of BlockPlacementHandler on the merge commit (e61b2e259c3990b3310f71c6299ad6d0cf258c3b). See how I commented a bunch of lines out (the code was causing useItem() to be called twice)? Now check the same file on the commit directly afterwards (8f96a33630a8dd2187da62c89d39e0965481025f). It's like the merge commit never existed...

Then stuff got weird. First, the merge was reverted. Then, a branch (revert-18-master) was merged (what, just reverting it wasn't good enough for you? PR #19) and right after that, all my new functions were added back in (c93e81c03ef8352edb81deb6439b1bb9ac496014) but without all the backend stuff I did - so 1. All my bugfixes are gone and 2. next to none of my hooks and functions work anymore. To finish it all off, said re-uploaded functions weren't even re-added into Functions.java, so they aren't usable.

Then... 0d476f6006e89a1582846bc8c23fff5c3a3e5dae . This commit did some weird stuff that's just... weird. I don't know if you guys have something on in your Skype chat or something but to everyone outside, this code breaks the whole program - it won't even build. It removed a random function of mine (um, why??) and added all this crud:

    +<<<<<<< HEAD
    public static String[] Functions = new String[] {"clientMessage", "addItemInventory", "getCurrentWorld", "setBlock", "setArea", "getServerName"};
    public static String[] Functions = new String[] {"clientMessage", "addItemInventory", "getCurrentWorld", "setBlock", "setArea", "getServerName"};
    public static Class[] Classes = new Class[] {clientMessage.class, addItemInventory.class, getCurrentWorld.class, setBlock.class, setArea.class, getServerName.class};
    public static Class[] Classes = new Class[] {clientMessage.class, addItemInventory.class, getCurrentWorld.class, setBlock.class, setArea.class, getServerName.class};
    +=======
    + public static String[] Functions = new String[] {"clientMessage", "addItemInventory",     "getCurrentWorld", "setBlock"};
    +
    + public static Class[] Classes = new Class[] {clientMessage.class, addItemInventory.class,  getCurrentWorld.class, setBlock.class};
    +>>>>>>> c9809ff8faa896943476b654da20442535c08aca

W.. what? I just wanna know: What the heck is going on? The code doesn't work anymore and there's some really weird stuff (with regards to my additions, and pretty much everything else).

Thanks! -Ash

[DefinitlyEvil]

@TheMCPEGamer

[ashquarky]

Indeed, @TheMCPEGamer

[wordandahalf]

I forgot to sync the stuff that I added before adding your PR, and then I tried to sync it, which resulted in well... that, I'll fix it now My fault

[wordandahalf]

Just pushed a fix (hopefully) Comment here if I forgot to do something

[ashquarky]

I should mention that useItem is called twice per actual item use, as it's mentioned in both the digging/building handlers and PlayerInteractEvent. My fix was to get rid of the one in the dig/build handlers (as some plugins call PlayerInteractEvent themselves, see b6dfb8ce5bf9b9336ffe76d6a9b18d2aa29ce072), but since you specifically added it back in (ff3d7b93d06d2f0608a420d82d1e14df9ef526e8) I guess you have some reason. Also, Rhino doesn't seem to be happy with Byte being an argument in both setArea and setBlock - I had to remove that to get the server to even start. It seems changing the type to Object and casting it later fixes the immediate pulsing errors (Like so:

    public static void setBlock(String worldName, int x, int y, int z, String tileName, Object tileData)
    {
        org.dragonet.DragonetServer.instance().getServer().getWorld(worldName).getBlockAt(x, y, z).setType(Material.getMaterial(tileName));
        org.dragonet.DragonetServer.instance().getServer().getWorld(worldName).getBlockAt(x, y, z).setData((Byte) tileData);
    }

However, a new issue comes up with this one: No matter how hard I try, I cannot convince Rhino to create a byte in the Javascript itself. Like, I tried everything and it just doesn't work. I reckon it'd be the most painless by changing the argument to a Integer or String and parsing it into a byte (with java.lang.Byte.parseByte or something). Right now, I just can't get setBlock to work...

EDIT: I eventually got setBlock to work, but the way you have to do it is, frankly, insane:

function useItem(x, y, z, face, name, player) {
    data = java.lang.reflect.Array.newInstance(java.lang.Byte.TYPE, 1);
    data[0] = 3;
    setBlock(getCurrentWorld(player.name), x, y, z, name, data[0]);
}

Even then, only about half of the wool blocks I place end up turning blue, with a good spattering of NPEs on the PlayerInteractEvent. It's just... a bit buggy.

[wordandahalf]

Again, my fault, will fix soon.

[wordandahalf]

Hopefully fixed it, comment if I missed something again!

[ashquarky]

One last thing (wow, this is tough to get working) Rhino also doesn't like org.bukkit.entity.Player as an argument for clearInventory. After searching for a while, I found the Javadoc page for JS Functions which in the constructor section mentions that JavaScript methods can only have arguments of Object, String, boolean, Scriptable, int, or double. Yeah... wow.

Anyway, it seems the way we'd have to do things is like so:

public class clearInventory
{
    public static void clearInventory(Object plr)
    {
        Player player;
        try {
            player = (Player) plr;
            player.getInventory().clear();
        } catch (ClassCastException e) {
            org.dragonet.DragonetServer.instance().getLogger().warn("[DragonetAPI] Script passed non-player on clearInventory();! Please alert the script author.");
        }
    }
}

Apparently, that's the only way to get past Rhino's weird argument limitations.

Also, having stopServer() and stopServer(String) in the same class is chucking Rhino for a tizzy. It's also having similar issues with banPlayer - looks like the only fix is to rename the methods (but you don't have to change the class - something like this:

    public static String[] Functions = new String[] {"clientMessage", "addItemInventory", "getCurrentWorld", "setBlock", "setArea", "getServerName", "runConsoleCommand", "disconnectPlayer", "clearInventory", "stopServer", "banPlayer", "stopServerWithMsg", "banPlayerWithReason", "banPlayerWithReasonAndDate"};

    public static Class[] Classes = new Class[] {clientMessage.class, addItemInventory.class, getCurrentWorld.class, setBlock.class, setArea.class, getServerName.class, runConsoleCommand.class, disconnectPlayer.class, clearInventory.class, stopServer.class, banPlayer.class, stopServer.class, banPlayer.class, banPlayer.class};

Sorry for finding so many issues ;3

[DefinitlyEvil]

WTF!! You shouldn't do those things. Just add a getPlayer() method and inventory clearing can be done by doing getPlayer("blah").getInventory().clear(). Javascript can call Java class methods. PLEASE, read more about Bukkit API before you add things! @TheMCPEGamer @QuarkTheAwesome

[ashquarky]

@DefinitlyEvil I was aware of getPlayer() and stuff like that, but @TheMCPEGamer added it (with Player as an argument, mind you; see e1f649d9af79d09a0175eea71f2772ef0222c495) so I decided to keep the methods (and their arguments) as-is. Also, isn't the whole point of the DragonetAPI to provide an easy wrapper for the existing commands that Bukkit provides while still adding a few of our own? I mean, by your logic, we shouldn't make any functions at all because JS can access all of these directly. It's just easier to have these functions, that's all.

On top of that, doing stuff this way will get everyone in the habit of using our methods instead of Bukkit's, so we can further customize them later - maybe event priority? Maybe compatibility with a possible future custom Player class? It's about future-proofing the code, and making Javascripts go through us is the best way to do it.

[wordandahalf]

@QuarkTheAwesome You are the best bug finder I have ever seen, thank you!

[ashquarky]

@TheMCPEGamer Really? ;D Thanks!

[ashquarky]

Okay, just one last thing with banPlayer(): See how clearInventory has a try/catch? banPlayer doesn't have that, so if a script author is lazy, they could pontentially spam up the console with errors. Also, a player must leave and rejoin for a ban to take effect - I'd suggest kicking them after adding them to the ban list.

P.S This must be an oddity in Glowstone, but it's damn hard to unban a player again! Ugh!

[wordandahalf]

@QuarkTheAwesome if.... (I just had to say that, sorry, I'll fix it right away!)

[ashquarky]

@TheMCPEGamer xD

[wordandahalf]

I think I can close this now... Am I right?

[ashquarky]

@TheMCPEGamer Yeah, I think the original issue has been fixed.