Enhance Visibility of EventContextKey Usage

gabizou commented 5 years ago

First, some background (there is a TLDR at the bottom, but really you should read this issue)

As the API and official implementations have evolved over the years, @Zidane @bloodmc and I are coming to a head about exposing bits of what we can expect for being able to better use Sponge's implementation for control flow of various events. As most plugin developers are aware, Sponge's tracking system does an impressive job at capturing and emitting objects for Cause, and EventContext. However, we've had a recent discussion to add more EventContextKeys to provide some insight to plugins about using ChangeBlockEvent.Pre as it serves as a "pre-process a bunch of things" event that can safely be used to cancel things like Pistons (because pistons are already requiring a rewiring/re-engineering of some bits about how captures are done...

The point

So, having given some background about how EventContextKeys are being populated, I'd like to get to the root of this discussion: We want to provide visibility via documentation and possibly default provided methods for common expected EventContextKeys that would be more than likely available for various events.

So far, we're agreeing and will be committing to adding Javadoc annotations (like @see) to various Event classes to suggest possible EventContextKeys that would be available to those events, and documenting when those keys would be optionally provided (better to sanely explain the contracts defined in the API for implementations to abide by). What is an option, and the root of this discussion is whether we, the API, will also define default implemented method accessors for these optional EventContexts that we're almost sure will exist, but still having an Optional return. This would be the same treatment that Entity interfaces went through to facilitate "guaranteed" accessors with Data API, except, because the events are still contracted to have no guarantee on what will exist in EventContext, the methods would still be Optional.

Examples

For Javadoc purposes, we'd be attaching a custom tag, named @optionally or something thereof, with a {@link EventContextKeys#FOOO} and explanation when that key would normally be provided (because of the dynamic nature of tracking, there's a multitude of combinations in which contexts can be constructed, none are contractual, all are situational).

/**
 * Base event for when {@link BlockState}s at {@link Location}s are being
 * changed.
 *
 * @optionally {@link EventContextKeys#NOTIFIER} When a User notifier is provided as indirect for the root
 * @optionally {@link EventContextKeys#OWNER} When a User owner is provided as indirect for the root
 * @optionally {@link EventContextKeys#FAKE_PLAYER} When a plugin/mod is acting or performing changes on behalf of a Player/User
 * @optionally {@link EventContextKeys#USED_ITEM} When a Player or FakePlayer has an ItemStack that is performing block changes
 * @optionally {@link EventContextKeys#LIQUID_BREAK} When Fluids are performing changes
 * @optionally {@link EventContextKeys#LIQUID_FLOW} When Fluids are performing changes
 * @optionally {@link EventContextKeys#LIQUID_MIX} When Fluids are performing changes
 */
public interface ChangeBlockEvent extends Event, Cancellable {
// -- stuff
}

As for the default implemented methods, they would look like so:


public interface ChangeBlockEvent extends Event, Cancellable {

    default Optional<Player> getFakePlayer() {
        return getContext().get(EventContextKeys.FAKE_PLAYER);
    }

    default Optional<World> getLiquidMix() {
        return getContext().get(EventContextKeys.LIQUID_MIX);
    }

TLDR

We want to expand the documented contract of which events and when certain EventContextKeys are being used. We will be adding javadocs to the event classes for better visibility, but we are considering adding optional accessor methods like the examples above for events that are more likely to contain those keys.

VOTE YOUR OPINION!

Please react to this issue description with the following reactions:

:rocket: to add default methods using EventContextKeys (Includes having Javadocs on the class with @optionally)
:eyes: to keep just the Javadocs, no default methods
:tada: to be excited about the possibilities this issue will provide for the @SpongePowered/docs to document!

EhhChris commented 5 years ago

As a noob who's never really been exposed to this kind of pattern before, I found myself looking for examples on different events and the context available to each instead of just using the Javadocs. Default methods would absolutely bridge some of this gap nicely.

Inscrutable commented 5 years ago

excited about the possibilities this issue will provide for the @SpongePowered/docs to document images

liach commented 5 years ago

I strongly oppose adding default methods. Such additions may violate the goal of entity component systems and harms flexible additions.

MarkL4YG commented 5 years ago

I agree to @liach . However, I would say that some way to provide code assistance to developers is desirable and would be provided by method definitions in interfaces. Just to make sure: do these methods have to have their default implementations in the interface or wouldn't it be sufficient to have them in their implementations?

This way, proper tool-assisted coding (and possibly deprecation) would be possible.

gabizou commented 5 years ago

I strongly oppose adding default methods. Such additions may violate the goal of entity component systems and harms flexible additions.

The point isn't that they would be guarantees, they're commonly found in the events regardless of implementation (hell, one could say it's almost a requirement in certain scenarios). Case in point: ChangeBlockEvent.Pre is fired in two scenarios:

A block update event is being scheduled (such as pistons)
Certain block physics that are complicated to track and roll back have a pre event thrown (such as liquids)

In these scenarios, we will always provide a LocatableBlock instance, or TileEntity instance, or BlockSnapshot instance.

In the case of NotifyNeighborEvent and any successive events, we take advantage of the PhaseTracker to provide a BlockSnaphot of the "final" placement, or combination there of of what block is performing the neighbor notification (the source), after which, if a block event is being scheduled, well, now the NEIGHBOR_NOTIFY_SOURCE or whatever the context key is called, will be populated in the EventContext and visibly, it's something that we'd want to make people aware as a possibility existing, instead of telling people to read the docs, debug their plugins, print out Cause.toString() etc. because they want to be able to deduce and reliably track/prevent/cancel/modify/associate changes to a world with our already powerful event system.

The goal of ECS isn't feasible in Minecraft, and likely won't be until the point when all Entity classes are data driven and composed of functions declared by their configurations. Flexible additions are fine and all, but the default methods again, are only serving as a "here's what you could potentially have with this event" compared to other EventContextKeys.

liach commented 5 years ago

sounds good. we can just model event context keys and their usages after data keys.

SpongePowered / SpongeAPI