Fabric API

Fabric API

Mods
152M Downloads

[Suggestion]: Porting events from Puzzles Lib to Fabric Api

Fuzss opened this issue · 11 comments

commented

Hey there, I've been maintaining my multi-loader library Puzzles Lib for some years now. Among other things Puzzles Lib includes a huge collection of events ported more or less directly from NeoForge / Forge. While certainly not all, I think still a bunch of them will fit perfectly fine into Fabric Api itself.

I'm opening this issue to hopefully bring on a bit of a discussion and gather feedback regarding which events members of the community would find useful to have in Fabric Api, and also to explore some implementation details.
If there is a better place to talk about this let me know. I didn't want to post in some Discord channel though since I figured it would get lost quickly.

I'd be happy to make separate PRs later in the process for individual events.

Alright, so let's get into it. Here are my classes containing all Fabric events:
Common: https://github.com/Fuzss/puzzleslib/tree/main/1.21.6/Fabric/src/main/java/fuzs/puzzleslib/fabric/api/event/v1
Client: https://github.com/Fuzss/puzzleslib/tree/main/1.21.6/Fabric/src/main/java/fuzs/puzzleslib/fabric/api/client/event/v1

The actual event interfaces / classes are found within the common sub-project. Note that this is common code, so not all events necessarily lack a Fabric equivalent. The ones that do are exclusively found in the two packages linked above.
Common: https://github.com/Fuzss/puzzleslib/tree/main/1.21.6/Common/src/main/java/fuzs/puzzleslib/api/event/v1
Client: https://github.com/Fuzss/puzzleslib/tree/main/1.21.6/Common/src/main/java/fuzs/puzzleslib/api/client/event/v1

Please note that the API design of my events is rather opinionated: I've standardized return values (void, EventResult, EventResultHolder) and introduced mutable values for certain parameters, since I personally find Fabric's lack of standardization regarding return values a bit annoying to work with (like boolean returns sometimes cancelling vanilla when false, sometimes when true; and the worst offender events that return InteractionResult 😬).
But I will certainly adapt this to Fabric's more simple style.

Finally I'd like to mention that all events in Puzzles Lib have a purpose, meaning they are useful in at least one of my personal projects. I've not been blind-porting whatever I can find from NeoForge. So if necessary I should be able to provide existing use cases for every event.

commented

ComputeCameraAnglesCallback looks interesting. I know there are quite a few authors patching in roll on fabric, so it would probably be good to make sure the callback suits their needs also. Might be worth actually patching in roll and having a injected interface to expose it?

commented

I think there has been discussion of a renderstate API callback before, but it was decided to wait a minute to see what Mojang does?

commented

While I think a lot of these events are a good idea in general, I think that the forge implementation, or it's translation, may be patchier than it needs to be. Ideally every mod should be able to implement these events without conflicting with fabric API.

commented

Thank you for your feedback.

Would you mind sharing which events in particular you think are a good idea, and which you think are too patchy?

Also you can have a look at all the Mixins implementing these events here: https://github.com/Fuzss/puzzleslib/tree/main/1.21.6/Fabric/src/main/java/fuzs/puzzleslib/fabric/mixin

commented

I'll clarify that I'm not saying any particular event is too patchy. Just that neoforge has different goals and priorities while implementing their API, and make choices in patches that shouldn't really be made in mixin. I wrote my original comment while looking at https://github.com/Fuzss/puzzleslib/blob/0d9519c860facf5010bccf18bfcbd6973c44cf6b/1.21.6/Fabric/src/main/java/fuzs/puzzleslib/fabric/mixin/LivingEntityFabricMixin.java#L319. I've been considering this event for a while, and I don't think it needs a conditional overwrite

commented

I agree with this in general, but of course each event and its use cases need to be considered separately. Event results can also be standardized in the contribution guidelines (this needs discussion too), though existing events that don't follow the standard will have to remain as is to prevent breaking changes.

  • 🎉1
commented

I also think that the anvil result one will need discussion. I've seen the horror stories that come of that area of the code in modpacks, and whatever we could do should ideally just work.

commented

Yeah, an injected interface would be great for that.

@cputnam-a11y What do you think would be a good signature for the callback? Maybe a single context object for the three rotations with getters and setters? Or should I keep the MutableFloats (replacing them with the ones from Apache Commons)?

void onComputeCameraAngles(GameRenderer renderer, Camera camera, float partialTick, MutableFloat pitch, MutableFloat yaw, MutableFloat roll);
commented

Yeah, an injected interface would be great for that.

@cputnam-a11y What do you think would be a good signature for the callback? Maybe a single context object for the three rotations with getters and setters? Or should I keep the MutableFloats (replacing them with the ones from Apache Commons)?

void onComputeCameraAngles(GameRenderer renderer, Camera camera, float partialTick, MutableFloat pitch, MutableFloat yaw, MutableFloat roll);

Pretty sure partialTick migrated to a different type than float (it's a float here 🎉), and is called tickProgress in yarn terms.
I'm also curious if mc ships some form of mutable Vec3F. It could make it slightly more consise

commented

Ah that's a good idea, there is Vector3f from JOML which is mutable.