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

chore: Port to 1.20.5/1.20.6 #44

Merged
merged 25 commits into from
May 12, 2024
Merged

chore: Port to 1.20.5/1.20.6 #44

merged 25 commits into from
May 12, 2024

Conversation

Awakened-Redstone
Copy link
Contributor

@Awakened-Redstone Awakened-Redstone commented Apr 23, 2024

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

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Copy link
Contributor

@LCLPYT LCLPYT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work pretty well in general. However there is a critical bug when quitting to the main menu from a singleplayer world. The singleplayer worlds a no longer listed because of
a NullPointerException that occurs because of the SimpleRegistryMixin.
You didn't change code related to that, it just seems like 1.20.5 doesn't really like null values in the registry :)

Also, some warnings are logged when using the "Update 1.21" experiment and loading runtime worlds:

[21:38:08] [Worker-Main-6/WARN] (Minecraft) Empty or non-existent pool: minecraft:trial_chambers/chamber/addon/c6
[21:38:08] [Worker-Main-6/WARN] (Minecraft) Empty or non-existent pool: minecraft:trial_chambers/chamber/addon/c6
[21:39:05] [Worker-Main-16/WARN] (Minecraft) Empty height range: [0 above bottom--8 absolute]

Im not sure if it is the fault of this mod, or a vanilla issues though.

I also noticed trial chambers not generating properly, but this is likely because of the default dimension configs that temporary worlds have (minimum height etc.).
2024-04-23_21 39 21

@Awakened-Redstone
Copy link
Contributor Author

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

@Awakened-Redstone Awakened-Redstone marked this pull request as draft April 24, 2024 19:47
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
…e from entryToRawId

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
@Awakened-Redstone Awakened-Redstone marked this pull request as ready for review April 25, 2024 02:10
@Awakened-Redstone
Copy link
Contributor Author

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

Copy link
Contributor

@LCLPYT LCLPYT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working fine!
Your solution to the null value problem is fine I think.
Temporary worlds are also correctly deleted now.

I don't really see an issue with the rest of the code.
But I realized there is an outdated gradle dependency: modLocalRuntime "eu.pb4:polymer-reg-sync-manipulator:0.7.0+1.20.3-rc1".
Actually, I am not really sure if it is actually used in this mod...
Maybe it can be removed? Otherwise I suggest updating to version 0.8.0-beta.6+1.20.5.

Other than that, I think the port it done! 🎉

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
No difference was noticed with and without it

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
@Awakened-Redstone Awakened-Redstone deleted the branch NucleoidMC:1.20.6 April 25, 2024 14:53
@Awakened-Redstone Awakened-Redstone deleted the 1.20.4 branch April 25, 2024 14:53
@Awakened-Redstone
Copy link
Contributor Author

oh crap

@Awakened-Redstone Awakened-Redstone restored the 1.20.4 branch April 25, 2024 14:54
@Awakened-Redstone
Copy link
Contributor Author

Awakened-Redstone commented Apr 25, 2024

it turns out that PRs don't like branch renaming

Copy link
Contributor

@LCLPYT LCLPYT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, hopefully this will be merged soon.
I realized 1.20.6 is coming out next week... There should be very few changes and this is probably compatible with this PR

Bump the mod version as the PR has a small behavior change (RuntimeWorldConfig.java:78)

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Oops, I forgot about this

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
@LCLPYT
Copy link
Contributor

LCLPYT commented Apr 29, 2024

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:

  • fabric.mod.json
-"java": ">=17"
+"java": ">=21"
  • .github/build.yml
-java-version: 17
+java-version: 21
  • .github/release.yml
-java-version: 17
+java-version: 21
  • fantasy.mixins.json
-"compatibilityLevel": "JAVA_17",
+"compatibilityLevel": "JAVA_21",

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

@Awakened-Redstone Awakened-Redstone changed the title chore: Port to 1.20.5 chore: Port to 1.20.5/1.20.6 Apr 29, 2024
@Awakened-Redstone
Copy link
Contributor Author

Mixins doesn't seem to like JAVA_21 as compatibilityLevel

@Awakened-Redstone
Copy link
Contributor Author

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

@LCLPYT
Copy link
Contributor

LCLPYT commented Apr 29, 2024

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
Copy link
Contributor

LCLPYT commented Apr 29, 2024

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).

I missed some stuff

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
Update to 1.20.6, bumping mc ver as modders should use it over 1.20.5, not bumping required mc ver for compatibility

Signed-off-by: Awakened-Redstone <40528665+Awakened-Redstone@users.noreply.github.com>
@LCLPYT
Copy link
Contributor

LCLPYT commented May 4, 2024

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.

@Awakened-Redstone Awakened-Redstone requested a review from LCLPYT May 10, 2024 22:06
@Patbox Patbox changed the base branch from 1.20.4 to 1.20.6 May 12, 2024 09:43
@Patbox Patbox merged commit 7fe6963 into NucleoidMC:1.20.6 May 12, 2024
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.

3 participants