-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support for FG2-era Forge #70
base: dev/0.10.0
Are you sure you want to change the base?
Changes from 4 commits
ea44973
6ff9211
64a1aba
f0cafe8
0ff81f4
c009507
6a2a900
98173e3
beb659e
9eaae61
75f0fa8
8e30bfd
7f33c12
4fa31a4
5f33bfd
c9c2765
f7b4916
bd1eb92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,6 +139,14 @@ default boolean isForgeAndNotOfficial() { | |
return isForge() && !getMcpConfigProvider().isOfficial(); | ||
} | ||
|
||
default boolean isLegacyForge() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not really sure where to put this comment so I'll put it here) I'd prefer negating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I flipped a bunch of conditionals accordingly. Lmk if there are any more you'd prefer flipped. |
||
return isForge() && getForgeUserdevProvider().isLegacyForge(); | ||
} | ||
|
||
default boolean isModLauncher() { | ||
Juuxel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return isForge() && !isLegacyForge(); | ||
} | ||
|
||
boolean supportsInclude(); | ||
|
||
default SrgProvider getSrgProvider() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,7 +101,7 @@ public void configureMixin() { | |
ConfigurationContainer configs = project.getConfigurations(); | ||
LoomGradleExtension extension = LoomGradleExtension.get(project); | ||
|
||
if (!extension.ideSync()) { | ||
if (!extension.ideSync() || extension.isLegacyForge()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The IDE sync check here is done for a reason and the mixin AP shouldn't be used in IDE builds. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tbh I don't recall why I even added it. I must have had some issues with Mixin before but I can no longer reproduce that, so I'll remove it again. Out of curiosity, do you happen to know the reason? Removing mixin from the AP classpath just for the IDE seems wrong (you have to manually build to get its errors/warnings) and I've not noticed anything wrong while it was there. The oldest commit I could track it back to (5c2b669) doesn't give any reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Afaik the 0.8.5 AP detects IDEA and disables itself accordingly, Mixin has done that for eclipse for a while too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Mixin AP would be used to have errors if it's not disabled, and those errors are wrong |
||
for (Configuration processorConfig : apConfigurations) { | ||
project.getLogger().info("Adding mixin to classpath of AP config: " + processorConfig.getName()); | ||
// Pass named MC classpath to mixin AP classpath | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -31,7 +31,7 @@ | |||||
import java.nio.file.Files; | ||||||
import java.util.ArrayList; | ||||||
import java.util.Arrays; | ||||||
import java.util.HashMap; | ||||||
import java.util.LinkedHashMap; | ||||||
import java.util.List; | ||||||
import java.util.Map; | ||||||
import java.util.Set; | ||||||
|
@@ -66,15 +66,15 @@ public void provide(DependencyInfo dependency, Consumer<Runnable> postPopulation | |||||
.property("client", "java.library.path", getExtension().getMinecraftProvider().nativesDir().getAbsolutePath()) | ||||||
.property("client", "org.lwjgl.librarypath", getExtension().getMinecraftProvider().nativesDir().getAbsolutePath()); | ||||||
|
||||||
if (!getExtension().isForge()) { | ||||||
if (!getExtension().isModLauncher()) { | ||||||
launchConfig | ||||||
.argument("client", "--assetIndex") | ||||||
.argument("client", getExtension().getMinecraftProvider().getVersionInfo().assetIndex().fabricId(getExtension().getMinecraftProvider().minecraftVersion())) | ||||||
.argument("client", "--assetsDir") | ||||||
.argument("client", new File(getDirectories().getUserCache(), "assets").getAbsolutePath()); | ||||||
} | ||||||
|
||||||
if (getExtension().isForge()) { | ||||||
if (getExtension().isModLauncher()) { | ||||||
Juuxel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
launchConfig | ||||||
// Should match YarnNamingService.PATH_TO_MAPPINGS in forge-runtime | ||||||
.property("fabric.yarnWithSrg.path", getExtension().getMappingsProvider().tinyMappingsWithSrg.toAbsolutePath().toString()) | ||||||
|
@@ -103,11 +103,29 @@ public void provide(DependencyInfo dependency, Consumer<Runnable> postPopulation | |||||
} | ||||||
} | ||||||
|
||||||
if (getExtension().isLegacyForge()) { | ||||||
launchConfig | ||||||
.argument("client", "--tweakClass") | ||||||
.argument("client", Constants.LegacyForge.FML_TWEAKER) | ||||||
.argument("server", "--tweakClass") | ||||||
.argument("server", Constants.LegacyForge.FML_SERVER_TWEAKER) | ||||||
|
||||||
.argument("--accessToken") | ||||||
.argument("undefined") | ||||||
|
||||||
.property("net.minecraftforge.gradle.GradleStart.srg.srg-mcp", getExtension().getMappingsProvider().srgToNamedSrg.toAbsolutePath().toString()) | ||||||
.property("mixin.env.remapRefMap", "true"); | ||||||
|
||||||
for (String config : PropertyUtil.getAndFinalize(getExtension().getForge().getMixinConfigs())) { | ||||||
launchConfig.argument("--mixin").argument(config); | ||||||
} | ||||||
} | ||||||
|
||||||
addDependency(Constants.Dependencies.DEV_LAUNCH_INJECTOR + Constants.Dependencies.Versions.DEV_LAUNCH_INJECTOR, Constants.Configurations.LOOM_DEVELOPMENT_DEPENDENCIES); | ||||||
addDependency(Constants.Dependencies.TERMINAL_CONSOLE_APPENDER + Constants.Dependencies.Versions.TERMINAL_CONSOLE_APPENDER, Constants.Configurations.LOOM_DEVELOPMENT_DEPENDENCIES); | ||||||
addDependency(Constants.Dependencies.JETBRAINS_ANNOTATIONS + Constants.Dependencies.Versions.JETBRAINS_ANNOTATIONS, JavaPlugin.COMPILE_ONLY_CONFIGURATION_NAME); | ||||||
|
||||||
if (getExtension().isForge()) { | ||||||
if (getExtension().isForge() && !getExtension().isLegacyForge()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
addDependency(Constants.Dependencies.FORGE_RUNTIME + Constants.Dependencies.Versions.FORGE_RUNTIME, Constants.Configurations.FORGE_EXTRA); | ||||||
addDependency(Constants.Dependencies.JAVAX_ANNOTATIONS + Constants.Dependencies.Versions.JAVAX_ANNOTATIONS, JavaPlugin.COMPILE_ONLY_CONFIGURATION_NAME); | ||||||
} | ||||||
|
@@ -197,7 +215,7 @@ public String getTargetConfig() { | |||||
} | ||||||
|
||||||
public static class LaunchConfig { | ||||||
private final Map<String, List<String>> values = new HashMap<>(); | ||||||
private final Map<String, List<String>> values = new LinkedHashMap<>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with a normal hashmap? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's a bit subtle but I couldn't think of any better solution. It all comes down to the fact that order of "--tweakClass" arguments matters. We add the default server/client forge tweakers to the "server"/"client" arguments right from the start. So if the user adds their own "server" or "client" tweaker, all is fine. But if they add their own tweaker to the "common" arguments, then the actual order of the final arguments depends on HashMap iteration order because that's the order in which the three section ("common","client" and "server") are written to the dev-launch-injector file (common isn't merged into the other ones until the game actually starts; come to think of it, maybe we should just merge them at build time rather than runtime? thought that would be kinda out of scope for this PR). |
||||||
|
||||||
public LaunchConfig property(String key, String value) { | ||||||
return property("common", key, value); | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the current AT tool not work for old MC versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require remapping the entire minecraft jar (and presumably also forge? I guess ATs can apply to it as well?) from official to srg and back, only to run the AT tool. Much slower than this implementation which basically just converts the names as it looks them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. You could also remap the AT since we have tools (CadixDev's library) for that.