How should we handle event property changes?

[Variadicism-zz]

I woke up this morning and realized that there's a big problem with the way we're handling events right now. The way we're handling it now is fine for certain events that can't really be modified, only cancelled, like onPlayerDisconnect or onPluginUnload. However, for other things, we need a way to allow plugin authors to be able to change some properties of the event. For example, when a player dies, there's a "death message" that plugin authors should be able to change in the event handling. However, with our current setup, all that can be done is cancel the event.

So, how should we do this? Any ideas? I'd like to avoid CraftBukkit's method of making Event Objects for every single different kind of event because it means making lots of extra classes and it means you can't immediately see all the properties of the event. However, if that's what it comes down to, we may have to do that.

Another idea might be to not have Event Objects to pass into the listener methods, but have the listener methods return EventResult Objects of some kind. We could have a general EventResult object that just tells whether or not the event is cancelled that could be used for most events; for that, we could even make some public static final EventResults called CANCELLED and NOT_CANCELLED so that plugin authors can simply say "return EventResult.CANCELLED". It's not the prettiest solution, but it might work, too.

[Niadevv]

Yeah, that could work. Perhaps in EventResult also store all possible event variables, so instead of multiple classes, it's all in one place, and you can change things like in Forge's event system. Perhaps we could set it with a setter method that checks if the event is an appropriate type for the property being set, likely checking the current event's event id (which should be preferably string ids). I'll gladly get to work on it and see how feasible it is, if this is an appropriate plan for the EventResult system.

[Variadicism-zz]

I think it would be better to have multiple types of EventResult. Otherwise, the EventResult Object is going to be carrying around a lot of baggage that it won't even be using. Lots of events have properties specific to them, so if we made it into one class, that Object would be quite large.

The EventResult should be small and easily mobile because events are going to be one of the biggest, most heavily used parts of the A.P.I. Just think: you can have up to 20 "onPlayerMove" events every single second PER PLAYER on the server. That's a whole lot of events. If every EventResult from every player movement includes a player's death message, the item and velocity of the item dispensed by a dispenser, the amount of experience orbs to drop when an entity is killed, and more, we're going to have memory and performance problems, especially considering how notoriously bad Java's garbage collection is.

http://lifeofasoftwareengineer.tumblr.com/post/62681511158/java-garbage-collection

Then, there's the problem of just simply confusing the plugin authors. What are they going to think if they're trying to see what kind of things they can change in an event and it looks like they can set the amount of experience orbs dropped on a player movement event, then they get an error when they try to use setExperienceDropped()? It's confusing because it looks like you can do it, but you actually can't. If we have subclasses for specific events' results, then everything the plugin authors see they can use and they can't use anything they can't see. There's no ambiguity or confusion.

[Niadevv]

Hm, yup, multiple types of EventResults it is, like EventResultPlayerAction for player actions, or EventResultCommand for commands. Each could contain inner classes for specific events, like EventResultCommandExecuted and EventResultPlayerStep. Therefore, there's only a few actual independent classes, but more internally.

And by the way, that image is hilarious XD

[Variadicism-zz]

Ha ha. Yeah. I love that image. I have it bookmarked.

Anyway, I think we have a good plan, but I'd like my coder friend @roryee to see this before we go forward with it. He's going to start committing with us. I'd like to see if he has any better ideas. I think the EventResult plan isn't bad, but I still feel like there's a better solution out there. I just don't know what it is.

[Variadicism-zz]

I discussed it with @roryee. He said it seemed fine. I've implemented it in our existing listener stuff and I think it looks pretty good. I'm closing this issue.

[Variadicism-zz]

I know that this was closed a while ago, but I've thought up some more issues with it, namely designing for change. If we have parameters for each listener method representing the input information concerning the event, that would mean that if we wanted to add a new piece of information later, all of the old listener methods would break even though they didn't need that new information.

[Niadevv]

Forge has a flexible solution in that it passes data when the event is fired, and I'm fairly sure I suggested a similar solution the first time around. I suppose we could do something similar, but it'd bloat the amount of classes enormously. Another option would be to pass the data in an Object..., however, that would reduce clarity and make it just harder on the plugin author as they'd have to cast those to do anything useful with them, as well as making it not immediately obvious. Personally, I'm leaning towards the single class for each event approach as it makes things easier to understand to the new user. After all, I'm sure there's a reason the Forge team went with their design choice for events. That way allows for events to be expanded quite easily, by just having a separate class for each event. And these classes are rarely anything massive, just a constructor and a few getter methods at most. And, if need be, I'm sure we can shoehorn in the EventResult class somewhere.

[Variadicism-zz]

Ah, I see. It looks like they did it like Bukkit did with Annotations and Reflection. I didn't like the idea before because it's not very fast, but on the other hand it's very easy to write.

So, for choices on how to implement listeners, we could do it a couple different ways:

• We could register all the EventHandler methods we could find when we load the class into a list or map.
  • We would still have to use Reflection to call the methods any way we go, which is slow and more prone to runtime errors.
  • On the other hand, lots of people are already familiar with that setup and it's very easy to use. In fact, with the way Corundum loads all classes individually, we wouldn't even have to ask plugin developers to make their listener classes extend Listener or use that registerEvents() method. (We may still want to use a Listener interface so that Corundum can load plugins more easily by not having to check every single method in every single class for an EventHandler Annotation... or maybe not.)
• We could also use interfaces to avoid Annotations and Reflection.
  • That would require us to make a lot of interfaces for all the different listener methods and it would be more tedious for plugin authors to have to make their classes implement each interface for each event they want to listen for.
  • On the other hand, it would be faster to process and if we make each interface named intuitively, e.g. give them the exact same name as the event they listen for with "Event" replaced with "Listener" at the end, I don't think it would be a big deal for the plugin authors. That would also mean a much smaller chance of forgetting to make your classes with listener methods implement Listener, which is very easy to forget. (I know from experience.)

If we do that first option, we probably couldn't use the EventResult system with the different input parameters; having all sorts of different listener methods with different arguments all over the place would make it easy for plugin authors to get the arguments wrong, causing runtime errors, while using the second option would make incorrect arguments cause compilation errors instead, which is preferable because you can see the error before testing or releasing your plugin.

If we do the second option, we could use the EventResult system or a more Bukkit-like or Forge-like Event object system; either would work.

I, too, am leaning toward the Event object system instead.

[Niadevv]

I'm pretty sure Forge did some fancy ASM stuff for their annotations to avoid the performance issues that come with Reflection, so if we did a similar thing with ASM we could have a fast AND flexible system. Plus, with annotations, it allows for more freedom with method naming, as I know that there are some people who name methods rather differently, and would be much happier with their method naming rather than a method name they have to use, or else it doesn't work at all. However, you have previously stated your dislike for ASM as it complicates things, which is correct, so I'm leaning towards the interfaces method. When I did an event handler system, I initially went with interfaces, to avoid complication. I later switched to Reflection and finally ASM.

Fortunately, I've used ASM before, and have a good idea of how to get method param types, method names, etc., so if we go the ASM route, I wouldn't mind doing the ASM parsers :). The only thing I had issues with was collecting the bytes of classes from the class path, which is the main reason that my project with the event handlers has been sitting at > 290 commits for a few months now.

[Variadicism-zz]

Yeah. There are also pre-processor extensions you can make for I.D.E.s like Eclipse that can use Annotations in a similar way (like Lombok), but all that gets complicated and we would either have to try to control the build time processing in different I.D.E.s or the bytecode transformations on plugin load. Either way, I think it's way too complicated for our tiny team here. Interfaces would work fine and be a little tedious to write, but not too bad. Reflection would also not be too difficult to write and the performance hit might not be too bad since we have myList to sort things and find items in O(lg(n)) time.

[Variadicism-zz]

I gave using interfaces with Event Objects a try; I put it in a new branch called "reenginerring_Events". Let me know what you think! I got it to the point where if we need to run an event, all we should have to do is create the Event object for the event we want to make and call Event.runOn(CorundumServer server)!