ejektaflex / Kambrik

A Lightweight Kotlin-Fabric Library
https://kambrik.ejekta.io/
10 stars 4 forks source link

preLaunch entrypoint causes game classes to be initialized #7

Closed Johni0702 closed 2 years ago

Johni0702 commented 2 years ago

Kambrik's preLaunch entrypoint accesses Kambrik.Logger, causing the whole Kambrik object to be initialized, which initializes KambrikSerialApi which contains references to various game classes such as Block, Item, etc.

This can cause unexpected and difficult to track down compatibility issues and should be changed.

From the docs on PreLaunchEntrypoint:

Avoid interfering with the game from this! Accessing anything needs careful consideration to avoid interfering with its own initialization or otherwise harming its state. It is recommended to implement this interface on its own class to avoid running static initializers too early, e.g. because they were referenced in field or method signatures in the same class.

ejektaflex commented 2 years ago

Can you show me what is causing an issue for you? I have a reason to be using a preLaunch entry point - and I've read the docs before. These vanilla classes in the Serial API may be classloaded, but they are not instantiated. The only vanilla class that I explicitly instantiate in preLaunch is Identifier, which does not require a game context (and it something I could look into avoiding, if need be).

Johni0702 commented 2 years ago

The specific issue I'm running into is to do with how Mixin chain-loads dynamically registered mixin configs. To sum up a lot of painful details (not exactly the same but similar to https://github.com/gnembon/fabric-carpet/issues/341#issue-642630160), it will only load them while no mixins have been applied yet. By referencing the Item class, it gets loaded and mixins which target it (e.g. fabric-api has some) get applied, thereby preventing other preLaunch entrypoints from dynamically adding mixins (in general because of how mixin behaves, but also specifically for Item because that class has already loaded then).

Fixing this should be as simple as moving the Logger field into a different class, so it doesn't pull the whole Kambrik object with it when initialized. It doesn't look like your preLaunch entrypoint actually needs any of that to be initialized.

ejektaflex commented 2 years ago

That is pretty legitimate reasoning! I do like putting most API calls into one object for easy access - so first I may try to lazy load data in the Serial API. If that doesn't work, I can investigate moving the logging feature elsewhere.

ejektaflex commented 2 years ago

This has been changed for the next version. Next version release is TBD, but thanks!