API suggestion (Listeners and TempBlocks)

[YourAverageNerd]

Hello fellow human beings,

While working on an addon ability, I stumbled upon something that, in my opinion, the API lacks. So I was told that the API is really clean and easy to use since an addon only requires 1 class. This was very nice to hear and I was looking forward to a clean design and easy to use structure. I'm completely satisfied with some sides of it, but at the same sides disappointed by other sides. Allow me to explain and suggest my solutions.

There are 2 things I was disappointed about. These 2 things are Listeners/activation and TempBlocks.

Listener:
If you're a developer and reading this, you probably know what a Listener is. For those who don't know, it's used to 'listen' for when something happens, AKA when an event is fired. The most used events for ability activation are 'PlayerAnimationEvent' (when a player left clicks) and 'PlayerToggleSneakEvent' (when a player toggles sneaking). Now, a method for either or both of these events has to be made somewhere. One could just go ahead and make the class implement Listener and put the methods in there, but I found it became quite messy quite quickly, because you also have to check if they have the ability bound, can bend in that area, etc, etc. I'd like to propose a solution to this.

There will be 2 abstract methods in CoreAbility, or another implementation of Ability that the developers see fit. These methods will be called 'click(Player)' and 'sneak(Player)'. In PK's Listener, there would be a check to see if the event's player has an addon ability bound, if so it gets that ability and calls either 'click(Player)' or 'sneak(Player)', depending on the event's type. Before calling one of these methods, it would also check if they are allowed to use the move, if they can bend in that area, and all the other checks, due to the method being called from PK's listener.

Pros:

• Less messy in the addon's code.
• Less registered listeners.
• Less calls to 'heavy' methods such as 'isRegionProtectedFromBuild' (last time I checked it was quite the heavy method )

Cons:

• There probably are some, but I really can't think of any at the moment.

If the ability needs to listen for any other actions, they still can do so by implementing Listener and listening for the event they need. But most addons don't have to listen for any other I think, and are fine with just sneak and click, so they would not have to register an addon.


TempBlocks:
Now, for TempBlocks. Don't get me wrong, they are great, really. They are already extremely useful and I love them. I just have 1 problem with them, and that's the removal/reverting of TempBlocks. I have to keep track of them, and revert them manually. I think it would be great if there was an additional constructor with a 'time' argument, which specifies after how many ticks or milliseconds the TempBlock should revert to it's original state. I believe currently all addons and stock abilities have to do this themselves, which I think is not a great thing. I suggest there is 1 central map in the TempBlock class or in a TempBlockManager class, which keeps track of all TempBlocks, and will revert them if they have a revert time specified and if that time has ran out.

Pros:

• Abilities don't have to keep track of TempBlocks individually.

Cons:

• Again, I can't really think of any at this moment.

Thank you for reading my suggestions! In my opinion, the first one is a bit more important than the second, yet I still think they will both be useful.

Please let me know what you think and if you think this idea can be improved! Thanks in advance.

~~ YourAverageNerd ~~

 

[Loony]

@OmniCypher

 

[OmniCypher]

Hey @YourAverageNerd, let me address a few issues with your two suggestions. Firstly, on your suggestion about a click() and shift() method in CoreAbility; lets compare the amount of code required for using listeners or your click() or shift() methods.

Listeners:

@EventHandler
public void onLeftClick(PlayerAnimationEvent event) {
    if(bPlayer.canBend(this)) {
        // Code for your Addon
    }
}

Your Method idea:

public void click(Player player) {
    // Code for your Addon
}

As we can see here your change only takes away 2, maybe 3 lines from the method itself, one of which is just the annotation for EventHandler. In addition to the code only being slightly shorter you end up giving up all information accessible through the direct reference to the event. For instance, you have access to cancel the event or get its handlers if needed. If you were using PlayerToggleSneakEvent you also wouldn't be able to check if the player began sneaking or stopped sneaking because you wouldn't have access it as displayed below.

Listeners:

@EventHandler
public void onLeftClick(PlayerAnimationEventevent) {
    if(bPlayer.canBend(this)) {
        event.setCancelled(true); // For whatever reason you may want to cancel the event.
    }
}

@EventHandler
public void onToggleSneak(PlayerToggleSneakEvent event) {
    if(bPlayer.canBend(this)) {
        if(!event.isShifting()) {
            // Code pertinant to the player starting to shifting
        } else if (event.isShifting()) {
            // Code pertinant to the player unshifting
        }
    }
}

Your Method idea:

public void click(Player player) {
    // Cannot cancel the left click event
}

public void shift(Player player) {
    // Cannot determine the shift case of the event
}

Finally, many addons may want to use Left and Right click which are two separate events, PlayerAnimationEvent and PlayerInteractionEvent. Would you propose we have two separate methods for each type of click?

In the end this suggestion ends up taking away from what developers would be able to accomplish with their addons at the cost of making their methods a few lines longer. I can't see this being worth all the backend changes that would need to occur to account for these two special event cases.

 

[OmniCypher]

Now onto your second suggestion which regarded auto timeouts for TempBlocks. At first glance, this seems like an awesome idea and it is something I really want to implement. However, there is a critical issue in making this a possibility, the performance hike. Now going off what you said in your post

an additional constructor with a 'time' argument, which specifies after how many ticks or milliseconds the TempBlock should revert to it's original state.

If we were to make this operate at a tick or millisecond increment we would need to perform possibly thousands of boolean checks every tick/millisecond to decern if the current index was passed it's individual auto revert time. This would cause the amount of processing power ProjectKorra used to skyrocket, all for a background process. On a side note, this is actually similar to the current issue with PhaseChange which is causing massive lag throughout tons of servers in our community. As you can hopefully see this just isn't feasible at the level you discussed. However, a per second check or per minute check is much more likely. We just need to find the correct balance between performance and functionality.

 

[YourAverageNerd]

Hey @YourAverageNerd, let me address a few issues with your two suggestions. Firstly, on your suggestion about a click() and shift() method in CoreAbility; lets compare the amount of code required for using listeners or your click() or shift() methods.

Listeners:

@EventHandler
public void onLeftClick(PlayerAnimationEvent event) {
    if(bPlayer.canBend(this)) {
        // Code for your Addon
    }
}

Your Method idea:

public void click(Player player) {
    // Code for your Addon
}

As we can see here your change only takes away 2, maybe 3 lines from the method itself, one of which is just the annotation for EventHandler. In addition to the code only being slightly shorter you end up giving up all information accessible through the direct reference to the event. For instance, you have access to cancel the event or get its handlers if needed. If you were using PlayerToggleSneakEvent you also wouldn't be able to check if the player began sneaking or stopped sneaking because you wouldn't have access it as displayed below.

Listeners:

@EventHandler
public void onLeftClick(PlayerAnimationEventevent) {
    if(bPlayer.canBend(this)) {
        event.setCancelled(true); // For whatever reason you may want to cancel the event.
    }
}

@EventHandler
public void onToggleSneak(PlayerToggleSneakEvent event) {
    if(bPlayer.canBend(this)) {
        if(!event.isShifting()) {
            // Code pertinant to the player starting to shifting
        } else if (event.isShifting()) {
            // Code pertinant to the player unshifting
        }
    }
}

Your Method idea:

public void click(Player player) {
    // Cannot cancel the left click event
}

public void shift(Player player) {
    // Cannot determine the shift case of the event
}

Finally, many addons may want to use Left and Right click which are two separate events, PlayerAnimationEvent and PlayerInteractionEvent. Would you propose we have two separate methods for each type of click?

In the end this suggestion ends up taking away from what developers would be able to accomplish with their addons at the cost of making their methods a few lines longer. I can't see this being worth all the backend changes that would need to occur to account for these two special event cases.

Now onto your second suggestion which regarded auto timeouts for TempBlocks. At first glance, this seems like an awesome idea and it is something I really want to implement. However, there is a critical issue in making this a possibility, the performance hike. Now going off what you said in your post



If we were to make this operate at a tick or millisecond increment we would need to perform possibly thousands of boolean checks every tick/millisecond to decern if the current index was passed it's individual auto revert time. This would cause the amount of processing power ProjectKorra used to skyrocket, all for a background process. On a side note, this is actually similar to the current issue with PhaseChange which is causing massive lag throughout tons of servers in our community. As you can hopefully see this just isn't feasible at the level you discussed. However, a per second check or per minute check is much more likely. We just need to find the correct balance between performance and functionality.

I see, thank you for explaining!