Create EventResult enum, an simpler alternative to InteractionResult

[Patbox]

This pr creates an EventResult enum, which main usecase is to provide simple {ALLOW, DENY, PASS} options for events to use (both Fabric API and for other mods). Additionally migrate EntitySleepEvents from InteractionResult to EventResult as an example. Some events from interactions module might get the refactor in later pr (that module needs slightly more work).

Main reasoning with this change is that a lot of events don't need, use or implement full functionality of InteractionResults, which provides {SUCCESS, SUCCESS_SERVER, CONSUME, FAIL, PASS, TRY_WITH_EMPTY_HAND} values and additionally supports setting the resulting ItemStack. All of this functionality might be confusing, if event only really needs SUCCESS, PASS and FAIL (in which case the names aren't the best either).

Alternative to this would be to just use existing TriState enum, however I feel that it's names could be confusing in some contexts.

[Fuzss]

That's a really great change, thanks.

Personally I'd love if this could be expanded to cover events currently using boolean return values.
Right now it's often not clear and not consistent with such events whether true or false preserves vanilla behavior, prevents certain logic from running etc. Imo it would be great if this could be streamlined, like by using EventResult.PASS and an additional EventResult.INTERRUPT (or just EventResult.DENY).

This is the approach I've been using for a long time in my own library, here is my EventResult class.

[Technici4n]

Having more values than strictly necessary makes it all confusing. In fact this was precisely the problem back when InteractionResult was still an enum. What the boolean does should be obvious from the event name (e.g. allowElytraFlight), or are there cases where that can't be done?

[Patbox]

I think Fuzzs means events that return a nullable Boolean, but I'm not sure if fabric api has those

[Fuzss]

@Patbox
I meant primitive boolean return values.

@Technici4n
I don't see a parallel with any previous issues like we were facing with InteractionResult. After all EvenResult is a custom class specifically for event return values, whether there are three or four values in it all of them have a distinct usage. Nothing confusing about it.

Also Fabric callbacks are mostly used as lambdas so you usually don't even get to see the method name.
I'm just talking from personal experience here. It's confusing at times what specific return value does what in certain callbacks. Standardizing return values would remedy that.

[Fuzss]

I've also previously talked about standardizing return values for Fabric events (EventResult for boolean or TriState return values, EventResultHolder for other types of return values): #4677 (comment)
I still very much believe Fabric would greatly benefit from streamlining all that, as it makes it much clearer how to use events and what return values will yield what behavior (interrupt vs pass).

This is an idea I originally got from Architectury Api, where it's also applied for all of their events.

[Fuzss]

Here are a few examples for events where imo it's not intuitive how the boolean return value should be used, and digging into the Javadoc is required (which standardized EvenResult return values would solve):

fabric-api/fabric-events-interaction-v0/src/main/java/net/fabricmc/fabric/api/event/player/PlayerBlockBreakEvents.java

Line 99 in a208210

boolean beforeBlockBreak(Level level, Player player, BlockPos pos, BlockState state, @Nullable BlockEntity blockEntity);

fabric-api/fabric-events-interaction-v0/src/client/java/net/fabricmc/fabric/api/event/client/player/ClientPreAttackCallback.java

Line 60 in a208210

boolean onClientPlayerPreAttack(Minecraft client, LocalPlayer player, int clickCount);

fabric-api/fabric-screen-api-v1/src/client/java/net/fabricmc/fabric/api/client/screen/v1/ScreenMouseEvents.java

Line 197 in a208210

boolean allowMouseClick(Screen screen, MouseButtonEvent event);

<...>

Also not a fan of how events that "pass through" vanilla values are handled, with some of them requiring null to be returned to keep the vanilla value untouched, while others require the vanilla value to be returned which is passed as one of the parameters to the callback. A standardized EventResultHolder would solve that. Examples:

fabric-api/fabric-loot-api-v3/src/main/java/net/fabricmc/fabric/api/loot/v3/LootTableEvents.java

Line 128 in a208210

LootTable replaceLootTable(ResourceKey<LootTable> key, LootTable original, LootTableSource source, HolderLookup.Provider holder);

fabric-api/fabric-model-loading-api-v1/src/client/java/net/fabricmc/fabric/api/client/model/loading/v1/ModelModifier.java

Line 85 in a208210

UnbakedModel modifyModelOnLoad(UnbakedModel model, Context context);

<...>

[Fuzss]

And to anyone wondering how standardized event return values in the form of EventResult and EventResultHolder could look once implemented; I invite you to have a peek at the common events in my personal library, which mostly mirror Fabric events and already use EventResult and EventResultHolder everywhere:
https://github.com/Fuzss/puzzleslib/tree/main/1.21.11/Common/src/main/java/fuzs/puzzleslib/api/event/v1

[Technici4n]

allowMouseClick should be quite clear from the name, but that convention was created relatively late, so older events like in the interaction events module don't follow it.

There is some merit in what you suggest but I remain skeptical of a one-size-fits-all approach. Even NeoForge moved away from @HasResult and related functions, but admittedly there it's not super clear if cancelling is enough or if methods of the event need to be called. Usually you have to read the source code anyway ;)

[modmuss50]

I agree I dont think there a reason for having a generic EventResult enum to be used by many events. Would it not be better for each event to have an enum that is taylored for the specific event? Even if its structuraly the same as another one the docs/names could be written for the specific use case.

[Patbox]

A lot of events would end up with enums that would be 1:1 with this one if that was implemented, since the minimal {ALLOW, DENY, PASS} would work in most cases (like how InteractionResult was used before, even if slighly incorrectly).
I do think that in case of events that need more values / more specific behaviour it would make sense to just provide extra enum class rather than forcing EventResult to work with them. Also main goal for me is to replace incorrectly used InteractionResult, not to change how other boolean events work (through if it did improve them, someone could also pr that).