Mekanism

Mekanism

111M Downloads

You're registering stuff at the wrong time

jaredlll08 opened this issue ยท 1 comments

commented

Issue description:

You're registering the following at the wrong time:
Rebuilding a cache too late:
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L657-L663
that cache should be built in postinit, it is the last stage that recipes can be changed for them to still appear in JEI, and is used in many mods as a "well now that everything is done, lets do stuff to the registered things", maybe just build the cache during that stage instead of rebuilding it?

This is just a thought:
Shouldn't this code be in FMLServerStoppedEvent?
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L669-L691
Doing it this way, if a mod is using FMLServerStopping properly, and taking long to do so, it may cause issues if those lists are still in use or something? Just seems a tiny bit risky to clear them when the server is still technically running.

not really an issue, but just naming pls
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L694

that method name is completely wrong.

Events should be done in preinit
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L759-L760
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L772
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L840

Entities should be registered via the event:
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L800

According to forge docs, this should be done in Preinit, but that seems stupid, I personally would move this to the Register event or init
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L837

Lex has mentioned that TileEntities should be registered with blocks in the Register event
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L617-L642

Not too big, but the mod isn't fully loaded at this state
https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/Mekanism.java#L829

This stuff should happen later, right now this gets called as soon as the Mekanism class is loaded, so FMLConstructingEvent essentially, I would change it to at least preinit, maybe init.

https://github.com/aidancbrady/Mekanism/blob/1.12/src/main/java/mekanism/common/MekanismFluids.java#L54-L55

Version (make sure you are on the latest version before reporting):

Forge: NA
Mekanism: 1.12 branch
Other relevant version:

commented

Rebuilding a cache too late:

Not really a cache, but I'm not sure why its done then.

Events should be done in preinit

A) not according to https://mcforge.readthedocs.io/en/latest/conventions/loadstages/#initialization
B) they are player events, it literally doesnt matter
C) they're so simple i might even just move them to the static annotation at some point

Shouldn't this code be in FMLServerStoppedEvent

Those lists shouldn't be changed at that point, unless the server doesn't actually stop

According to forge docs, this should be done in Preinit, but that seems stupid, I personally would move this to the Register event or init

Nope, that's to do stuff after other mods have done all their stuff. It's not adding to the oredict.

Lex has mentioned that TileEntities should be registered with blocks in the Register event

As long as its done before world load its a non-issue

This stuff should happen later, right now this gets called as soon as the Mekanism class is loaded, so FMLConstructingEvent essentially, I would change it to at least preinit, maybe init.

it's a static list, why does that matter?