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

Update all forge event code, attempt to become loader unaware #10468

Merged
merged 11 commits into from
Dec 22, 2024

Conversation

Thodor12
Copy link
Contributor

Changes proposed in this pull request

  • Introduce a new event handler in the IMinecoloniesAPI, which is backed by an interface containing basic methods for the events we want to send
  • Event handler is implemented in such a way it could be made loader unaware in the future if needed.
  • All logic for sending Forge specific events is ripped out in favor of the API event handler

Testing

  • Yes I tested this before submitting it.
  • I also did a multiplayer test.

Review please

@Thodor12 Thodor12 requested a review from Raycoms November 19, 2024 18:40
@Nightenom
Copy link
Member

Also (without looking at code if it's even possible) cannot we reuse existing forge eventbus for implementing the bus logic?

@Thodor12
Copy link
Contributor Author

Also (without looking at code if it's even possible) cannot we reuse existing forge eventbus for implementing the bus logic?

The point was from the beginning to not use Forge code where possible, so we quickly glanced over using a Guava event bus.
But I found that to be less than ideal since it's a third party library Mojang happened to insert, if that were to be dropped we'd have to fallback to shading it, which doesn't work with Guava.

For that reason I thought it was a better idea to make a small lightweight event bus tailored to our needs.

@someaddons
Copy link
Contributor

Forge eventbus is an extion of guavas afaik

@uecasm
Copy link
Contributor

uecasm commented Nov 20, 2024

What's the point of avoiding (Neo)Forge APIs? Unless you're anticipating a Fabric port in the future or something, but that can be handled with an abstraction layer rather than a different implementation.

Copy link
Contributor

@Raycoms Raycoms left a comment

Choose a reason for hiding this comment

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

Two smaller comments

…pe, rename all events to ModEvents to reduce name conflicts
Copy link
Member

@Nightenom Nightenom left a comment

Choose a reason for hiding this comment

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

Generally speaking I would avoid renaming event classes to ModEvent, just imho though

@Raycoms Raycoms merged commit 3c8a387 into version/main Dec 22, 2024
5 checks passed
@Raycoms Raycoms deleted the feature/update_forge_events branch December 22, 2024 12:49
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.

5 participants