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

Let's talk mod loading #26

Closed
asiekierka opened this issue Dec 8, 2018 · 13 comments
Closed

Let's talk mod loading #26

asiekierka opened this issue Dec 8, 2018 · 13 comments

Comments

@asiekierka
Copy link
Contributor

The current way of mod loading uses a list of initializer classes in the .JSON which can provide various interfaces. Specially annotated interfaces are, then, indexed in an accessible manner for queries - Fabric queries them for instances of "ModInitializer". This has a few issues, however.

Client/Server environment specific code

(As of 18w49a, there is only really client environment specific code worth worrying about, but for the sake of argument...)

Many classes exist solely on the client and accessing them on the dedicated server is... a bad idea, to say the least. My proposal here would be to add a "ClientModInitializer/ServerModInitializer" interface - with the caveat that those classes only get loaded on the specific environment. Another option would be marking classes as client/server-side-specific in the JSON file. Of course, we could also replace ModInitializer with something else entirely.

Using the JSON interface list for mod add-on resolution

Is this a good idea? Say, if JEI were to add plugins, it would just add a JeiPlugin interface which can be implemented by classes added to the JSON initializer list. For this, we could extend the former to also not load classes if the interfaces on them are not present in the classpath after all mods are added to said classpath.

META-INF/services vs JSON

Someone once said we should use the former versus a custom list format for classes. Just bringing it up, although honestly I'm not a huge fan.

Advantages:

  • IDE tooling support

Disadvantages:

  • Features such as Kotlin singletons are not supported, unlike our custom JSON format where implementing them is possible.
  • Every ServiceLoader for a given interface re-instantiates the class in question, which is a bit less comfortable for certain mod class patterns, but probably not a huge deal.
@modmuss50
Copy link
Member

I like the idea of the ClientModInitializer as well as the other 2. This keeps things clearly separated in code. As long as it doesnt try to load any of the client classes called from within then it should be fine.

Using the current system and not loading classes when things dont exist I think is a good idea. How ever what happens if something critical was removed (either in fabric or a mod that is depended on) the mod would just slienly fail.

Service loader, its great for what it does, but not so good for stuff it doesnt do. Having multiple instances of the same class imo is something we dont want. One idea that was mentioned was implementing a custom service loader that uses the same files/locations (so IDE support) but provides what we want.

@asiekierka
Copy link
Contributor Author

@modmuss50 "How ever what happens if something critical was removed (either in fabric or a mod that is depended on) the mod would just slienly fail."

We could use an annotation, or we could mark it in the JSON. Open to ideas here.

@asiekierka
Copy link
Contributor Author

Another question: Should we use "mod.json" or "META-INF/mod.json" or something else altogether?

@modmuss50
Copy link
Member

Not META-INF imo, doesnt really add anything. I dont see anything wrong with the current mod.json file name.

@kashike
Copy link
Contributor

kashike commented Dec 9, 2018

I'd prefer fabric.json or fabric-mod.json to avoid any conflicts with other platforms.

@Prospector
Copy link
Contributor

Prospector commented Dec 9, 2018

fml.json (Fabric Mod Loader, of course)

Edit: also petition to rename ModInitializers to Fabricators... come on

@ChloeDawn
Copy link
Contributor

fabric_mod.json

_ > -

@NikkyAI
Copy link
Contributor

NikkyAI commented Dec 9, 2018

i would also not use META-INF unless there is some killer feature that requires it.. but it seems to be more restrictive than just the mod.json

i wonder if it would be possible to have split dependencies for Client/Server
so that modders cannot compile with references to the wrong one
i am imagining a structure using multi module builds common, client, server
gradle multi project builds

@liach
Copy link
Contributor

liach commented Dec 9, 2018

we can even just define source sets including client main and test, then using clientApi gradle configuration to add the client code to client source set dependency

@LemmaEOF
Copy link
Contributor

LemmaEOF commented Dec 9, 2018

I do think changing it to fabricmod.json or similar will help clear things up a lot, since if I recall forge is moving to a json-based system too.

@oliviathevampire
Copy link

@asiekierka
Copy link
Contributor Author

I'd say, for 0.2.0:

  • add ClientModInitializer/ServerModInitializer interfaces
  • Rename mod.json to fabric.mod.json (support the former until 0.3.0)
  • Remove semver getVersion(), keep String getVersionString() (figure this out for 0.3.0)
  • No ServiceLoaders

Who's with me?

@asiekierka
Copy link
Contributor Author

Done as of 0.2.0. New breakage should go into new issues.

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

No branches or pull requests

9 participants