Add event phases

[Technici4n]

This is an extension of the event system to allow ordering constraints between event listeners.

Design points

• No direct ordering constraints between listeners, as they cannot be identified reliably.
• No integer priorities, and no fixed EventPriority enum (or a similar limited and hardcoded set of priorities).
• An arbitrary number of event phases may be added, with soft ordering constraints w.r.t. other phases (a failure to resolve the constraints should not crash the game). Adding both soft and hard ordering constraints might also be possible, but I wasn't able to figure out how to implement them.
• Listeners may be added to any phase.
• This ordering system should only be used as a last resort. When it is clear from the start that multiple phases are necessary (for example ALLOW, ADD, MODIFY), multiple events should still be created as usual. However, the phase system allows adding ordering constraints beyond what the API author originally thought of.

Examples

Damage canceling

An example use case: some potion mod registers an AllowDamage handler for an invincibility potion, and some tech mod registers an AllowDamage handler for its powered armor. Ideally, the powered armor callback should be executed later, to avoid consuming energy when the potion already prevents the damage. This is impossible to predict when adding this kind of event.
This is how this PR would address it:

// Define a set of standard phases in the API (names just for illustration).
public static final Identifier
  FREE = new Identifier("fabric", "free"), // for free damage cancel (no cost).
  RENEWABLE = new Identifier("fabric", "renewable"), // for renewable damage cancel (e.g. power armor)
  DEFAULT = Event.DEFAULT_PHASE,// default
  SINGLE_USE = new Identifier("fabric", "single_use"); // for single use expensive cancel
// Define the event
Event<AllowDamage> ALLOW_DAMAGE = EventFactory.createWithPhases(AllowDamage.class, factory, FREE, RENEWABLE, DEFAULT, SINGLE_USE);
// Register a few listeners:
ALLOW_DAMAGE.register(FREE, (player, damage) -> return !player.hasPotion(INVINCIBILITY)); // potion handler
ALLOW_DAMAGE.register(RENEWABLE, (player, damage) -> return !(isWearingPoweredArmor(player) && tryUseEnergy(player, damage))); // powered armor
// etc...

Override the transfer API's item fallback

A somewhat involved use case, that would be useful to port TechReborn to the item transfer API: if API Lookup fallback providers were exposed as an Event<BlockApiProvider<A, C>>, a mod could override the default wrapping of Inventory as Storage<ItemVariant> in the item transfer API by registering its own fallback provider in an early event phase.

interface BlockApiLookup<A, C> {
    // Assume this is added:
    Event<BlockApiProvider<A, C>> fallback();
    default void registerFallback(BlockApiProvider<A, C> provider) {
        fallback().register(provider);
    }
}
// Default behavior:
ItemStorage.SIDED.registerFallback((world, pos, be, state, direction) -> {
    if (be instanceof MyModBe) {
        // runs after fabric's compat with Inventory, this will never run if MyModBe implements Inventory
    }
});
// With event phases:
Identifier phase = new Identifier("mymod", "before_fabric");
ItemStorage.SIDED.fallback().addPhaseOrdering(phase, Event.DEFAULT_PHASE);
ItemStorage.SIDED.fallback().register(phase, (world, pos, be, state, direction) -> {
    if (be instanceof MyModBe) {
        // runs BEFORE fabric's compat with Inventory, this will run even if MyModBe implements Inventory!
    }
});
// In practice fabric would provide this early phase, for this exact use case.

Any comment is welcome! 😄

[liach]

Should we create a new interface for these phased events? Some callbacks don't need phases, and it is a somewhat expensive feature.

For the phase ordering, maybe we can find out strongly connected components first, group them equally, and then do topological sort? So if phase a and phase b have a loop dependency, the order of execution among a and b is arbitrary, but a and b will be ran before any of their descendant phases (that is not in a loop, or descendant loops).

Also this reminds me of the strategy where you create two anchors for the beginning and the end of the phase; you create one for start of free blocking damage and one for end of free blocking. Then, when you register your listener, you order it to go after the start and before the end. In your example, you do:

ALLOW_DAMAGE.register(FREE, INVINCIBILITY_POTION_HANDLER);

except you still need to guarantee that INVINCIBILITY_POTION_HANDLER runs before RENEWABLE.

[Technici4n]

Should we create a new interface for these phased events? Some callbacks don't need phases, and it is a somewhat expensive feature.

That would prevent the feature from being adopted on day 1 though, and would require that we somehow expose the new type for most existing events.
What makes you say this is an expensive feature? I just noticed that this doubles the asymptotic memory footprint of events when many listeners are registered... I'll push a fix for this in a bit. But other than that, events with a single phase should have essentially the same performance as non-phased events.

For the phase ordering, maybe we can find out strongly connected components first, group them equally, and then do topological sort? So if phase a and phase b have a loop dependency, the order of execution among a and b is arbitrary, but a and b will be ran before any of their descendant phases (that is not in a loop, or descendant loops).

Yes that's a good idea! Doing a visit, reversing the list and then doing another visit should be enough! (That's more or less Kosaraju's algorithm, but we don't need to group SCCs together at the end). :)

Also this reminds me of the strategy where you create two anchors for the beginning and the end of the phase; you create one for start of free blocking damage and one for end of free blocking. Then, when you register your listener, you order it to go after the start and before the end.

Not sure what you mean exactly with these anchors? Do you build an ordered tree of handlers this way?
This may be similar to the "hookchain"... I've seen some commits but I'm really not familiar with how it worked.

except you still need to guarantee that INVINCIBILITY_POTION_HANDLER runs before RENEWABLE.

This is handled by createWithPhases directly, that's the point! Centralizing event phases in the event declaration (when possible) avoids each mod having to specify ordering wrt. the N-1 other mods. :)

[liach]

Hmm, now I have an alternative idea, given we can split an event phase into a start and an end:
event phases may be hierarchical; when a phase Y is enclosed in X, we have ordering like X.start -> Y.start -> Y.end -> X.end.

We can have

public interface EventPhase<T> {
  // get or create another enclosed phase within this phase/event
  // may use the same phase if non-phased, but the new listener should have the given path prefix
  EventPhase<T> subPhase(String path);
  // adds ordering between two elements (phase/listener) in this phase/event
  // may be noop if not supported
  void order(String from, String to);
  // register a listener within this event/phase
  // maybe just straight register if no phase support
  void register(String path, T listener);
  // we can optionally have a pathless register, and the listener registered is 
  // only guaranteed to be called during this phase, and order with other
  // components of the phase is arbitrary; or we can direct the register to a custom subpath by default
  default void register(T listener) {
    subPhase("default").register(/* make a path */, listener);
  }
}

Was writing the comment when you replied; now I think we can actually add these 3 methods to the default Event if you see fit, and in the default impl we will just disrespect these soft orders.

So the usages are like:

EventPhase<AllowDamage> FREE = ALLOW_DAMAGE.subPhase("free");

and we don't need to add two calls to link a listener registered to the beginning and the end of the free subsection.

[liach]

What makes you say this is an expensive feature?

Oh right, you compile an array of the actual invoked listeners after that, so ain't that slow

[liach]

except you still need to guarantee that INVINCIBILITY_POTION_HANDLER runs before RENEWABLE.

This is handled by createWithPhases directly, that's the point! Centralizing event phases in the event declaration (when possible) avoids each mod having to specify ordering wrt. the N-1 other mods. :)

Um, I think you misunderstood this part. So we have 3 tasks, A, B, and C, and orderings A -> B and A -> C. Then ABC or ACB are both valid execution orders for the tasks. Now, plug in FREE for A, and INVINCIBILITY_POTION_HANDLER for B, and RENEWABLE for C. In your example, the A -> C is defined by event itself, and user defines A -> B. You need another B -> C link to prevent INVINCIBILITY_POTION_HANDLER from running before anything from RENEWABLE.

Hence, I proposed that nested EventPhase model above, as to avoid such user errors.

[Technici4n]

Um, I think you misunderstood this part. So we have 3 tasks, A, B, and C, and orderings A -> B and A -> C. Then ABC or ACB are both valid execution orders for the tasks. Now, plug in FREE for A, and INVINCIBILITY_POTION_HANDLER for B, and RENEWABLE for C. In your example, the A -> C is defined by event itself, and user defines A -> B. You need another B -> C link to prevent INVINCIBILITY_POTION_HANDLER from running before anything from RENEWABLE.

Hence, I proposed that nested EventPhase model above, as to avoid such user errors.

Hmmm, in my example INVINCIBILITY_POTION_HANDLER is not an id but rather the callback, which I just register to the FREE phase. That might have been a little bit confusing, so I've updated the PR description.

[Technici4n]

Nested phases may be an option as well. The default phase list with the option to insert some phases should be good enough for 95% of the cases where you'd need event phases, so I'm a bit hesitant. I need to think about this a little bit more. 😄
Note that we can easily extend this PR's event phase system to subphases in the future if that is necessary.

I've noticed you used String instead of Identifier in your example. I've also considered that, but I'm not sure what is better.

[liach]

Here is another vision if we only allow one level of phase nesting (like as is)

public interface Event<T> {
  EventPhase<T> subPhase(String path);
  void order(String from, String to);
  default void register(T listener) {
    subPhase("default").register(listener);
  }
}

// we can later enhance this api to support nested phases (for easier sorting, etc)
public interface EventPhase<T> {
  void register(T listener);
}

Imo, creating an EventPhase<T> for registering events is more visible than specifying the phase in register.

[sfPlayer1]

IMO nested phases don't really add much, they are only slightly more expressive than flat ordering and that's in very specific corner cases. In general phases shouldn't see that much use overall, only a few operations like damage application really need them.

Damage handling may even need a more powerful scheme, something like IC2 armor using can't be computed properly in successive steps but needs linear distribution taking all armor pieces and other effects into account. This acts as if a later handler has to influence an earlier handler, so it really needs a property gathering step followed by computation to spread the damage across the absorbing elements and then application.

[Technici4n]

Hmmm separating EventPhase will make the overall structure clearer, and give me a place where I can put the javadoc for event phases. We can also use a more expressive syntax for ordering:

event.phase("whatever").runBefore("otherphase"); // replaces the current event.addPhaseOrdering("whatever", "otherphase");
event.phase("whatever").runAfter("otherphase"); // replaces the current event.addPhaseOrdering("otherphase", "whatever");

What about String vs Identifier? In most cases, a static final constant will be used, so it shouldn't make a big difference.

[liach]

Given these sorts are quite expensive, can we have an option to add multiple orderings at once (so the sort is only done after adding all of those ordering definitions)

[Technici4n]

If P is the number of phases and C is the number of constraints, the sort is only O(P+C), so it shouldn't be a big bottleneck. Event registrations themselves are O(N) each after all.

[liach]

But if we try to register C constraints, the current model has a total time complexity of O(C(P+C)) as you do such a calculation after registering each constraint. Often you would register multiple constraints at once, such as when declaring the default phases of an event.

[Technici4n]

Imo if this is a bottleneck then we should provide a way to register multiple constraints at once. But otherwise I'd like to keep the API changes minimal.

[Technici4n]

Ok so I've just split EventPhase into a separate class, which allows registering multiple dependencies at once (even though that shouldn't have a big performance impact @liach ). But I feel like it makes the API worse so I'll just revert it. You can look at the corresponding commit if you are interested.