Events and EventBus

[gritty lily]

agree on 8.

agree on 9.

[latent rivet]

No, every frame creates a new PoseStack and the partial tick is obviously different every frame, so they are not immutable over the lifetime of a globally unique event instance

[gritty lily]

agree on 10.

[inland imp]

Right.... I am stupid...... So that one for sure can not be an immutable event

[latent rivet]

Yeah, that's where the reset would come into play

[granite nimbus]

I think that resetting events will require context for places where they need to be reset. So, a universal reset method on all events is maybe not so useful. I'd say events can implement reset that takes the specific arguments needed to reset

[wheat sinew]

write this down, Tech

[granite nimbus]

So, reset to the render events would take a pose stack and partial tick

[rigid gust]

I am doing it as we speak

[gritty lily]

the goal of reset is to be more efficient than just building a new event instance

if there's complex logic involved in resetting, just build a new one.

[rigid gust]

should we have a reset method to clear isCancelable and result?

[inland imp]

Maybe, not sure it is usefull then....... Modern JVM and memory management is a tricky bitch. The event might not even be put on the heap, and then there is no real overhead IMHO, or at least not noticeable

[granite nimbus]

Yeah, but we have to provide that context for some events no matter what. And setting fields is more efficient than re-constructing and setting fields

[rigid gust]

(the relevant events probably won't be cancelable and won't have a result)

[wheat sinew]

I'd think those parameterized resets would be spec'd to require calling the spec'd no-args reset method

[gritty lily]

only marginally more luke, in some cases

[wheat sinew]

well, in the situation that we still have something needing to be reset in Event itself

[granite nimbus]

Eh, I'd say, once again, that individual events can implement that. That seems simplest

[gritty lily]

i would tend to think that cancelable and result events wouldn't make sense to even be resettable

[granite nimbus]

Given that many events will either be immutable or have extra stuff that needs reset

[rigid gust]

probably not indeed - but 2 field writes should be very fast

[latent rivet]

I doubt that the JVM manages to elide the heap allocation with the complexity of the event bus, even with the significant simplifications of the implementation we are planning

[rigid gust]

I do not mind either way - reset is not necessary anymore now that we have decided to remove getPhase and setPhase

or have we? please comment on points 11 and 12 😄

[inland imp]

Not sure, it is actually pretty smart about this, especially if it runs the same code path over and over again

But yeah it might be too complex

With too many levels of indirection and ASM hacks

[rigid gust]

a single unknown virtual dispatch and you're already dead I think

[inland imp]

Maybe......

There are some exceptions where virtual dispatches work, especially on hot codepaths

[rigid gust]

truly virtual dispatches? it should be fine with devirtualized dispatches, but I think it has no chance of eluding the allocation with something like a listener list

[gritty lily]

IEventBusInvokeDispatcher i think is there to allow posting in some weird way using lambdas

btw

[rigid gust]

I'm not sure - from my testing it doesn't affect performance when it's not used, so I don't mind keeping it

(I didn't test providing a custom dispatcher)

[gritty lily]

the idea iirc is that the listeners get transformed by lambda to one of those

i remember there was a use case for this at one point

i suspect it got removed tho heh

[rigid gust]

yeah the lambda can wrap each listener and do whatever - e.g. you could profile each listener (with massive overhead)

[gritty lily]

if its not used in forge or fml anymore, it's probably not important

i think it might have been used for some sanity checking at one point

[rigid gust]

we could say that I audit forge and fml first and remove it if it's not used (in practice I remove it and look for compile errors 😄 )

[gritty lily]

lol yeah

[rigid gust]

official answer for 12.?

[gritty lily]

me: yes, remove it

12 is good.

[latent rivet]

One very low-priority wish I'd have is an easier way to ask the bus for a purely informational listener list for a given event as it would allow better debugging in certain cases without having to cause a crash in a listener to have the list printed in the error

[gritty lily]

hmmm yeah

[rigid gust]

maybe something like String getFormattedListenerList(Class<? extends Event> eventType)... exact method name TBD

[humble junco]

Bus.printDebug(logger)

[wheat sinew]

printDebug(PrintStream)

[latent rivet]

Yeah, something like that would do the trick

[rigid gust]

would you just print it to stdout / log in every case? String is probably the simplest to consume in all circumstances

[wheat sinew]

make sure, if such a method is implemented, to explicitly specify the contents being a implementation-specific human-readable blurb, akin to iirc the spec for Object#toString()

just so no mod gets the smart idea of regexing that string for info

(and thinking they're fine)

[latent rivet]

I can see it being logged but also the case of just doing a quick evaluation in the debugger window

[rigid gust]

ok I see

I will cross off 7. from the list because it is not needed with the removal of getPhase

[outer steeple]

lex made use of it for the new MONITOR phase

[rigid gust]

for reference: https://github.com/MinecraftForge/EventBus/commit/c510e2cc196ae917c6221d7346aefdaa345086b6
personally I think it's a bad idea in general

[wheat sinew]

hmmm yes, the MONITOR phase
it reminds me of my Bukkit days

[gritty lily]

wtf is that for. poor lex.

[rigid gust]

the idea was that some pre events are cancellable, but some pre handlers expect the corresponding post handler to always fire

example:

pre event:
- push new matrix
- cancel render

[ canceled ] render thing

[ canceled ] post event:
[ canceled ] - pop matrix

so you get a fucked up matrix stack state

but if you did push new matrix in the new MONITOR phase then that issue could not occur, because it would not be possible to cancel the event after that

personally I think we should instead have three events for such cases:

• allow event, cancelable
  (check if action is allowed)
• pre event, not cancelable
  (do things before the action happens)
• post event, not cancelable
  (do things after the action happens)

[latent rivet]

I would say that proper use of priorities and/or case-specific safeguards are a better approach than a generic special phase. Your suggestion with another event would be one way

[gritty lily]

I would agree

A test event that checks before firing a do it event

[rigid gust]

cool

then that leaves us with proposed change 5 to discuss

[gusty dove]

just wanted to check, with whatever reworks are proposed here, will there still be the ability to call the list of listeners manually (e.g. for exception control)? Right now it can be done with event.getListenerList().getListeners(eventBusID)

[rigid gust]

no - why is that useful?

note that ListenerList is not in the API package

[gusty dove]

I would use it to fire a different ModelBakeEvent instance to each listener in 1.12 & current versions, currently I don't need it since it's fired on the mod bus, but that may change again in the future. Also I have a patch in mind for 1.18 that will require the ability to catch exceptions from specific listeners

So the two use cases are: catch exceptions from specific handlers, or send a different event object to each handler

[rigid gust]

hmmm that is super hacky

[gritty lily]

that sounds super gross

what's the detailed use case? is there a broken event handler?

[rigid gust]

the use case is to implement dynamic (i.e. on-demand) model loading and it requires hacks to work around the problematic design of ModelBakeEvent

[inland imp]

Wtf, why would somebody do this?!?

What is the net performance benefit from this?

[rigid gust]

it is a MASSIVE performance boost, trust me on that

that is the key feature of ModernFix! 😄

[inland imp]

I see no reason why that objectively can ever be the case..............

[rigid gust]

because baking all these models takes forever and uses a lot of RAM

[inland imp]

Yeah but they have to get baked eventually .........

So you are trading speed up time

[indigo cloak]

not necessarily

[inland imp]

For a much much worse frame time experience

[rigid gust]

no, most of them are unneeded - and you can remove those that haven't been baked in a long enough time to save RAM

Orion, ModernFix proves that it works - that should really be the starting point of the discussion, IMO

[indigo cloak]

if a given blockstate never actually gets used on the server then if the model is lazily-loaded it'll never get loaded

and in a bigass modpack where people aren't even using all the available mods then there might be a lot of blockstates that don't actually need their models loaded

[inland imp]

Wheter it works annecdotally, is not really proove that it works........

I heard the same thing for sodium

And optifine

Yet neither help my system at all

[rigid gust]

@gusty dove can you provide some numbers

[inland imp]

So I stopped taking that argument as a valid reason why something works

[inland imp]

Sure but then it has to be a fair comparison

And the total loading time needs to be considered

Not just the "I get to the world time"

And I want to see frame time flame graphs

Cause they will be horrible with stuff like this

Cause that is the trade of you make with stuff like that

[indigo cloak]

again it's important to consider that a lot of models aren't just having their loading deferred until later, they're having their loading deferred until never

[rigid gust]

I trust embeddedt to know what he's doing

[gusty dove]

this is not correct

[pale grove]

i can indeed confirm that just ModernFix's caching allows the game to load and run on a Chromebook, where just Forge by itself is incapable of loading due to baking everything at once

[gusty dove]

as most models are baked by the chunk meshing threads

not the render thread

[inland imp]

Then get me the numbers, and I will even consider implement that in forge it self

Cause I think it is in principal a great idea

Cause model baking is the single worst idea mojang had......

[gusty dove]

I would need someone else to do frametime graphs as my frametimes are horrible even without any of my changes lol

but I can get numbers on loading time - I believe I even provided some in #neoforge-github a few months back, will see if I can find them

[gritty lily]

this needs to be a separate discussion thread. but it needs to be a discussion thread

[inland imp]

Yep

[gritty lily]

because i think the hunch here sounds very valid

most models are never used

[gusty dove]

yep

there is a lot of sorcery being done to pull it off, especially pre-1.19.4

but it works

[indigo cloak]

I think modernfix also unifies model objects? so if two blockstates have the same model then it makes them actually have the same model instead of two copies of an identical model

[gusty dove]

nah, I didn't bother with that optimization

that's ferritecore's department

[indigo cloak]

ah I'm thinking of something else

yeah that one

[gusty dove]

also deduplication is not as essential with dynamic model loading, since there are far less models loaded at once anyway

[rigid gust]

#1140671853174734941

in any case the conclusion is probably that getting to the listener list is not a feature that we need to expose

[outer steeple]

@rigid gust can you do a fabric vs forge benchmark again with all of the changes

for posterity

[rigid gust]

^ no reason why this shouldn't still be up-to-date

"poster" benchmarks assume some sort of event poster than is cached and allows getting to the listener list faster

[outer steeple]

well the removal of setPhase will have some impact

since that's 5 more listeners that make registration slower and call a method that assigns a field. it's not an actual impact but it's something

maybe also PR that benchmark

[rigid gust]

we kinda need the phase listeners for the post overload that has a fixed EventPriority

also, I think this test only used a single phase, so that is only 1 more listener - kinda negligible 😄

[outer steeple]

skrem

wait how did you implement that

I also wonder where all of that slowdown comes from for single listeners

[rigid gust]

https://github.com/neoforged/Bus/pull/12/commits/c2cde4c96f2ab4b0d12ca110d1722ede202a5a8b

[gusty dove]

technici4n jumpscare

[haughty current]

That man has seen terrible things.

[rigid gust]

it's a fine solution if we assume that dispatching mod bus events is not performance critical

indeed I have 😛

[outer steeple]

it may actually just be the class checks

[mellow hearth]

Like EB internals?

[outer steeple]

also @gritty lily we should probably start EB v7 and start pulling techs changes

[rigid gust]

I'll make a clean PR soon-ish

[outer steeple]

"clean"?

[rigid gust]

probably just the variable reads and the branching

[outer steeple]

au contraire, it should be split across different commits imo

[rigid gust]

true

then https://github.com/neoforged/Bus/pull/2 should be the first to be reviewed

[outer steeple]

I can start a v7 branch in a bit

[rigid gust]

I'd wait for cpw's approval - he'll probably want to look through the code carefully

[outer steeple]

I didn't say pull, I said start :P

as in, crate the v7 branch, push the tag, make the changelog use the v7 branch and probably add a github action to run and upload jmh results as the "dummy commit"

[gritty lily]

why a branch?

we don't do that anywhere else but neo itself

that makes stuff super complex

[outer steeple]

not really? the old branch doesn't need to be updated

[wheat sinew]

i see maty is advocating for our adoption of git-flow /jk

[rigid gust]

that could all just go to main indeed

[outer steeple]

just used as a reference for old stuff

it makes it cleaner, you don't even need to maintain the old branch, just for archival's sake

[gritty lily]

no. that's piss poor practice and makes long term maintenance annoying

this stuff should go on main, unless we need to fork history for some reason

[inland imp]

That is what a tag is for.........

[outer steeple]

how is long term maintenance relevant, I just said you don't need to maintain the old branch

[gritty lily]

that is exactly why we tag stuff

well then why do we have a random branch point in our git history????

also yes, this is git flow and i hate git flow

[outer steeple]

I hate you ||/s||

[gritty lily]

seriously, we shouldn't be branching without a reason.

this stuff will need a "root" commit to tag, but that's about it

[wheat sinew]

we can make a branch in the future if it's even needed

[gritty lily]

exactly. we did it before for modlauncher

[outer steeple]

u do u ig

[wheat sinew]

also, mmm me like git commit --allow-empty

[gritty lily]

"tagging v8" or whatever 😄

[outer steeple]

no don't make the commit empty

at least fix the publishing location and branch offset smh

[wheat sinew]

thank you for volunteering, maty

I expect the push in an hour's time /jk

[gritty lily]

yes, as we push changes, we should be fixing up metadata

[outer steeple]

I volunteered already? lol

[rigid gust]

how does this work with semver? I guess we just don't care

[wheat sinew]

then I expect the push in an hour - however many minutes have passed since then

SemVer? we don't... do that here

[outer steeple]

ah yes, the greatest sin of forge

[wheat sinew]

SemVer requires maintainer-controlled versioning

[outer steeple]

semver

[wheat sinew]

which literally no Forge/NeoForge project does, as far as I'm aware

[gritty lily]

what's wrong with semver?

[outer steeple]

forge doesn't do semver lol

no forge project does semver

[gritty lily]

i tried to do it for all the ones i own

[outer steeple]

that means pushing a new tag for every new feature

[gritty lily]

but it turns out the boundaries are kinda fuzzy 😛

[rigid gust]

it's more about starting a v7 branch

[wheat sinew]

to properly adhere to SemVer, we'd have to control versioning manually in some way or form, instead of <tag>.<# of commits>

[rigid gust]

the breaking changes would be spread over multiple commits which would all be 7.x

[gritty lily]

meh

it's not that big a deal

[rigid gust]

not that we should really care - only Forge consumes EventBus

[outer steeple]

forge abuses the patch version so it's inherently not semver

[gritty lily]

few things consume bus

neo is definitely not semver tho

hey at least it's not two numbers that bump in sync all the time anymore

[outer steeple]

honestly I'd make gradleutils more smart and bump minor on commits that contain [minor] or whatever if we really don't want maintainer controlled

but w/e

[gritty lily]

what's minor?

that's the question

[outer steeple]

the second number, the one you're meant to bump every new feature

[rigid gust]

<tag>.<commit #> is by far the simplest to manage 😄

[gritty lily]

i know. but what does it mean to be a minor bump?

[wheat sinew]

that would mean walking revs all the way back to the last tag, which is for my plans of introducing CLI-powered GradleUtils

[gritty lily]

what does "new feature" mean?

did me changing 16 to 17 mean a major, or a minor, or just a patch bump in modlauncher for example

[outer steeple]

everything that's not a bug fix or hotfix

that should have been a major

because it's breaking

[rigid gust]

but it's so minor

[outer steeple]

but it's the most breaking one :P

the library can no longer be used on java 16 so it's breaking

[gritty lily]

it's still a minor because nothing else changed

[outer steeple]

it's not because it is breaking

it broke backwards compat

doesn't matter in what way

[gritty lily]

and that's my point. what is "major" vs "minor" vs "patch"

given we use none of the capabilities of semver to autopull small changes

we use literal fixed points

anyway

[outer steeple]

if the library can no longer be used in the exact same setup with the new version it is breaking and that's a major. full stop**.**

[gritty lily]

the point is that semver is annoyingly fuzzy. and your definition of breaking is not necessarily what everyone else agrees

[gritty lily]

that would apply to almost every single change

[outer steeple]

no? I said new on old, not old on new

[gritty lily]

so we'd be on major about 250 by now

it makes the whole idea of having major.minor.patch a mockery

most changes to modlauncher are breaking in some subtle way

they don't break, because they're not supposed to. but there will be pathological corner cases.

my point being, almost everything we do is breaking in some way.

consider the bus changes

each one will be part of a whole

[indigo cloak]

this is why I hate semver, it's too subjective

[gritty lily]

but by your rules, we need to major every single one because each is going to break

[outer steeple]

semver is more about binary breaking than behaviour breaking

[gritty lily]

so we'll be on about major 15 or so when done

[indigo cloak]

so my own mods' version numbers only describe what broke and what you need to do to unbreak it

[gritty lily]

semver does NOT make that distinction

[outer steeple]

MAJOR version when you make incompatible API changes

[gritty lily]

so yes. every one of the patches from tech to bus requires a MAJOR change bump

so we're going to be on about 15 by the end of it

[outer steeple]

that implies you publish each of them

[gritty lily]

the only way to get the PR is to publish 😛

[outer steeple]

you can fix that by making everything until the last commit a beta for example

[wheat sinew]

I note that this is why I specifically noted earlier that SemVer virtually requires maintainer-controlled versioning
the maintainer is the one who makes the distinction of breaking vs not, and who needs to keep what chages do when preparing releases and developing
i.e. the maintainer is conscious to gather as much breaking API changes as they can for a single major bump at some future (but not too far) point, and does development carefully to avoid breaking changes until they've collected what they deem to be enough

[outer steeple]

because that's what it is in reality

[inland imp]

I feel this discussion is pointless

And a literal waste of all our time.......

[gritty lily]

it is 100% pointless navel gazing

and totally wasting our time

[inland imp]

We have been doing this kind of versioning for 10 years

[gritty lily]

we use "semver" - like versioning

[inland imp]

Given that it will be fine if we do it for the next 10 years

It is not really something we should be discussing......

[gritty lily]

it's NOT strictly semver, and there are occasionally breaks in minor versions

but it IS nicely automated

which means it's not easy to fuck it up

[indigo cloak]

"we don't use semver but we do use semantic versions"

[wheat sinew]

I don't believe tradition should be the main reason by which discussions should be pooh-poohed

[indigo cloak]

that's the other reason I hate semver, the name's too generic

[gritty lily]

i don't agree that throwing shit out because it's not compliant to some artibrary standard is a good idea either

[inland imp]

That is the reason why this discussion should be pooh-poohed @wheat sinew #

[wheat sinew]

and just to add, my opinion is and always has been that we should move closer to maintainer-controlled versioning -- not specifically SemVer

[inland imp]

Which was already clear from the get go.........

[gritty lily]

maintainer controlled versioning is risky and error prone

and would require us to start using things like "beta" and "alpha" markers on published versions

[wheat sinew]

this is something iirc Curle has said to share -- the idea of moving to maintainer-controlled versioning or, at the very least, not build-per-commit

[outer steeple]

so is pushing breaking changes as patch versions..

[wheat sinew]

err, to clarify, release-per-commit

[gritty lily]

this is not the channel to keep discussing this

[inland imp]

Were was this discussed?

[outer steeple]

anyways

[wheat sinew]

(since I recognize build can be construed as CI building)

[outer steeple]

wrong thread

[wheat sinew]

yeah, not the place for this in 'ere

so let's travel back to the start of this

who's making the commit and tag

[outer steeple]

I need to eat so unless someone else snips me I can do it

can we change the module name as well while we're at it

[wheat sinew]

I'd like to point to it, but the fact of the matter is that I can't remember where it happened, nor is Discord the best platform to hold such conversations and remember them long-term
(I bring up the idea of forums/discussions once more)

[rigid gust]

Not sure about that bit (module name)

[outer steeple]

(it was FC)

[outer steeple]

@gritty lily

[gritty lily]

you can change the name if you want. it'll need updating elsewhere i think

[outer steeple]

I believe the forge args will need changing

anything else?

[outer steeple]

@wheat sinew why's bus in #neoforge and not #toolchain?

@rigid gust doesn't this need a new guard inside the synchronized block as well? https://github.com/neoforged/Bus/pull/2/files#diff-773feae400ebe519c12b7ee4b0c94f10ad17c5f9209acaf98c893796768d2c1cR31-R34

something something thread caching

[wheat sinew]

blame my past self

[rigid gust]

Not necessarily - it doesn't matter if the map is created twice - so it's really up to personal preference ^^

[outer steeple]

eh fair enough

the lack of the null check makes it unnoticeably faster so ig

[rigid gust]

I'll probably add the check it's slightly less gc work 😛

Oh well whatever no need ^^

I'll also clean up the tests to remove all of that commented code

[pale grove]

My brain just went like "WAIT, we want more commented code!"

And then I realised you meant commented out code

[rigid gust]

yes 😄

[wheat sinew]

mmm yes, remove the comments, keep the code

[granite nimbus]

Remove the code, keep the comments

[wheat sinew]

I see you're a fan of the specification-first programming design process

[analog sundial]

babe, new code style just dropped, commented whitespace

[granite nimbus]

Comment the code, then use algebraic proof solving to show that your code matches the comments

[outer steeple]

what did I say about math majors

[rigid gust]

the rebase Just Worked™️... love git

[rigid gust]

ok PR is fully ready now I hope - did some test and JMH cleanup

[kindred brook]

Listeners.get(MyEvent.class)
 .addListener(e ->{})
 .addListener(e ->{})
 .addListener(e ->{})
 .addListener(e ->{})
 .addListener(e ->{});

If the issue is having to have a Class for every time you want to addListener for x Class, this could work

public class Test {
    public static void main(String[] args) {
        test((String string) -> {
            System.out.println(string);
        });
    }

    @SuppressWarnings("unchecked")
    public static <T> void test(SafeConsumer<T> consumer) {
        boolean passedA = consumer.safeAccept((T) "Test"); // Pass
        boolean passedB = consumer.safeAccept((T) (Object) 1); // Fail
        System.out.println("PassedA %s".formatted(passedA));
        System.out.println("PassedB %s".formatted(passedB));
    }

    public interface SafeConsumer<T> {
        void accept(T t);

        default boolean safeAccept(T t) {
            boolean sucess = false;
            try {
                accept(t);
                sucess = true;
            } catch (Exception ignore) {
            }
            return sucess;
        }
    }
}
``` couldnt you do smtn like this?

[rigid gust]

Not sure what you're trying to show here 😛
We're keeping typetools

[kindred brook]

Yea

[rigid gust]

In the pins you can find the list of planned changes

[outer steeple]

also can we remove the stupid result enum. it's way too generic and causes a lot of confusion, each event that uses one should provide its own enum with descriptive names and docs

[rigid gust]

added to the wishlist at the bottom of the hackmd

let's go through the approved changes first 😉

~~go review PRs ~~

[royal veldt]

Remind me: yes or no generic event?

[rigid gust]

deprecated for removal

[royal veldt]

So I can yeet from CrT and simplify stuff I guess

[rigid gust]

it is only used for caps in Forge

[humble junco]

was also used for registries in the past but that one's been gone for a while

[kindred brook]

Couldnt you subclass AttachCapabilitiesEvent

Or somehow get rid of the need for generic

[humble junco]

subclass wouldn't work

because you can declare new object types for which capabilities can be attached

but the same solution can be used and have one general Attach event filtering by class, which is what the generic event does for you

[kindred brook]

hm

[rigid gust]

Subclasses would work fine

But we're getting rid of the attachment event entirely...

[agile slate]

so this situation creates a dependency between the bus rework and the cap rework

[rigid gust]

No because generic events are deprecated but they're staying for now 😉

[agile slate]

ew

[kindred brook]

ew indeed I guess

[rigid gust]

It doesn't really hurt to keep it 😛

[outer steeple]

https://github.com/neoforged/Bus/pull/11/files#diff-a908af7920a385fa76e83acf6e2eddcd51c9511ad4b6e6654e1a9cf3ecf65484R19-R20 @rigid gust since the predicate is run every time an event is posted, I'm thinking you could make a custom interface ClassFilter { boolean test(Class<? extends Event> type) } to avoid the casts of the generic

probably microoptimisation land but worth considering

[rigid gust]

that's not in the hot path (checkTypesOnDispatch is usually false), so I'd prefer readability 😉

it needs to be reworked a bit cause the error message supplier is ugly

[outer steeple]

I'd combine the test and error messager

[rigid gust]

the error message probably doesn't need to receive the class, so it could be a fixed string

or we make the tester throw an exception yeah

[rigid gust]

updated

[kindred brook]

Perhaps remove EventPriority enum and convert into integer?

Enum -> Int

0 would be normal

[analog sundial]

ah, yes, lets make the object that conveys meaning into some arbitrary integer :)

[spice sphinx]

mixin injectors have int priority, its not a horrible system and it is more flexible

The issue becomes providing guidance on what priorities to use

[pallid kraken]

0 is default, increments of 1000 are equivalent to going up by one enum constant in the current priority enum

However, using the enum as the guide is not a good idea since it would be replaced

[rigid gust]

Toposorted phases are a better approach

[pallid kraken]

True, those are just as flexible without being so arbitrary

[kindred brook]

Yeah

public class EventPhase {
  private final ResourceLocation ID;
  public EventPhase(RL ID) {
      this.ID = ID;
  }
}

IMO the EventPhase is what you use

Which converts into an integer based on configurations

This integer changes based on that configuration

Integer is useful for sorting

Perhaps a PhaseManager would be needed to make an EventPhase

[granite nimbus]

There's no need for an integer though

You just have a list of phases or priorities or whatever that you assemble into a sorted array

And then listeners are sorted based on that

[kindred brook]

With arrays?

Object[Phase][]?

No

You need an integer for that

[granite nimbus]

Huh? Confused why you'd need that. Phases depend on each other, and thus are ordered. An ordering of phases results in an ordering of listeners. You're not really working with an integer at any point in time, just a sorted list of phases

Which you use to sort your listeners into an array

[kindred brook]

Hm

[granite nimbus]

Basically, no more integer based than the current system - just use a partial order of phase objects instead of an enum

[rigid gust]

Fabric has event phases, you can have a look

[honest timber]

what is event phase

[rigid gust]

see https://github.com/FabricMC/fabric/pull/1669

[honest timber]

Fabric's phase system is basically useless due to a lack of useful default phases

[rigid gust]

first time I hear that?

[humble junco]

I haven't looked deeply into it but

if you need to run before/after something else

you know what the thing is you want to run before/after

[rigid gust]

yes, either mod developers can talk among themselves or standard phases can be added per-event if people request them

[humble junco]

if you want to add a custom phase to an existing event, is there a "DEFAULT" phase you can specify as a relationship?

I think yes right?

since that PR mentions a DEFAULT_PHASE

so it, in fact, does have default phases :P

[rigid gust]

yes it has a single default phase that is also what you register to when you don't specify a phase ^^

[honest timber]

i don't understand phases

i mean, why not use properties or other stuff

[rigid gust]

what does that mean? please provide an example

[honest timber]

wait no, i mean prorities

https://cdn.discordapp.com/emojis/1120679828279017542.png?v=1&size=48&quality=lossless

[rigid gust]

forge priorities are hardcoded to 5 values, fabruc phases are a lot more flexible (there can be any number of them, and multiple mods can define phases that are relevant and specify how they come before/after other phases)

[indigo cloak]

but then you have to compile against the other mod to make your stuff run after theirs

(or are phases just strings)

[rigid gust]

they're just reslocs

[indigo cloak]

ahh okay that makes sense

[rigid gust]

I just realized we can't use this approach in forge due to mod event busses

we can't coordinate phases across multiple busses unless the set is fixed

[honest timber]

in my event system they are integers with premade constants to use if you want

lol

[outer steeple]

fabruc

[rigid gust]

you'll notice that u and i are next to each other on a keyboard 😛

[indigo cloak]

"just shove half the vowels next to each other in the corner, it'll be fine"

[royal veldt]

Maybe they use Dvorak

[honest timber]

wait, is phase system something like mixin where you want to mixin in some place in function or something

[spice sphinx]

just do integer priorities 8hed

[honest timber]

yeah

[pastel ember]

just rename your jar file

[kindred brook]

Unless you handle phases first before doing anything....

[rigid gust]

Not really, the event busses are completely independent

[kindred brook]

Like a RegisterPhases thing

[rigid gust]

That would bloat every event 😛

[kindred brook]

Nono

Well

Why not have Json based phases then

So it doesn't need to load anything just the json

Json would define phases

[rigid gust]

???

[kindred brook]

Wdym ???

Data Driven Phases

[analog sundial]

sounds silly and needless

[kindred brook]

{
  "mod:name": {
      "order":"before/after"
      "phase": "forge:default"
   }
}

Wdym you cant use Phases because of modbus's then???

[humble junco]

they mean that because each mod has its own mod bus, phases wouldn't run in a sequence

mod1's "after" phase may run before mod2's "before" phase

[kindred brook]

So you load the phases first

So its all set in stone

[humble junco]

that's not at all what I just said

[kindred brook]

Then handle listeners adding/posting

[humble junco]

it's not that you cna't have phases

[kindred brook]

Ik thats not what you said

[humble junco]

it's that the phases wouldn't run in phase

[kindred brook]

I was stating you could load the phases first

So that the order is set

[analog sundial]

that's not the problem

at all

[pallid kraken]

Is any sort of priority system useful for mod busses then?

[kindred brook]

Ok then the issue persists on Fabric?

Because mods exist there

[humble junco]

fabric doesn't have a separate mod bus for each individual mod container

[analog sundial]

i doubt it, they should be self contained afaik

[outer steeple]

fabric isn't centralized to begin with...

[kindred brook]

Ok what Im saying is before anything is ever done with Listeners and bus stuff,

Make it so Phases are ALL SET IN STONE
So if a mod has a phase they want to register, theyd make a json or whatever to do so.

[outer steeple]

the only arguments I've heard for it is registration ordering

and since 90% of the events on the mod bus are registration events..

[analog sundial]

we know what you're saying mango, it just doesn't solve anything and frankly complicates what can be done just fine in code

[kindred brook]

The problem your saying is mod C may load before mod a, but mod a needs to have events done before mod c

[outer steeple]

I fail to see how datadriven solves anything during mod init?

[kindred brook]

So what is the god damn problem with mod bus's?

If its phases not being registered in the order that its wanted then data drive it

So you load phases before ANY MODS LOAD

[analog sundial]

mod B may use a listener with a phase that is supposed to be before mod A, but because the mod bus goes from mod A then mod B etc, it's impossible to do

[kindred brook]

What?

Then dont make it do that

I dont get the issue

You can do whatever, but your saying you cant?

So have the Mod bus go from Phase A, B, D F, E, Z, W

Whatever order is needed. Based on Phases

[pallid kraken]

What exactly would be ordered?

[outer steeple]

you have to track the phases globally is the point of the whole argument..

[outer steeple]

your items before another mod's I suppose?

or if you want to register last

[rigid gust]

@pallid kraken we can fix phases for mod busses

Assuming we keep the current enum

[pallid kraken]

I thought that was already handled by the mod load order

[outer steeple]

it is

but not the "register last" part

[rigid gust]

See https://github.com/neoforged/FancyModLoader/pull/8

[outer steeple]

which a lot of dynamic mods run into

[pallid kraken]

So listeners with a single priority are grouped and each group is ordered by mod load order?

That seems to be what the changes do

I think that's reasonable

The discussion now is then how to make this also work but with RL-based phases instead of enum-based phases?

[rigid gust]

There's no plan to move to RL phases atm

I don't think it's needed

I just mentioned them as something that was done somewhere else

[pallid kraken]

I assume that the current plan is to keep the priority enum then

[rigid gust]

Yeah

[granite nimbus]

That is not the problem here. The problem is that mod buses fire stuff in parallel or in mod ordering so phases are basically meaningless

[honest timber]

not really

you can break them into separate lists for every phase

and then run them in parallel

[rigid gust]

Dude...

[analog sundial]

lol

[outer steeple]

[steep narwhal]

saw that as well

[dark willow]

I mean what did you expect from a Mango with anger issues ¯_(ツ)_/¯

[visual maple]

can i request that in these event bus changes the getter on FMLModContainer moves to ModContainer?
and implemented per language provider, to allow for easier looking up the mod bus for a given mod

this would allow the following code to be valid
ModList.get().getModContainerById(ownerId).map(ModContainer::getEventBus)
rather than the following, which for obvious reasons does not work for all language providers (kotlin)
ModList.get().getModContainerById(ownerId).map(FMLModContainer.class::cast).map(FMLModContainer::getEventBus)

this would be useful for both architectury and my own usages for registering common mod events
currently there needs to be some "hacky" method to register mod buses to a backing list
see architectury EventBuses
which can be called back on to grab the desired mod bus
would be much better if can just ask the mod container for its event bus directly

[granite nimbus]

Not every language provider needs to provide mod busses through that container though, right?

So you can't really move it there without forcing language providers to do that. Which... Isn't always a good idea

[visual maple]

what language providers dont need mod buses?
wouldnt all providers need to provide one, since the registry events and such are fired on the mod bus
no mod bus you cant register stuff or do common lifecyce event stuff

[granite nimbus]

Language providers may wrap the bus in their own thing before exposing it to mods

[visual maple]

if thats the case, can there be some way to look up the mod bus given a mod id then
as thats basicly what im asking for
just thought all mod containers would have one, so thought adding helper on the container would be best play

[granite nimbus]

@outer steeple does GML screw with event buses in a way that this would affect? I'm not entirely sure how that chunk of it works

[granite nimbus]

I think the worry is basically that you have no guarantee that a given mod has a mod bus

Like, a low-code mod? No reason for a mod bus there

[visual maple]

i mean the method could return optional
if mod has a bus, its none empty, if not its always empty

[granite nimbus]

Sure? Could also make a subclass of ModContainer, ModContainerWithBus or something, that has that and that other stuff extends. I mostly worry about polluting every mod container with something that conceptually only makes sense to some of them

[visual maple]

sub clasing is what im trying to wrok around currently
dont know all the sub classes that have a mod bus so cant easily instanceof / cast to get the desired mod bus method

[granite nimbus]

No that's my point. If it extends that subclass, it has a mod bus. If it doesn't, it doesn't. Language providers that expose a mod bus would extend that

[visual maple]

until theres that one language provider that ignores the class and extends MdoContainer without extending the WithBus variant

cant trust everyone to implement/extend all the required stuff correctly imo

[granite nimbus]

Sure. They could choose to return null from the method you're proposing and expose it another way too

[latent rivet]

You can't expect mod busses from custom language providers to be EventBus busses, they could be something completely custom, so your entire point is moot

[granite nimbus]

Basically, adding that method would make an implicit assumption that every language provider acts like the Java one

Which is just not a good assumption

[visual maple]

oh ok, i just assumed since registration and other common mod life cycle events all happened on the mod bus
there must always be one hidden somewhere no matter the language provider
how else would mods listen for all those events for life cycle stuff and object registration?

guess il continue to to use EventBuses like system for my usages then
on mod init notify my backing systems of my mod bus for event registrations
was just wondering if neo could provide some way to lookup this bus rather than passing it around everywhere

[granite nimbus]

Keep in mind that stuff like lowcode is still a language provider. And that the event bus system is rather unintuitive to use in many non-java languages due to its use of typetools, meaning that you have to intentionally avoid any of those methods and use certain overloads

(the typetools-using methods, which you use all the time in java, rely on runtime reified generics and can easily break on non-java languages if they don't treat those like Java does, which there's no requirement that they do)

[outer steeple]

@gritty lily remember to postpone getting hit by a bus to after you look at the bus changes 😛

[gritty lily]

soon. ok

is there any chance of a summary report of what's been recommended?

[outer steeple]

soon to which part

the one where you review or the one you want to check out the inner workings of a bus

[gritty lily]

i have no idea or recollection of what you're talking about.

[outer steeple]

the hackmd?

[gritty lily]

what hackmd?

[rigid gust]

The hackmd is still up to date

See pins

[gritty lily]

i'll look tonight. maybe. hopefully.

[rigid gust]

I was thinking of requesting subproject lead for this project and implement the items that we agreed on

Otherwise a good start is to look at my 3 Bus PRs

[rigid gust]

next up we have: https://github.com/neoforged/Bus/pull/11

can somebody have a look?

the concept is that we can do this:

        var modBus = builder.get().markerType(IModBusEvent.class).checkTypesOnDispatch().build();
        var forgeBus = builder.get().checkTypesOnDispatch().classChecker(eventClass -> {
            if (IModBusEvent.class.isAssignableFrom(eventClass)) {
                throw new IllegalArgumentException(
                        "The Forge bus does not accept IModBusEvent subclasses, which " + eventClass + " is. Post it on the mod event bus instead.");
            }
        }).build();

this will ensure that IModBusEvent listeners cannot be registered to the Forge bus

[light mantle]

but we can register "regular" listeners to the mod bus?

[rigid gust]

no

that is already something you cannot do at the moment

[light mantle]

ah ok

[rigid gust]

gonna look into ripping out ASM step-by-step now

first step: https://github.com/neoforged/Bus/pull/14

(please review)

[outer steeple]

@rigid gust address my comments on #14, then it can be merged and you can update the package to net.neoforged

[rigid gust]

Will do tomorrow evening

[rigid gust]

@outer steeple hi please check the changes again

[outer steeple]

why did you change the warmup time

[rigid gust]

ffffs sec

check again ^^

[outer steeple]

https://github.com/neoforged/Bus/pull/14/files#r1335236858 wdym

[outer steeple]

I was hinting towards making this construct with a load factor of 1

and this

[rigid gust]

Hmmm

Load factor of 1 is terrible for hashmaps

(should be illegal)

[outer steeple]

well then account for the load factor when constructing a new map?

or add a Function<Map, Map>

[rigid gust]

is new LockHelper<>(size -> new HashMap<>((size + 2) * 4 / 3)) fine?

[outer steeple]

does head maths

sure

[rigid gust]

pushed something a bit better

[outer steeple]

I'll let tests run and then merge it

[rigid gust]

they ran

remember to squash

[outer steeple]

can do better

[rigid gust]

nice

do we change the package name now then?

or do we wait a bit for that?

[outer steeple]

do it

[rigid gust]

net.neoforged.bus?

[outer steeple]

yeah that's fine

[rigid gust]

intellij try not to reformat code when doing an unrelated operation (impossible challenge)

[charred sluice]

Use the formula the JDK uses for HashMap.newHashMap(int)
https://inside.java/2022/10/24/sip069

[rigid gust]

there we go: https://github.com/neoforged/Bus/pull/15

[rigid gust]

lol they finally added that ^^ well the PR was merged already

[charred sluice]

https://github.com/openjdk/jdk/commit/87faa85c59e94d66c3c61d997eacdd2dbe5a1772#diff-9837dc8a9161163b6a1489098920d47c7fb62f19d9f929c1447f3c1c9bf7aa68R2564

[outer steeple]

best formula ever

tech's is better

[charred sluice]

A surprising amount of thought went into the formula
https://bugs.openjdk.org/browse/JDK-8186958#:~:text=The most common,the correct one

[rigid gust]

annoying that they didn't just fix the inconsistency, oh well

The int-only computation (3) can wrap around to negative for certain values. 😦

[charred sluice]

wdym? in the commit I linked they fixed it across a large part of the whole jdk

[outer steeple]

(3) can wrap around to negative for certain values.
hwat

[charred sluice]

a bit trigger-happy with the merging eh? 😛

[rigid gust]

well it's true if numMappings is too close to Integer.MAX_VALUE

I think the merge is fine as is

we need to move forward and there's a lot of upcoming event stuff 😛

[outer steeple]

2^29 could trigger it but that's an absurd amount of listeners

[charred sluice]

don't underestimate how badly modders can use an api at times

[rigid gust]

it's not even listeners

[outer steeple]

how much time did they spend testing all of those forumulas lol

[rigid gust]

you'd need 2^29 different event classes or object methods

[charred sluice]

remember that one mod that registered new listeners for every event in forge, for every listener it actually cared about registering?

absolutely killed perf

[gusty dove]

lol what

[outer steeple]

2^29 event classes is probably impossible if they're on the same class loader if the class loader uses a list to store the classes it loaded

[rigid gust]

anyway once the package rename is done (please review carefully) I want to remove the subclass transformer

[charred sluice]

#general message

[viscid mango]

diesieben07

[Jump to referenced message.](#general message)

paint_ninja referenced · Jun 28, 2022, 5:51 PM

[outer steeple]

oh vault hunters.. yeah we don't talk about that mod

[charred sluice]

scroll up and down for more context

[outer steeple]

that is for sure one of the most cursed mods (in terms of both code and content) out there

[granite nimbus]

The heck... I... but...

[spice sphinx]

yeah that was the most fucked up shit

[rigid gust]

@outer steeple waiting for you to double check the package change whenever you have the time... 😉

[outer steeple]

ok

[rigid gust]

thanks!

will get back to you tomorrow with more PRs 😄

[mellow hearth]

👀 shouldn't of that been a major version bump?

[rigid gust]

No, this is on the 7.x series already

Otherwise every other commit would be a BC

I hope the maven artifact is correct

[rigid gust]

how stupid would it be to ask people to addListener(object, MethodHandles.lookup()) 😄

I guess that wouldn't work with EBS anyway 😦

[light mantle]

avoiding boilerplate is something we should strive for imo

[rigid gust]

it's just unfortunate that we'd be sidestepping all rules of java access control 😛

[granite nimbus]

Hey, we're also sidestepping type erasure

And people have already expressed that they think sidestepping that one is a good thing, so I don't really see the issue with sidestepping access control too

[rigid gust]

the argument for type erasure was that it already "works"

I guess the same argument holds for access control

[rigid gust]

I wonder if it's even safe to refer to a private field from an ASM event listener nvm, has to be safe, it's just that the method can't be private itself

https://github.com/neoforged/Bus/pull/16

[rigid gust]

so, I did a bit of profiling with different strategies of turning a Method into an IEventListener:

Benchmark                                              Mode  Cnt    Score    Error  Units
EventBusBenchmark.testClassLoaderCombined              avgt   15  108.977 ± 16.727  ns/op
EventBusBenchmark.testClassLoaderDynamic               avgt   15   42.961 ±  8.129  ns/op
EventBusBenchmark.testClassLoaderLambda                avgt   15   49.375 ±  6.678  ns/op
EventBusBenchmark.testClassLoaderStatic                avgt   15   46.355 ±  5.797  ns/op
EventBusBenchmark.testLMFCombined                      avgt   15  110.961 ± 15.475  ns/op
EventBusBenchmark.testLMFDynamic                       avgt   15   48.799 ±  5.834  ns/op
EventBusBenchmark.testLMFLambda                        avgt   15   53.936 ±  7.609  ns/op
EventBusBenchmark.testLMFStatic                        avgt   15   42.744 ±  2.616  ns/op
EventBusBenchmark.testMethodHandlesCombined            avgt   15  121.443 ± 18.432  ns/op
EventBusBenchmark.testMethodHandlesDynamic             avgt   15   64.065 ±  8.803  ns/op
EventBusBenchmark.testMethodHandlesLambda              avgt   15   49.044 ±  6.795  ns/op
EventBusBenchmark.testMethodHandlesStatic              avgt   15   63.061 ± 10.254  ns/op
EventBusBenchmark.testModLauncherCombined              avgt   15  105.640 ± 25.913  ns/op
EventBusBenchmark.testModLauncherDynamic               avgt   15   52.981 ±  6.329  ns/op
EventBusBenchmark.testModLauncherLambda                avgt   15   53.912 ±  6.868  ns/op
EventBusBenchmark.testModLauncherStatic                avgt   15   50.152 ±  6.556  ns/op

[viscid mango]

Created Gist at the request of @outer steeple: https://gist.github.com/NeoCamelot/7d6e7860de98ac6dc115cc35256cb4cd

[rigid gust]

method handles are a bit slow, but they are a decent fallback when --add-opens java.base/java.lang.invoke=net.neoforged.bus is not available

I estimate about 20 ns/op of overhead

[outer steeple]

have you tried wrapping them in a constantcallsite?

[rigid gust]

how so? MethodHandles is with invokeExact, whereas LMF uses LambdaMetaFactory

[outer steeple]

public class ABC {
    public static void main(String[] args) throws Throwable {
        final var lookup = MethodHandles.privateLookupIn(ABC.class, MethodHandles.lookup());
        new ConstantCallSite(lookup.findStatic(ABC.class, "hi", MethodType.methodType(void.class)))
                .dynamicInvoker().invokeExact();
    }

    private static void hi() {
        System.out.println("hi!!!");
    }

}

there's a chance that causes the handle to be constant-folded

[rigid gust]

the handle depends on the listener sadly

this is the listener:

IEventListener listener = event -> {
                try {
                    updatedMH.invokeExact(target, event);
                } catch (Throwable throwable) {
                    throw new RuntimeException(throwable);
                }
            };

[outer steeple]

yeah but the updatedMH can I assume be wrapped by a constantcallsite?

[rigid gust]

yes? but updatedMH can change depending on which listener is being used

[outer steeple]

show the entire code, I'm confused

[outer steeple]

yeah so just replace the methodHandle.asType... calls with new ConstantCallSite(methodHandle.asType(..)).dynamicInvoker()

[rigid gust]

I don't see what there is to fold

[outer steeple]

constant-folded handles have better performance

[rigid gust]

yeah but what is it going to constant-fold?

[outer steeple]

the callsite target

just try it and see if it helps

[rigid gust]

this uses a single IEventListener class for all the listeners

and the class has a methodhandle field which might be different for different instances

there's nothing it can fold

[outer steeple]

did you try it?

[rigid gust]

no and it's not gonna make a difference

[outer steeple]

just push the code somewhere, I'll do it myself..

[rigid gust]

I'm telling you 😛

https://github.com/Technici4n/Bus/tree/lmf-tests

while it runs please look at https://github.com/neoforged/Bus/pull/16 😛

[rigid gust]

@outer steeple feedback addressed

(the force push only removed the transformer and changed Event.java, but I wanted that to be in the first commit)

[inland dawn]

🚌

[rigid gust]

the next PR is ready on my computer, please review 16 😄

[rigid gust]

thanks

[rigid gust]

more PR: https://github.com/neoforged/Bus/pull/17 😄

[rigid gust]

next one will be removing listener inheritance

[pale grove]

Make sure the major is bumped for that one

No matter what, even if it means we have 5 total major bumps

That's a huge breaking change that should be clearly marked

[deft palm]

does it need to be removed? I have a usecase which would break with this.

Basically: I extend an event, to use the Stackwalker to figure out which mod canceled an event

[rigid gust]

no

we already have a major bump for the whole chain of commits

and nobody is using this atm

[rigid gust]

honestly, wtf

[deft palm]

well, we had like 4 people ask why their powered spawner wasn't working and they all had a mega torch somewhere that canceled the spawnevent, telling them that spawn is blocked by megatorch or any other mod will definitely improve their search for why the spawner doesn't spawn anything

[rigid gust]

removing listener inheritance is not a required feature of this refactor

the argument to remove it was that it is a confusing feature

however it also comes with a lot of flexibility

[pale grove]

I figured every merge was being built and published automatically, as is the system we use. If this huge breaking change comes up as a patch commit we've failed as software developers :p

[rigid gust]

the package name (from net.minecraftforge.eventbus to net.neoforged.bus) change came as a patch commit

sorry not sorry 😄

[deft palm]

yes, I know this is niche use case, and listener inheritance has caused issues (people listening to pre and post events etc.), I just wanted to say why I would miss this

[rigid gust]

well it's good to discuss this now

[deft palm]

I could replace that with a mixin into a neo class(not sure if that was blocked or not), so if mixins work, I can do that without inheritance (also the constructor is annotated as internal, but tbh, I was fine with that)

[rigid gust]

<@&1128776937410670663> thoughts on removing inheritance awareness in Bus? (i.e. the fact that listeners registered to BaseClass also receive SubClass extends BaseClass)

[pale grove]

The thought I had was to move the highest inheritable event to an annotation

@EventInheritanceRoot or something

So that every subclass of that annotated class will be handled by a handler for the annotated class

It doesn't happen accidentally, that way

Should remove all the footguns of the system while remaining flexible

[humble junco]

I'd be fine with that ONLY if we make sure all use cases are preserved.
my notable use case is the config event, for which I handle the parent so that I can run the same logic in both load and reload events

[rigid gust]

so Pre and Post events would have to be marked with an annotation every time?

that doesn't seem ideal

[outer steeple]

that's also one of the event people misuse inheritance for most frequently

why would someone run logic in both pre and post

[lone kelp]

create an extra method? And just call that from the load and reload handler

[rigid gust]

giga is talking about config load and reload

[humble junco]

it makes no sense to have a handler for combined pre/post

[pale grove]

The event that has Pre and Post subclasses.

The root of that tree

[humble junco]

IMO it's a bug to do that in all cases I can imagine

[spice sphinx]

scrim not more annotations

[humble junco]

I already do that, but I would rather not have unnecessary event handlers.
if we had a config event for any time the config values are updated, then I wouldn't need two events at all :P

[pale grove]

It's just a marker this time :p

[spice sphinx]

I already wanted to delete @Cancellable and @HasResult

[humble junco]

cancellable and hasresult are stupid annotations

[outer steeple]

also the config event also has an unloading event, listening to the root is stupid

[humble junco]

there wasn''t an unload event when I wrote the code, in fact I didn't know it existed until I looked at the class just now

[outer steeple]

see? that's my point lol

you're asking for trouble

[humble junco]

but

as I saiud a while ago here or somewhere else this conversation was happening

[lone kelp]

fair, but ¯_(ツ)_/¯

[humble junco]

I strongly disagree with having event subclasses for this

unloading should not extend modconfigevent

[rigid gust]

why not? it's a config event 😛

it just happens that it's convenient for you to listen to 2/3 of the events

[agile slate]

I think in 99% of cases listening for the parent event is a bug and the other 1% should use separate listeners

[humble junco]

wait nevermind I retract that

[mellow hearth]

Maybe a registration helper instead? Where you specify the base type and the sub types your interested in and EB registers the listeners which then downcasts to the base and call you with it? Although obviously annotation based registering wouldn’t support that

[humble junco]

I mistead the event description

it happens before the config becomes unavailable

so it's fine for it to be bundled in with the rest and have a reference to the config

it just will be a bit pointless when it fires in my mods :P

[rigid gust]

I mean that's basically looping over a list of classes and registering for each, doesn't need an API

[deft palm]

what have I caused? The InheritanceRoot annotation sounds like a decent option at that point

[rigid gust]

I don't think it would work

[outer steeple]

moar magic annotations

[rigid gust]

I think @Cancelable is fine

could be an interface instead, doesn't make a difference 😛

[outer steeple]

no it's not, it only has one L

[deft palm]

that should just be an interface imo

[agile slate]

technically it has 2

[mellow hearth]

True which is one of the reasons why I said Helper rather than API

[lone kelp]

which would then remove the cancel method from all other events, right?

[agile slate]

correct

[outer steeple]

can't wait for Object2BooleanMap<Event> isCancelled

[lone kelp]

it's weird that they exist in non-cancellable events anyway
unless I'm misremembering and they don't exist in those

[outer steeple]

I mean if we make it an interface, we could just make the cancelled field in the Event class package-private so only the interface can access it

[rigid gust]

it's already a private field

ah I see

[outer steeple]

package-private
. unless you want to make the cancellable interface an inner of Event

[lone kelp]

can you not move the field to the interface then

[rigid gust]

no, interfaces can't have fields

[outer steeple]

ahem

interface
java

😛

[rigid gust]

I think it's a good idea... I don't know if we should be doing this in this refactor though

[outer steeple]

I mean we're already breaking stuff. I think this kind of refactor is a "now or never*" kind of thing

[rigid gust]

I'd also like to remove the result stuff and that should probably wait for another time

[outer steeple]

the result is only used by like 4 or 5 events in neo and it's confusing to use

[spice sphinx]

result should be yote

it needs to be per event

its useless the way it is

[lone kelp]

yeah, I misremembered, you can have constants, but that's not useful

[rigid gust]

is that all???

[deft palm]

sounds about right last time I checked that annotation

[rigid gust]

note that the annotation does literally nothing - you can call setResult on any event

(I just changed this in my latest bus PR, but still...)

ah no there's 19 events that have a result

[outer steeple]

sighs

[rigid gust]

so, this event only has two possible "results" but instead of being cancelable it has a result... lol

I don't think we can realistically clean this up on time

[agile slate]

20 events

[rigid gust]

so I suggest moving the annotations to interfaces, but keeping the Result enum for now

[humble junco]

I bet 1.20.3 is going to happen within a few months at most

no point rushing things

[rigid gust]

yeah

however I want to reach a decision for event inheritance

[outer steeple]

i think it's fine to just yeet it imo

[spice sphinx]

The result enum should be per-event so the enum values can be documented per event imo

[outer steeple]

two listeners is not the end of the world

[spice sphinx]

^

[rigid gust]

I agree but IMO we're a bit late for that

rip agnor's cursed hack then ^^

[outer steeple]

I mean

she can still do it

but it's not going to be clean

the listener list of her custom event just needs to copy over the listeners of the "root event"

it's not clean but it is possible

so

¯_(ツ)_/¯

I mean heck we could even add a "hidden api" for listenerlist inheritance

[rigid gust]

definitely not

removing inheritance makes the code much simpler

[spice sphinx]

sekrit .addSubclassListener method™️

[rigid gust]

what you'd want is a way to post a subclass as a superclass

[deft palm]

it's not cursed

[rigid gust]

yet

[deft palm]

yeah, my code will be a lot less clean, but if that makes the eventbus a lot cleaner I would be okay with that. As long as it's stable and doesn't change mid mc version lifecycle

[rigid gust]

Yeah it should be stable

[deft palm]

okay, that's all I need

[gusty dove]

you should know that this is a footgun if your event handler is not thread-safe

[primal mist]

This is irrelevant to the inheritance issue, since the problem is primarily a single event (Reloading) being fired multiple times concurrently.

[frankly i don't think it's worth keeping around anyway - any legitimate use case can be replaced trivially with a shared logic function called from both handlers given that the classes themselves would still inherit]

as for the config event, why can it fire on any thread? is this just something we have to put up with because of nightconfig internals?

i kind of wonder if this is the reason for that infamous notepad++ glitch - or, rather, some aspect of the config internals other than the event hook being run concurrently due to two closely spaced file modifications

[primal mist]

note that this is assuming that we keep the inheritance hierarchy itself as-is and simply remove the ability to add listeners to most parent classes, rather than doing something more radical like converting all events to records

though, let's not forget that there are parent classes like EntityTeleportEvent where it's not only legitimate for someone to listen to the parent event, it's legitimate for some mod to add new child classes and expect those listeners to pick it up

so I don't think we should necessarily remove the actual machinery for listener inheritance, just have some way [opt-in or opt-out] of preventing people from listening to parent classes that are not designed for it

[outer steeple]

[rigid gust]

We could prevent people from listening to abstract classes

So the children of an abstract class would act as an inheritance "root"

Idk if that makes any sense

I think it's better to remove inheritance entirely

[indigo cloak]

I'd make events records if we didn't need to, like, mutate them, which we do

[rigid gust]

We don't need hashcode or equals so records are probably not too useful

[agile slate]

inheritance is nice in the case of entity/living/player event where they actually use inheritance the right way. what should be removed is lestening to non concrete event classes (super-classes)

[outer steeple]

I'm still yet to find a concrete, non niche, use case for inheritance (besides the network event, but that can be fixed with an enum, no need for 4 different event types that just represent different directions)

[agile slate]

the example from I sent? you have the stuff that many events have in common in the base class to reduce code duplication

[outer steeple]

you're talking about having A extend B, not B listeners receiving A

currently, by inheritance we're taking about the latter

[agile slate]

the latter does not make sense in any case

[primal mist]

i already pointed out entity teleport

[granite nimbus]

I'd argue that cases like that should be resolved by an argument in the event, not by event inheritance of listeners

[spice sphinx]

we could keep it and make it impossible to listen to abstract classes, maybe

[honest timber]

And then with valhalla value classes, zero allocation events

[spice sphinx]

but yeah for the most part EntityTeleportEvent could be just given a TeleportCause enum

[wheat sinew]

i have a feeling valhalla might land in the Java LTS after next

[honest timber]

I’m hoping for incubator or preview in java 22

[latent rivet]

Enum would defeat the point Random made where mods might be adding custom variants/causes, so it would have to be some context object like we have for other things

[granite nimbus]

The benefit of removing it is that you don't have to handle that stuff while firing events or building event listener lists or whatnot, at least from what I understand. Allowing abstract classes doesn't help there

[granite nimbus]

The better option than an enum is a dedicated type that holds that custom variants/causes

[spice sphinx]

not allowing abstract makes it an error condition to try to listen to an invalid parent

it doesn't make the code cleaner though

[granite nimbus]

And have that bit have inheritance

[latent rivet]

That's basically what I meant, yes

[wheat sinew]

TeleportCause

[spice sphinx]

I invented this cursed catch-all arbitrary context system in the inventory tick pr

[wheat sinew]

final TeleportCause cause;

class TeleportCommand extends TeleportCause { ... }
class EnderPearl extends TeleportCause { ... }

[spice sphinx]

https://github.com/neoforged/NeoForge/blob/225b0e4927b4139d33a752124f4496e3167ab5ee/src/main/java/net/minecraftforge/common/context/ContextKey.java

cursed little bit of software that is

[latent rivet]

Yeah, something like that plus a type that identifies it (i.e. either a registered type or a manually interned thing like PlantType, SoundAction and the old ToolType)

[wheat sinew]

TeleportCauseKind

[granite nimbus]

TeleportCause<T extends TeleportCauseToken>

With a token type per cause type and a getToken method

[wheat sinew]

i do wish we're on Java 21, if only to leverage Pattern matching instanceof here

uh

Pattern matching switches with instanceof

[granite nimbus]

instanceof is a bit slow though

But I guess this event is called pretty rarely

I just like my token approaches too much

[wheat sinew]

think

TeleportCause cause = ...;

switch (cause) {
  case TeleportCommand command -> { ... }
  case EnderPearl pearl -> { ... }
  default -> { ... }
}

[granite nimbus]

delegation based off type is bad practice (/hj)

[wheat sinew]

say that to the JDK and its Class-File API

[granite nimbus]

But yeah that would work if we had that feature

[latent rivet]

I'd say the token/type approach is better in this case, if only because it allows you to check for another mod's cause type without any kind of dependency on the other mod since you as the one checking for it would be retrieving the type/token by its name

[granite nimbus]

Yeah, interned, resource key based tokens are great

Or just interned tokens of whatever other sort

[wheat sinew]

Registry<TeleportCause>

[lone kelp]

now I wonder if that's smart enough to not require default on sealed classes

[wheat sinew]

for exhaustive, I believe so

[latent rivet]

Right, that's actually a good call, the type could literally just be a ResourceKey

[granite nimbus]

Yep! The issue is how to attach extra context to it - and that's trivial by making the context extensible. Heck, it can be integrated into the resource key type if you're clever with some recursive generic fuckery

[latent rivet]

I would just do something simple like this (boilerplate code ommitted):

class TeleportCause<T extends TeleportCause> {
  final ResourceKey<T> type;
}

[granite nimbus]

Yep, and then you'd have a <T extends TeleportCause<T>> @Nullable T get(ResourceKey<T> key) method in the event

That returns that type cause if it's the cause of the event or null otherwise. You'd have a getKey method too obviously

[wheat sinew]

this isn't a novel concept in the API, right? i'm getting a bit of deja vu

[granite nimbus]

Registry events

Creative tab events

Yeah, it's already used a few places

[latent rivet]

That would work, yeah. I would probably go with the trivial variant of a "dumb" getter though and have the consumer of the event just do the checks themselves

[granite nimbus]

Those checks involve casts though

Might as well provide a typesafe API if we can, and we definitely can

That's the nice thing about a parameterized key

[latent rivet]

Hmm, yeah, fair enough

[granite nimbus]

(technically the key should be to a type stored in a registry so you'd need a TeleportCauseType<T extends TeleportCause<T>> type to register somewhere but whatever, not really a big deal)

But yeah, obviously that's details to figure out if this is actually implemented but the basic useful info is that there's no need for inheritance in listeners to do it

[latent rivet]

If you had a registered type then you wouldn't even need the key since you could instead pull the type from the registry at some point of startup and then use that

[granite nimbus]

The key is what let's mods easily check it without a hard dependency though. There's precedent all over in vanilla

And in forge too, for that matter

If it's in a registry already, you might as well delegate based on the key

Just because a key can be shoved in a static final field somewhere and call it a day

But yeah, the general concept is a type token in a registry, at a given resource location, and then looking up contexts based on the token (or a key associated with the token, either way)

It's basically just BlockEntityType

Or EntityType, for that matter

[granite nimbus]

Heck, once 1.20.2 has a neoforge release so I have something to go based on I can work on a PR to do that

[primal mist]

does valhalla support byref mutable parameters?

so, regarding config events

i'm not convinced load and reload should actually be distinct events at all

[spice sphinx]

probably not, no

[primal mist]

like, handling both with the same logic seems like it's super common, and said logic usually consists of copying the config values into static fields [along with transforming from e.g. a serialized string into the runtime type you actually want]

[spice sphinx]

generally the only point of the event is to update cached versions of the config values

and so you have to listen to both

[primal mist]

what would you do in one that you wouldn't do in the other, ignoring the thread safety issue

[like, presumably Load doesn't have the thread safety problems because it's triggered by startup instead of some cursed file change listener, but A) we should be fixing that anyway and B) if people do bother to write in a thread safe way there's not that much overhead to also doing that on load]

maybe have an isInitialLoad boolean on the event, in case someone does care, but 99% of the time you're going to be doing the same thing in both cases

[rigid gust]

I think preventing listeners from being registered to abstract events would make sense?

idk, it's hard to say how many mods are using event inheritance and how much they are relying on it

[inland imp]

We could make it posisble to do @vernal latch with a concrete type multiple times

[granite nimbus]

Is there a benefit to it if we can't wrip out the whole inheritance subscription system? Mods might very well be relying on it the same as non abstract inheritance - think the whole teleport example

[inland imp]

or even bus.register with a concrete type

[rigid gust]

that's already something one can do

you mean this with teleport events?

[granite nimbus]

I'd say personally it's worth making inheritance not do anything for event subscription, which would involve making it impossible to listen to abstract events

[granite nimbus]

See Random's explanation earlier. While trivial to replace them with a single event with a context, you could imagine that other mods have a similar system

Though I guess the real question here is how much pain is allowing listener inheritance causing technically?

[rigid gust]

not too much

[granite nimbus]

Then maybe it's not a big deal

[rigid gust]

the code already exists after all

I'd like to do something about people subscribing to event inheritance chains that don't make sense

[granite nimbus]

The issue is that "not abstract" isn't a good heuristic for "makes sense"

Like, if that root EntityTeleportEvent was abstract, it would still make sense to subscribe to it

[rigid gust]

could add a new annotation

[granite nimbus]

Honestly not a terrible idea - UnlistenableEvent or something

Or the other way around I suppose for listenable abstract events

[rigid gust]

listenable should be the default IMO

we can require that UnlistenableEvents be abstract

[spice sphinx]

bleh not more annotations

[rigid gust]

it's a good use of them though 😛

I'll remove the other two

[granite nimbus]

It's not a great solution, I agree

Anything done via annotation should have a non annotation equivalent

[rigid gust]

wtf no

[deft palm]

it's the same amount, cancelable is removed after all

[rigid gust]

what kind of nonsense is this? 😛

[spice sphinx]

I think its reasonable to just make abstract unlistenable

[granite nimbus]

In terms of stuff with runtime effects - yes. See: EBS and SubscribeEvent

[rigid gust]

that's not a given - see @Mod

[granite nimbus]

Mod actually isn't required with a different language provider

[spice sphinx]

Also to make an equivalent "Unlistenable" feature without an annotation you would need a global Set<Class<?>>

[granite nimbus]

And a way to add stuff to it

[spice sphinx]

and then you run into this issue of "anyone can insert any random garbage into that set"

with untraceable errors

[granite nimbus]

I mean, we can verify that it's an event subclass

But I see what you mean

[rigid gust]

registrations to a global set won't happen

[spice sphinx]

yes, but I could just go mark some random event as unlistenable and nobody could tell it was me, lol

[granite nimbus]

Wait, okay, how about this one instead: you can only listen to final event classes

...wait fuck same issue

[spice sphinx]

same thing as abstract :p

[rigid gust]

that defeats the whole point

[granite nimbus]

Hmm. Annotations are not a solution though, that's the issue here

[rigid gust]

that's the same as removing inheritance

[spice sphinx]

abstract is simple, if you want the event listenable to you just don't make it abstract

[rigid gust]

EntityTeleportEvent could be made abstract though

[granite nimbus]

You know, fair enough

[rigid gust]

and it should still be listenable to

[spice sphinx]

sure, so just don't, lol

you do not need to make anything abstract

[granite nimbus]

The issue is that the root event shouldn't be instantiable/fireable

[spice sphinx]

the teleport event is perfectly fireable

[granite nimbus]

You do if it has methods no good default implementation can be provided for

[spice sphinx]

you can have a non-contextual teleport

[spice sphinx]

throw enters the chat :p

[rigid gust]

imo "not listenable" should imply "abstract", but not the other way around

I think an annotation is the best solution

[spice sphinx]

stab

[granite nimbus]

An annotation isn't a terrible option but there still needs to be another way to mark stuff - I'm tired of the annotation-centric APIs forge likes to make, and they're not pleasant to work with in the long run

[rigid gust]

you should stop making blanket statements though

annotations are the best tool for the job here

[spice sphinx]

there is no other option other than an implicit class property (abstract) or an explicit class marker (annotation)

a global set is spaghetti

[granite nimbus]

I really don't think annotations should be our only solution here. If they're the only one, then there's some underlying design flaw leading to that

[agile slate]

annotations are the only sensible solution that covers all bases

[granite nimbus]

Then there's some underlying design flaw

[rigid gust]

the fact that you dislike annotations is not a sufficient reason to not use them 😛

[agile slate]

this is the exact usecase annotations are made for

annotating something

[granite nimbus]

Yes. For like verification and whatnot. Not for behavior!

[rigid gust]

^

[granite nimbus]

That's not really an argument. They lead to badly differing behavior between two classes that can be determined only by some other class's contents that might not be referenced from either of the classes in question

It's bad for code understandability

[agile slate]

but it is not behaviour that changes in this case it is verification that you don't subscribe to parent events

[granite nimbus]

But I don't have the time or energy to fight this. I think we should be trying to toss out magic annotations, not add more, but if that's an unpopular opinion do what you'd like I guess

[agile slate]

make a better suggestion or shut up

[rigid gust]

a bit harsh but yes this is not productive

[granite nimbus]

An underlying paradigm shift so that you're delegating off an event type, not a class, because delegation based on classes sucks?

You know, the thing I recommended literally months ago?

[rigid gust]

that's too much of a breaking change

[granite nimbus]

Then now's the time to do it

[rigid gust]

the benefits are not worth it

[granite nimbus]

That's your very subjective opinion. Now, seeing as you're currently the only one writing these PRs that's fair

But it's not an argument against any such work in the future

[rigid gust]

I don't think the amount of breakage that a change causes is subjective

this is not clean slate development, we have to take existing code into account

[agile slate]

hmm with the capabilities rework we could also nuke IGenericEvent and GenericEvent (since the only event using it atm is AttachCapabilitiesEvent)

[rigid gust]

gonna be deprecated for removal

[granite nimbus]

And yet we can still make major breaking changes where necessary. See: capabilities, registries. There's no reason we have to stick to the old approach if, frankly, the old approach has some flaws

[rigid gust]

the flaws are quite minor I would say

[granite nimbus]

And that is where the subjective opinion comes in. I saw someone saying the same thing about the old capability system yesterday

[rigid gust]

we also need to keep a bus-like design for mod busses and their implications (somewhat nicer registration, parallel firing)

my opinion is probably a lot more informed

[primal mist]

if we were going to do a green field redesign of the event system i'd want per-flyweight listener lists before i'd want to change the class-based system

[rigid gust]

what does that even mean

[primal mist]

e.g. register a listener for LivingUpdateEvent that only fires on a specific EntityType

i know it's never going to happen, you don't need to waste time arguing with me

[rigid gust]

then why do you mention it if you know it's never gonna happen?

[primal mist]

it's something that could be on the table if we were going to redesign the event system from scratch. but we're not going to

[rigid gust]

I doubt there would be a clean design for something like that in the first place

[granite nimbus]

I mean. A switch to an event type system like I'd suggested suggests several fairly clean approaches to that

[rigid gust]

I really don't think so

you'll have the same problem as a generic capability system again

[granite nimbus]

No, you wouldn't. You'd have no less of an option than you have now with stuff like EntityTeleportEvent

Cause it wouldn't be depending on the type, which is a very iffy way to do it

You wouldn't be able to use EBS for those particular events, true, but that one there's no way around

[rigid gust]

I mean that you'd have to e.g. get the Item from an ItemStack for your dispatch