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

Implement support for NeoForge #13

Merged
merged 5 commits into from
Aug 5, 2023
Merged

Implement support for NeoForge #13

merged 5 commits into from
Aug 5, 2023

Conversation

Tkain
Copy link
Contributor

@Tkain Tkain commented Jul 31, 2023

This PR allows for ForgeWrapper to launch NeoForge just as it launches Minecraft Forge. This is mainly accomplished using Forge's fml.forgeGroup launch argument, which specifies the base path of Forge-related packages; for Minecraft Forge, this has always been net.minecraftforge, whereas NeoForge could use net.neoforged. With this PR, ForgeWrapper now stores the value of this flag and uses it when determining the path to the Forge installer.

Additionally, this PR fixes a bug preventing ForgeWrapper from compiling on Java versions newer than Java 8, which I found useful developing using Java 17.

Resolves #12.

Tkain added 2 commits July 30, 2023 22:07
This uses Forge's built-in fml.forgeGroup flag to distinguish Forge and NeoForge's package names. Consequently, this flag's value needs to be passed to most installer-related functions.
@Tkain Tkain changed the title Implement support for NeoForged Implement support for NeoForge Jul 31, 2023
@Tkain
Copy link
Contributor Author

Tkain commented Aug 1, 2023

Now that I've come across concrete proof of it, I should also acknowledge that NeoForge's version of FML doesn't actually use the fml.forgeGroup flag for anything anymore; only the original Minecraft Forge's FML uses it. I'm open to changing things if this is unsuitable for distinguishing Forge and NeoForge, but it'll probably take extra work.

Tkain added 3 commits July 31, 2023 22:42
I was hoping not to hardcode any package names here, but this is the only way to unbreak manual installs without unintuitively asking users to add the flag themselves.
@Tkain
Copy link
Contributor Author

Tkain commented Aug 2, 2023

I'd say the PR is feature-complete now, so I'm un-marking it as a draft. That said, there is still one lingering issue I haven't been able to figure out during development, but I'm not sure if it's related to the PR or not; when launching an installed NeoForge instance, NeoForge crashes because it can't find its built-in language providers. From what I've investigated regarding the issue, for whatever reason the language provider classes don't end up in FancyModLoader's class loader despite being on the classpath. Classic Forge works as expected, however.

I'll update the PR with a fix if I can find one out, but I've had no luck over the last two days.

Edit: This (hopefully) might be just a NeoForge bug; see neoforged/NeoForge#68.

@Tkain Tkain marked this pull request as ready for review August 2, 2023 23:27
@ZekerZhayard
Copy link
Owner

In response to your crashes, I'll look into it this weekend and discuss it when I have a definitive solution.

@Tkain
Copy link
Contributor Author

Tkain commented Aug 4, 2023

No need! As of NeoForge version 47.1.60 (where the bug I mentioned was fixed), NeoForge works as expected when installed using this PR!

@ZekerZhayard ZekerZhayard merged commit 7a8427d into ZekerZhayard:master Aug 5, 2023
@Pyker
Copy link

Pyker commented Aug 5, 2023

Can a release be created to push out these changes?

@ZekerZhayard
Copy link
Owner

@Pyker released

@Pyker
Copy link

Pyker commented Aug 5, 2023

Thanks!

@Pyker
Copy link

Pyker commented Aug 5, 2023

I confirm, it works just fine!

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

Successfully merging this pull request may close these issues.

NeoForged support
3 participants