-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add Event#hasListener
, remove unused junk
#3889
base: 1.21
Are you sure you want to change the base?
Conversation
Although this is cool, there is no reason to keep this as part of shipped code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some tests.
fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java
Outdated
Show resolved
Hide resolved
* | ||
* @return whether the event has any listeners | ||
*/ | ||
public boolean hasListener() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is a bad pattern. If you are concerned about allocation, reuse the context object. In typical modpacks, most events will be used and these checks will be pointless anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a fair point, @apple502j did you have a specific use case in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A usecase I had for this was for a deprecated event, where actually properly servicing it required setting up some expensive stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not all context objects can be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link some specific examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/BasiqueEvangelist/ne-v-seti/blob/sanity-1.17/src/main/java/me/basiqueevangelist/nevseti/OfflineDataCache.java#L30
I guess I misremembered specifically how much it would take to service this event, but I think it will still be useful for events that are deprecated and require running extra logic to run their invokers
* | ||
* @return whether the event has any listeners | ||
*/ | ||
public boolean hasListener() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not resolved, please don't mark it as resolved.
Still wating on a good usecase for this, in places where it matter we already re-use a context object, and firing an event with no listeners should have little overhead. |
I think I had some events in my mind when I filed this PR, I need to figure that out again. |
Events with non-zero invocation cost
Will do a more thorough research later. |
IMO this would only benefit if there was a separation between singular and wildcard events (like With the current implementation, if a single mod wants to modify a single loot table, all the benefits of such a check disappear. |
|
Should be self-descriptive.