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

FMLConstructModEvent does not provide optional mod constructor arguments #122

Open
Fuzss opened this issue Apr 30, 2024 · 20 comments
Open
Labels
bug Something isn't working

Comments

@Fuzss
Copy link

Fuzss commented Apr 30, 2024

Neo allows for some optional arguments (IEventBus, ModContainer, FMLModContainer, Dist) to be passed to the constructor of a mod main class.

Initializing a mod via FMLConstructModEvent has long been an alternative to the constructor setup.
It would be great to be able to access those optional arguments from the event as well, especially ModContainer, which the event already has a package-private getter for (ModLifecycleEvent to be precise).

@shartte
Copy link
Contributor

shartte commented Apr 30, 2024

We're considering removal of this event (or at least, we did). Can you explain your use-case for this?

@Fuzss
Copy link
Author

Fuzss commented Apr 30, 2024

Just a personal preference for the event style syntax in favor of the constructor. Wouldn't mind the event being removed for streamlining the mod setup process.
Actually there is a use case, see below.

@Technici4n
Copy link
Member

Technici4n commented Apr 30, 2024

We should have a public getter for the ModContainer IMO. You can grab it from ModLoadingContext right now but it's not the cleanest.

@shartte
Copy link
Contributor

shartte commented Apr 30, 2024

What do you actually get from using the event? I might be missing something, but you still have to have the mod class?

@Fuzss
Copy link
Author

Fuzss commented Apr 30, 2024

Mod setup during construction in multiple places that are not bound to the @Mod class.
For me it's useful for client-only setup in a separate class via marking the class with @EventBusSubscriber(value = Dist.CLIENT).
Examples:
https://github.com/Fuzss/helditemtooltips/blob/main/1.20.4/NeoForge/src/main/java/fuzs/helditemtooltips/neoforge/HeldItemTooltipsNeoForge.java
https://github.com/Fuzss/helditemtooltips/blob/main/1.20.4/NeoForge/src/main/java/fuzs/helditemtooltips/neoforge/client/HeldItemTooltipsNeoForgeClient.java

@shartte
Copy link
Contributor

shartte commented Apr 30, 2024

I hate the current way of client-only setup too, but I'd rather allow split mod-classes for that purpose :P
The event still only is fired once per mod-class, making it bound to it 1:1 at least.
And yea, this is not all that great either:

https://github.com/AppliedEnergistics/Applied-Energistics-2/blob/main/src/main/java/appeng/core/AppEngBootstrap.java#L40

@Fuzss
Copy link
Author

Fuzss commented Apr 30, 2024

For the setup I linked above I just really like that the client setup can be fully separate from common, just like Fabric, where there is ClientModInitializer. I far prefer this over the manual FMLEnvironment.dist check in common that you've linked, which I've also seen in most mods.

And if the goal is to at some point only provide a single method for mod construction I'd honestly rather axe the constructor instead of FMLConstructModEvent. The dedicated construct event is way more inline with how Neo normally works (via events that must be subscribed to and can be used in a decentralised manner, even all the rest of mod loading / setup is done via events and not some poorly documented reflection). The constructor being invoked via reflection really is quite ambiguous regarding what is allowed and what is not, mainly like what arguments are accepted and the order they must be placed in. I haven't found any documentation in Neo itself on that, had to look up the actual implementation in FML to find that e.g. the order does not matter. Just having an event with proper getters and actual documentation makes for a so much cleaner api.

@Shadows-of-Fire
Copy link

@Fuzss With the addition of multiple (and dist-specific) mod entrypoints in #126, does this issue report still have merit? The prior usecase appears to be covered by the new features.

@Shadows-of-Fire Shadows-of-Fire added the bug Something isn't working label May 18, 2024
@Fuzss
Copy link
Author

Fuzss commented May 18, 2024

Well, the original report is still valid. There remains a disparity between the @Mod class constructor and FMLConstructModEvent. At least turning the ModContainer getter public would be great.

Besides that the documentation issue regarding optional arguments remains a concern especially for new modders and should be addressed. And it still seems to me that FMLConstructModEvent is way more inline with NeoForge's usual design in favor of the @Mod class constructor.

@shartte
Copy link
Contributor

shartte commented May 18, 2024

Besides that the documentation issue regarding optional arguments remains a concern especially for new modders and should be addressed.

Yes, but new modders will even less likely use FMLConstructModEvent...

@Fuzss
Copy link
Author

Fuzss commented May 18, 2024

Since all other mod loading stages are triggered via events such as FMLCommonSetupEvent, FMLClientSetupEvent, FMLLoadCompleteEvent it just seems much more intuitive and logical for the very first stage to be triggered via an event as well. It's rarely used only because few devs seem to know about the event it seems to me.

But there isn't really an issue here, now that dist can be specified in @Mod. Whichever you guys decide to keep that's just how it is.

@Fuzss
Copy link
Author

Fuzss commented May 18, 2024

Another thing that just came to mind (maybe worth a separate issue): On Fabric client mod initialization is always guaranteed to happen after common mod initialization. This is very useful so that the client part can rely on common stuff to already be available.
On NeoForge with FMLConstructModEvent the order is unpredictable. Not sure if this is any different with dist in @Mod in 1.20.6. It would be great to have a way to run client construction after common. Maybe a new event FMLConstructClientModEvent (and FMLConstructDedicatedServerModEvent respectively)? Or have @Mod with a specified dist be constructed after all classes without by default?

@Matyrobbrt
Copy link
Member

Those kinds of heuristics are very confusing, I would rather have a priority/ordering field in the mod annotation (which would guarantee serial ordering between entrypoints of the same mod). For ordering after the entrypoint of another mod, you need to use the ordering field in the dependency declaration.

@Technici4n
Copy link
Member

The side-specific mod constructor should logically come after the common one. No need for more confusing options.

@ChampionAsh5357
Copy link
Contributor

Besides that the documentation issue regarding optional arguments remains a concern especially for new modders and should be addressed. And it still seems to me that FMLConstructModEvent is way more inline with NeoForge's usual design in favor of the @Mod class constructor.

A bit off topic, but please write an issue on the docs repo when there are documentation issues. I can't address things I can't see or remember, and all it does is leave it incorrect.

The docs does mention the optional arguments, but if you believe it is insufficient, let me know and I can update them properly: https://docs.neoforged.net/docs/gettingstarted/modfiles#javafml-and-mod

@shartte
Copy link
Contributor

shartte commented May 18, 2024

The side-specific mod constructor should logically come after the common one. No need for more confusing options.

Yeah I'd agree with that. Someone had a very odd use-case for wanting to run client before common, but that seems very odd.

@Technici4n
Copy link
Member

This should be resolved once #145 makes it into a NeoForge release.

@Fuzss
Copy link
Author

Fuzss commented May 26, 2024

Great, thanks!

And coming back to the original issue regarding FMLConstructModEvent parameters, what's the conclusion for that?
Can the ModContainer getter be made public?
What's the future of the event, will it be removed now that all shortcomings (multiple mod entry points and specifying sides) of the constructor mod construction method have been addressed?
Can I still use FMLConstructModEvent on 1.20.6 or should I refrain from doing so?

@Fuzss
Copy link
Author

Fuzss commented Jul 8, 2024

I came across another feature of FMLConstructModEvent the constructor method does not provide. There is no work queue for sequential code execution. This would be useful when accessing non-synchronized collections this early on.

Maybe there could be an option in @Mod to allow the class to be constructed sequentially as part of a work queue.

@Technici4n
Copy link
Member

Which non-synchronized collections do you want to access that early? Most are probably not initialized, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants