Spongeapi: Event Cause Refactor

[gabizou]

Currently, the first iteration of Cause usage was useful for a variety of things, however, we've come to a point where we need to separate what is the "cause" of an event versus the "context" of an event.

One of the things that would take place would be the following:

Event gains a new getter for the direct cause of an event, possibly will recycle Cause.

Event also gains a new getter for the context such as EventContext, of which named objects are included, a method simply named as Event#getContext() or possibly getEventContext.

Of course, this will not take place until likely 1.9 as a majority of breaking changes are taking place there.

[gabizou]

Prodding @SpongePowered/developers for input

[Aaron1011]

@gabizou: Will EventContext take on all of the other functions of Cause?

[bloodmc]

We shouldn't require every cause to have a name due to performance reasons so whatever we can do to fix this would be good.

[modwizcode]

@gabizou What are you considering part of the event's context rather than its cause?

[gabizou]

Will EventContext take on all of the other functions of Cause?

EventContext is going to be implied that it will be contextual information, mostly by String keys (basically take Cause and rename it EventContext, it'll be easier to manage since it will be utilizing strictly NamedCauses over the current mapping used).

Cause itself will likely need to be rethought, but the current idea is that Cause will only contain the _direct_ cause of the event and eliminate the needs for naming.

We shouldn't require every cause to have a name due to performance reasons so whatever we can do to fix this would be good.

The root performance reason is that you're chaining causes continuously, which of course is going to end up having some cost of either generating new strings, or validating that the objects are distinct.

@gabizou What are you considering part of the event's context rather than its cause?

Read above.

[TheRaspPie]

@gabizou So basically, do you take NamedCause.SOURCE as a cause and all the other ones as context? May a context be empty then and a cause not?

[gabizou]

@TheRaspPie They'd be two separate objects entirely. The Cause of an event would only be a single object, if not a collection of directly directly causing cause objects to remain, as such they are considered the "Source" of an event. Contexts would reside within the related event, so DamageEntityEvent.ATTACKER, etc.

[DarkArc]

One thing which troubles me, is many times I've wanted to differentiate between a plugin and something natural. Like an added item drop, and a vanilla item drop. I don't like how in the current system, a cause can be a plugin. That's an implementation detail IMO, and as such it should not be part of the cause.

Instead, I think we need some concept of "markers." I'm not sure what that should be, or if it should just be part of the context. But I think there should be a separation between game play context, and implementation context, and furthermore causes should never be based on an implementation detail, but instead what is observable from the players perspective.

[Zidane]

@DarkArc

I don't fully agree on the premise that the plugin container being in the cause IS the marker. Only when an API method triggers the event will the plugin container end up in the cause. Otherwise, that is a fault on Sponge's behalf. The API contract of these methods that trigger events should always take a PluginContainer and, therefore, it can be reasonably expected for a plugin dev to check a cause FOR a PluginContainer and then know right then if its natural or not. That fact alone removes the implementation detail aspect.

On a similar note, why does it matter? I mean, the point of Cause/Context is for you to simply query for what you want. Sounds like you don't want to perform logic on plugin-triggered events, which is fine, but otherwise plugin devs should never care. Query for what you want and go.

[DarkArc]

@Zidane by allowing the plugin to be the cause, we in a sense corrupt the picture of what's going on.

For instance, if a block spawns an item, and that item spawning implementation is determined by a plugin what is the root cause? Is it the block, or is it the plugin?

What if I've written some code that blocks drops directly created by a block? If the root cause in this case is a plugin, I'm not going to get what I expected. Maybe this will change in the new cause tracking system, maybe not, but as it stands, a forge mod can get picked up by cause tracking, doing the exact same thing, and have the root cause be classified as the block, instead of a plugin/mod.

This creates a very disjoint picture of what is actually happening, and it leads to a lot of special cases. Also, keep in mind, that you cannot safely merely query for a block somewhere in the cause, as that may expand the scope of your code too significantly. You're trying to prevent blocks from dropping items, not item drops that may have a block somewhere in their cause chain.

For a more concrete example of where this becomes awkward/problematic. I have region protection code, which blocks a players changes, if a player is at all involved. If they are directly involved (the root), it sends them a message. Now, if an action is performed on behalf of a player by a plugin, despite it being in the player's eyes, directly caused by them, they won't get a message, because the root cause is a plugin.

---

On a related note, there are situations in the current system, where the source at the implementation level (whether the behaviour is from vanilla, a forge mod, or a plugin) is completely lost.

The other night, I wanted to prevent Vanilla drops on Zombies and add my own, within the context of a minigame. The only way I could accomplish this currently, is by blocking all drops from zombies, and then delaying the custom item drops by a tick. This allowed me to escape cause tracking pulling them into the event (and thus subsequently having my code block them as well).

---

I agree having the plugin be part of the context (in respect to the proposed changes) can signify a "marker", however, I don't know if there is a better way to represent implementation detail context.

At the same time, I have strong reservations about allowing the root cause (in respect to the old system), or the cause (in respect to the proposed changes) to be a plugin.

[Zidane]

@DarkArc

First off, I don't think its fair to continue calling it an implementation detail. If an API method takes a PluginContainer, its gonna fire an event WITH the container in it.

Now, your issue might go away if, with the refactor, we make PluginContainer become the owner of the event. This change is supported even more so by the fact that API events can be fired by plugins. If a method takes a PluginContainer, same deal.

Point I'm trying to make is I don't find your argument of "Do what the Player sees" as valid. We shouldn't muddy the waters; it should be crystal clear when a Player does something and a Plugin does something on the Player's behalf. If you decide to intermix it, to make it more "realistic to what is going on in-game" then you are basically lying to plugins which is something that would, personally, annoy the hell out of me. Sponge's job is to give you an accurate representation of what is going on, not what APPEARS to be going on.

As for your issue regarding the Zombie, you should be able to add on to its item drops list?

[Zidane]

@DarkArc

Been thinking about your Zombie example all evening. Can you paste your event listeners? I need to see how you are tackling this issue.

[ryantheleach]

@DarkArc , @Zidane correct me if I'm wrong, but this is my understanding of how the system is designed to work, not just how it currently happens to be operating.

At the moment, if you modify the drops / spawn items in an event listener, isn't the items being spawned currently in the mob drops "stage"/"phase" so that other plugins can see that items spawned on behalf of the zombie being killed? (this is how it currently works, the 1.9 refactor (SpongePowered/SpongeCommon/pull/645) merely formalizes it into a state machine)

This would then, naturally, throw another item spawn event, with the same/similar cause as the previous one, potentially with the pluginContainer added to the cause if the API method takes a plugin container / cause?

asside:

By delaying until the next tick, you are changing that context from being a modified mob drop, to a random item being spawned in by a plugin off the scheduler. and that's what Zidane perceives as wrong.

In order to not catch your own item spawns and potentially modify them, you need some way of identifying that the event was caused by you, or previously processed, so couldn't you just not listen to events with your plugincontainer as the cause?

However, if 2 plugins conflict, I could see this ending in a stack overflow, so the better bet is for plugins, to not even listen to plugin caused events 99% of the time, unless it's for cancellation / logging and not modification.

However, getting plugin owners to do this by default, when testing their plugin on their own causes no issues could be tough.

[ryantheleach]

@DarkArc Wait, I think I get what you are saying.

So instead of just assigning a plugin container, you potentially want more details.

So you could attach a marker/tag/string and a pluginContainer that owns the marker (for debugging/disambiguation) then operate around that.

So you could have a string in the context of CreeperMend.BlockTransform for an example of instead of a creeper creating block damage, it changes the blocks to cracked bricks, coarse dirt, andesite, etc.

Some blocks could explode naturally, others transform.

the blocks that exploded naturally would have the vanilla cause, the blocks transformed would have the plugin.marker + vanilla cause?

Another plugin listening, could then see that the cause was the BlockTransform as opposed to an explosion caused naturally, or CreeperMend performing some other action, like turning the blocks to air without the drops spawning.

Is that what you mean?

[DarkArc]

First off, I don't think its fair to continue calling it an implementation detail. If an API method takes a PluginContainer, its gonna fire an event WITH the container in it.

I just don't see how a plugin breaking a block on behalf of a player is any different from vanilla breaking a block on behalf of a player, yet we don't say the source is vanilla. It's important to know which is which, but I don't see the plugin as the cause, part of the context, yes, the source/cause itself, no.

If you decide to intermix it, to make it more "realistic to what is going on in-game" then you are basically lying to plugins which is something that would, personally, annoy the hell out of me. Sponge's job is to give you an accurate representation of what is going on, not what APPEARS to be going on.

This already happens with Forge mods with cause tracking, we just disregard that a mod made the change, and treat it as though it was a vanilla change. (Though, I do think if we can figure out a way, we should include the Forge mod in the context).

---

Been thinking about your Zombie example all evening. Can you paste your event listeners? I need to see how you are tackling this issue.

Maybe I'm going about this in a stupid way. Currently I have a listener which blocks drops from creatures:
https://github.com/Skelril/Skree/blob/master/src/main/java/com/skelril/skree/content/zone/ZoneCreatureDropBlocker.java

Then there is some code called inside of the DestructEntityEvent.Death event:
https://github.com/Skelril/Skree/blob/master/src/main/java/com/skelril/skree/content/zone/group/catacombs/instruction/WaveDropInstruction.java#L143-L153

https://github.com/Skelril/Skree/blob/master/src/main/java/com/skelril/nitro/item/ItemDropper.java#L42-L51

Currently the items added to the world by the ItemDropper are indistinguishable to the items added by the Vanilla code dropping items, there's no discernible difference in the cause. Thus as @ryantheleach said, if I delay a tick, I escape the cause tracking, and it looks like an item just being dropped in the world, with no inferred binding to the entity.
[EDIT: Which allows my dropped items to enter the world]

Perhaps the more ideal situation would be to just have a mutable collection of item stacks be part of the DestructEntityEvent.Death event, though there may be some issues with capturing mods here.

potentially with the pluginContainer added to the cause if the API method takes a plugin container / cause?

Unfortunately, not at the current time.

---

the blocks that exploded naturally would have the vanilla cause, the blocks transformed would have the plugin.marker + vanilla cause?

Another plugin listening, could then see that the cause was the BlockTransform as opposed to an explosion caused naturally, or CreeperMend performing some other action, like turning the blocks to air without the drops spawning.

Is that what you mean?

Maybe... In my mind it should look something like this:

Vanilla:

Situation: A player destroys blocks using a standard pickaxe
Cause: Player
Context: [ItemStack(, and probably some other things I'm not thinking of)]

Plugin:

Situation: A player destroys blocks using a multitool implemented via the Sponge API
Cause: Player
Context: [ItemStack, PluginContainer, (, and probably some other things I'm not thinking of)]

This way I can choose to care about a plugin, or not. I still know what's really going on in game, which is a player broke a block using a tool, whether that situation is procured via Vanilla functionality, or a plugin, I don't really care.

For instance, if I break a block using WorldEdit as well, just about any listener I write to prevent direct block breaks from a player will do what I expect.

WorldEdit is still just a tool, just like a pickaxe, it's merely being triggered by a command in most cases, not vanilla block change logic. Except in this case, there's probably not an ItemStack involved in the context, just WorldEdit. Thus you would see something more like this:

Cause: Player
Context: [WorldEditPluginContainer(, and probably some other things I'm not thinking of)]

---

I suppose yes, there are some situations where it makes sense for the plugin to be the cause ex:

Situation: A plugin changes blocks in an area randomly (for a minigame)
Cause: MyPluginContainer
Context: [(maybe some other things I'm not thinking of)]

I retract my previous statement, we do need to be able to have a plugin be the root cause, this case somehow slipped my mind.

That said, I still feel particularly for implementing tools, and commands, having the ability to say X has a root cause of some player/command sender, not a root cause of random plugin activity which happened to involve a player, is important.

This is especially true if we disjoin the Cause and Context as we will have no equivalent to simply saying Cause.first(Player.class). This would be due to the fact that in some situations the cause would contain the player, in other situations the context would contain the player.

[TheRaspPie]

I think the most important thing to note here is that Causes will NEVER make any sense to be included in events without plugins passing it to API methods.

Now, as a plugin developer, what should you put in the Cause?
this? the PluginContainer? Hmm...
Too often have I seen something like this or Server in the Cause.
I think that most developers just won't do much in terms of tracking or capturing beyond the actual contextual information (event source, plugin) because they don't know WHAT to capture.
Plugins that filter for a Cause in a @Listener probably won't find the Cause included that fits their needs.

My idea here is simply to remember the past, buf differently than we currently do.
Currently, nobody knows exactly where cause tracking starts and ends and where to stop tracking: How many levels deep would you nest?
I think that decision is mostly based on common sense: what plugins might want to look for, but it's very hard to document concretely and sometimes considered to be an implementation detail

Now, there are two ways a server can change it's state or perform an action:

• An incoming packet from a client connection
• A server tick (if you work on a Vanilla based implementation)

I would basically consider these as candidates for "direct" causes.
Everything that is known from that should be included in the Cause (e.g. the player connection).

If you want to look at past events though, it would be great to keep a transaction log: Which DataContainer changed how, at what time and _WHY_.

The _WHY_ could be the original Cause of the event that was triggered as a reaction to the happened action. I'm aware this might consume a lot of memory. To circumvent this plugins would register for a specific type of capturing to happen (maybe a CatalogType that other plugins could extend), just any DataManipulator that may get changed and any Event that may get called.

Now, to demonstrate how such a system might work, here an example:

Suppose a developer wants to know the past travel of a Player or of all Players, for example to later check if someone was hacking.
The plugin would then register at startup that it want's to track the player position.
Later it could recall and fetch the data it wants. (maybe also add a "I want to discard this because it's no longer relevant" feature).

This would completely eliminate the need for the server to track random stuff because it knows just exactly what to track. Also plugin developers wouldn't have such a hard time as Data changes would ideally be tracked via custom data or explicitly added as a CatalogType and "the need for the past" would be more thought-out and made explicit.

That said, sorry for that very long text, but I hope it was worth the effort to read.

[Zidane]

Due to how massive the change this is (and how many plugins it'll effect), we're going to simmer on this change for a while and do it at a later point in time.

[parlough]

@gabizou Can this be closed with the cause and context PR being merged into API 7 recently?