NucleoidMC / fantasy

Library to support creating dimensions at runtime
GNU Lesser General Public License v3.0
95 stars 26 forks source link

chore: Port to 1.20.5/1.20.6 #44

Closed Awakened-Redstone closed 3 months ago

Awakened-Redstone commented 4 months ago

A 1.20.5/1.20.6 port

While I did test the mod, I recommend more tests and checking, specially as I had some weird crashes, but after adding debug it didn't happen again

PR content

- Port - Fix registry mixins - Fix main sources - Update to Java 21 - Updated - Fabric loader - Fabric API - Loom - Gradle - Fix test sources for 1.20.5 - Bumped required loader version - Changes - Setting the shouldTickTime option updates the world's daylight cycle gamerule - Improve test commands - Fix recursive call - Changed mod dependency to fabric-api for better mod resolution errors - Bumped mod version

Awakened-Redstone commented 4 months ago

I'm a bot busy today, but I'll look more into it tomorrow, thanks

Awakened-Redstone commented 4 months ago

If a 1.20.5 branch is made I can change the PR to point to it

Awakened-Redstone commented 4 months ago

oh crap

Awakened-Redstone commented 4 months ago

it turns out that PRs don't like branch renaming

LCLPYT commented 4 months ago

Minecraft 1.20.6 came out today. I tested your version with the latest dependency versions and it seems to work perfectly fine.

minecraft_version=1.20.6
yarn_mappings=1.20.6+build.1
loader_version=0.15.10

fabric_version=0.97.8+1.20.6

Also, I scanned the version for any occurrence of "17", to see where Java version adjustments have to be done. I found:

Could you adjust these and maybe change the PR to "Port to 1.20.6"?

Awakened-Redstone commented 4 months ago

Mixins doesn't seem to like JAVA_21 as compatibilityLevel

Awakened-Redstone commented 4 months ago

should I also bump the min required loader version? Fabric API requires a more recent one than the currently required

LCLPYT commented 4 months ago

It seems that all versions of fabric-api for 1.20.6 require at least fabric-loader 0.15.6. I'd say upgrade it as well...

LCLPYT commented 4 months ago

The problem with the JAVA_21 mixin compatibility seems to be solved when upgrading to the latest versions... Maybe the latest fabric-loader version is required? I tested 0.15.10

[19:46:25] [main/INFO] (FabricLoader/Mixin) Loaded Fabric development mappings for mixin remapper!
[19:46:25] [main/INFO] (FabricLoader/Mixin) Compatibility level set to JAVA_17
[19:46:25] [main/INFO] (FabricLoader/Mixin) Compatibility level set to JAVA_21
[19:46:26] [main/INFO] (FabricLoader/MixinExtras|Service) Initializing MixinExtras via com.llamalad7.mixinextras.service.MixinExtrasServiceImpl(version=0.3.5).
LCLPYT commented 4 months ago

I tested fantasy for 1.20.5+ for a while now. Yesterday I found that permanent dimensions are saved to the level.dat file of the host world. Before, permanent dimensions had to be restored using getOrOpenPersistentWorld. This is also documented in the README.

I think the issue is in DimensionOptionsRegistryHolderMixin: In 1.20.4, the DimensionOptionsRegistryHolder had a Registry<DimensionOptions> field. But in 1.20.5 it is a Map<RegistryKey<DimensionOptions>, DimensionOptions>. So the mixin effectively injects an invalid getter that returns a FilteredRegistry instead of a map. The reason why there wasn't an error is because fabric-api introduced a mixin, completely replacing the MapCodec and thus making the mixin from fantasy useless: https://github.com/FabricMC/fabric/blob/1.20.6/fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/mixin/dimension/DimensionOptionsRegistryHolderMixin.java

I tried to patch the fabric api mixin to also use a modified getter, but I couldn't target code from their mixin... So I decided to deleted the DimensionOptionsRegistryHolderMixin and introduced another mixin WorldGenSettingsMixin:

package xyz.nucleoid.fantasy.mixin.registry;

import com.google.common.collect.Maps;
import net.minecraft.world.dimension.DimensionOptionsRegistryHolder;
import net.minecraft.world.level.WorldGenSettings;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.ModifyArg;
import xyz.nucleoid.fantasy.FantasyDimensionOptions;

@Mixin(WorldGenSettings.class)
public class WorldGenSettingsMixin {

    @ModifyArg(method = "encode(Lcom/mojang/serialization/DynamicOps;Lnet/minecraft/world/gen/GeneratorOptions;Lnet/minecraft/world/dimension/DimensionOptionsRegistryHolder;)Lcom/mojang/serialization/DataResult;", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/WorldGenSettings;<init>(Lnet/minecraft/world/gen/GeneratorOptions;Lnet/minecraft/world/dimension/DimensionOptionsRegistryHolder;)V"), index = 1)
    private static DimensionOptionsRegistryHolder fantasy$wrapWorldGenSettings(DimensionOptionsRegistryHolder original) {
        var dimensions = original.dimensions();
        var saveDimensions = Maps.filterEntries(dimensions, entry -> FantasyDimensionOptions.SAVE_PROPERTIES_PREDICATE.test(entry.getValue()));

        return new DimensionOptionsRegistryHolder(saveDimensions);
    }
}

It targets the method that is invoked when encoding the generator options to the level.dat file. The mixin replaces the DimensionOptionsRegistryHolder argument to the new WorldGenSettings() call with a filtered instance.

Maybe you could check my approach and include it in your PR if you approve it.