Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport @EventBusSubscriber annotation from 1.12.2 #80

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Conversation

Lyfts
Copy link
Member

@Lyfts Lyfts commented Oct 7, 2024

In 1.12.2 the @EventBusSubscriber annotation can be applied to a class to automatically register it on the MinecraftForge eventbus. In our ancient version we have no less than 4 official eventbusses so instead it will register the methods on the relevant event bus. This works without an instance and thus all methods annotated with @SubscribeEvent in the class are expected to be static.

Registering them statically through the standard EventBus.register is not ordinarily possible in 1.7.10 so the registration part is done in house. Going about this in the same way that Forge does it (via yucky reflection) proved to have a few issues, it is both slow and will cause the entire method table to be loaded. This in turn loads the class of all the method parameters which is a real problem since you can't put the annotation behind a Loader.isModLoaded check, meaning an annotated class that has an event from a mod that isn't loaded will crash the game unless the method is annotated with @Optional.Method.

In my eyes requiring any event from a different to be annotated with @Optional.Method is not viable, which meant I had to get a little creative with how the subscriber classes are parsed. Since reflection is out of the game and mixins aren't usable for this, that left ASM as the top option. It uses a coremod transformer (which is less of a transformer and more of a reader) to comb through the methods of the annotated class finding the ones that need to be subscribed to an event.

This is overall much less disgusting than doing it with reflection and doesn't load anything from the annotated class which in turn allows for any event to be present even if the mod that it's from isn't. The method will never be invoked and invoking the remaining methods in the class won't cause the JVM to try to load that event either.

@Dream-Master Dream-Master requested a review from a team October 7, 2024 17:17
@Lyfts Lyfts marked this pull request as ready for review October 9, 2024 19:47
@Lyfts
Copy link
Member Author

Lyfts commented Oct 9, 2024

I gave it a bit of a torture test by applying it to a large part of the functionally static event handlers in GT5U https://github.com/GTNewHorizons/GT5-Unofficial/tree/eventbussub and everything seems to be working swimmingly so far.

Copy link
Contributor

@mitchej123 mitchej123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dream-Master let @Lyfts merge in case he wants to address any of the comments I left.

@mitchej123
Copy link
Contributor

One other thing: Please confirm you've tested on RFB + Non-RFB

@Lyfts
Copy link
Member Author

Lyfts commented Oct 10, 2024

It's tested with and without RFB on both server & client

@mitchej123
Copy link
Contributor

@Lyfts Good from my end - feel free to either merge, or remove the don't merge label

@Lyfts Lyfts merged commit 2da1d6f into master Oct 10, 2024
1 check passed
@Lyfts Lyfts deleted the auto-eventbus branch October 10, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants