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

Change unnecessary mixin overwrite into an inject in ForgeMixinDatagenModLoader #621

Closed
wants to merge 3 commits into from

Conversation

karelmikie3
Copy link

This resolves #620.

It changes overwriting the entire method to only injecting the additional if statement seen in

if (!mods.contains("forge")) {
// If we aren't generating data for forge, automatically add forge as an existing so mods can access forge's data
existingMods.add("forge");
}

This will result in a duplicate if statement like so

Bootstrap.bootStrap();
ModLoader.get().gatherAndInitializeMods(ModWorkManager.syncExecutor(), ModWorkManager.parallelExecutor(), ()->{});
if (!mods.contains("forge")) {
    // If we aren't generating data for forge, automatically add forge as an existing so mods can access forge's data
    existingMods.add("forge");
}
CompletableFuture<HolderLookup.Provider> lookupProvider = CompletableFuture.supplyAsync(VanillaRegistries::createLookup, Util.backgroundExecutor());
dataGeneratorConfig = new GatherDataEvent.DataGeneratorConfig(mods, path, inputs, lookupProvider, serverGenerators,
        clientGenerators, devToolGenerators, reportsGenerator, structureValidator, flat);
if (!mods.contains("forge")) {
    // If we aren't generating data for forge, automatically add forge as an existing so mods can access forge's data
    existingMods.add("forge");
}
existingFileHelper = new ExistingFileHelper(existingPacks, existingMods, structureValidator, assetIndex, assetsDir);

However, this isn't a problem since existingMods is a Set and thus a duplicate entry won't be added.

This change makes the code changes identical to the old one while not using overwrite and thus increasing compatibility with other mods. Like fixing the crash with Fusion and Create as seen in issue #620.

I do however have my doubts about this mixin's necessity. As the variable existingMods isn't used before the original if-statement as seen in the forge code:
https://github.com/MinecraftForge/MinecraftForge/blob/88d2dc6c8f91fd3a6b38fa06bd92cde2db1c94db/src/main/java/net/minecraftforge/data/loading/DatagenModLoader.java#L35-L55

And also isn't in the overwrite mixin:

@Overwrite(remap = false)
public static void begin(final Set<String> mods, final Path path, final Collection<Path> inputs, Collection<Path> existingPacks,
Set<String> existingMods, final boolean serverGenerators, final boolean clientGenerators, final boolean devToolGenerators, final boolean reportsGenerator,
final boolean structureValidator, final boolean flat, final String assetIndex, final File assetsDir) {
if (mods.contains("minecraft") && mods.size() == 1)
return;
LOGGER.info("Initializing Data Gatherer for mods {}", mods);
runningDataGen = true;
Bootstrap.bootStrap();
ModLoader.get().gatherAndInitializeMods(ModWorkManager.syncExecutor(), ModWorkManager.parallelExecutor(), ()->{});
if (!mods.contains("forge")) {
// If we aren't generating data for forge, automatically add forge as an existing so mods can access forge's data
existingMods.add("forge");
}
CompletableFuture<HolderLookup.Provider> lookupProvider = CompletableFuture.supplyAsync(VanillaRegistries::createLookup, Util.backgroundExecutor());
dataGeneratorConfig = new GatherDataEvent.DataGeneratorConfig(mods, path, inputs, lookupProvider, serverGenerators,
clientGenerators, devToolGenerators, reportsGenerator, structureValidator, flat);
existingFileHelper = new ExistingFileHelper(existingPacks, existingMods, structureValidator, assetIndex, assetsDir);
ModLoader.get().runEventGenerator(mc -> new GatherDataEvent(mc, dataGeneratorConfig.makeGenerator(p -> dataGeneratorConfig.isFlat() ? p : p.resolve(mc.getModId()),
dataGeneratorConfig.getMods().contains(mc.getModId())), dataGeneratorConfig, existingFileHelper));
dataGeneratorConfig.runAll();
}

So effectively this mixin seems to change nothing with respect to the (current) forge code.

@object-Object
Copy link
Member

object-Object commented Jul 23, 2024

I agree that it doesn't seem like it would be doing anything, given that code; and it seems like the datagen runs fine with that mixin removed entirely. I think I'm just going to remove it (see #706), but if that ends up causing problems later, we can definitely revisit this PR. Thank you for the analysis/effort, and sorry for the huge delay in getting this fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[1.20.1] Mod incompatibility with Fusion causing crash with Create and potentially others
2 participants