From 8aa04e13a66179039729c79c4f15f967960f536d Mon Sep 17 00:00:00 2001 From: Fury_Phoenix <64714532+Phoenix-Starlight@users.noreply.github.com> Date: Tue, 26 Dec 2023 07:13:46 -0800 Subject: [PATCH 1/5] Remove testmod genSources task (#333) --- settings.gradle | 2 ++ 1 file changed, 2 insertions(+) diff --git a/settings.gradle b/settings.gradle index c5b39d7c..514910c1 100644 --- a/settings.gradle +++ b/settings.gradle @@ -10,6 +10,8 @@ pluginManagement { include("test_agent") include("common") +startParameter.excludedTaskNames.add(':fabric:testmod:genSources') + def current_platforms = getProperty("enabled_platforms").tokenize(',') current_platforms.each { it -> def platform_name = it.trim() From 12b0d352cd9ade08ff27d77cc2806bce2620b1f8 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 26 Dec 2023 20:44:59 -0500 Subject: [PATCH 2/5] Fix some concurrency issues in Forge's ModelDataManager Model data is now refreshed in a 3x3 radius if retrieved on the main thread, but not refreshed at all if retrieved on a worker. This should emulate the old behavior well enough for most (if not all) mods, while preventing weird CMEs from accessing block entities off-thread --- .../ModelDataManagerMixin.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java new file mode 100644 index 00000000..45836c89 --- /dev/null +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java @@ -0,0 +1,54 @@ +package org.embeddedt.modernfix.forge.mixin.bugfix.model_data_manager_cme; + +import net.minecraft.client.Minecraft; +import net.minecraft.core.BlockPos; +import net.minecraft.world.level.ChunkPos; +import net.minecraft.world.level.Level; +import net.minecraftforge.client.model.ModelDataManager; +import org.embeddedt.modernfix.annotation.ClientOnlyMixin; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.Shadow; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.ModifyArg; +import org.spongepowered.asm.mixin.injection.Redirect; + +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; + +/** + * Fix several concurrency issues in the default ModelDataManager. + */ +@Mixin(ModelDataManager.class) +@ClientOnlyMixin +public abstract class ModelDataManagerMixin { + @Shadow protected static void refreshModelData(Level world, ChunkPos chunk) { + throw new AssertionError(); + } + + /** + * Make the set of positions to refresh a real concurrent hash set rather than relying on synchronizedSet, + * because the returned iterator won't be thread-safe otherwise. See https://github.com/AppliedEnergistics/Applied-Energistics-2/issues/7511 + */ + @ModifyArg(method = "requestModelDataRefresh", at = @At(value = "INVOKE", target = "Ljava/util/Map;computeIfAbsent(Ljava/lang/Object;Ljava/util/function/Function;)Ljava/lang/Object;", ordinal = 0), index = 1) + private static Function> changeTypeOfSetUsed(Function> mappingFunction) { + return pos -> Collections.newSetFromMap(new ConcurrentHashMap<>()); + } + + @Redirect(method = "getModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)Ljava/util/Map;", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/client/model/ModelDataManager;refreshModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)V")) + private static void onlyRefreshOnMainThread(Level toUpdate, ChunkPos pos) { + // Only refresh model data on the main thread. This prevents calling getBlockEntity from worker threads + // which could cause weird CMEs or other behavior. + if(Minecraft.getInstance().isSameThread()) { + // Refresh the given chunk, and all its neighbors. This is less efficient than the default code + // but we have no choice since we need to not do refreshing on workers, and blocks might + // try to access model data in neighboring chunks. + for(int x = -1; x <= 1; x++) { + for(int z = -1; z <= 1; z++) { + refreshModelData(toUpdate, new ChunkPos(pos.x + x, pos.z + z)); + } + } + } + } +} From ebf1d93422503613271a2db2301e81804fe4397a Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 26 Dec 2023 20:51:26 -0500 Subject: [PATCH 3/5] Suppress mixin remap error --- .../bugfix/model_data_manager_cme/ModelDataManagerMixin.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java index 45836c89..bc3eb24f 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java @@ -31,12 +31,12 @@ public abstract class ModelDataManagerMixin { * Make the set of positions to refresh a real concurrent hash set rather than relying on synchronizedSet, * because the returned iterator won't be thread-safe otherwise. See https://github.com/AppliedEnergistics/Applied-Energistics-2/issues/7511 */ - @ModifyArg(method = "requestModelDataRefresh", at = @At(value = "INVOKE", target = "Ljava/util/Map;computeIfAbsent(Ljava/lang/Object;Ljava/util/function/Function;)Ljava/lang/Object;", ordinal = 0), index = 1) + @ModifyArg(method = "requestModelDataRefresh", at = @At(value = "INVOKE", target = "Ljava/util/Map;computeIfAbsent(Ljava/lang/Object;Ljava/util/function/Function;)Ljava/lang/Object;", ordinal = 0), index = 1, remap = false) private static Function> changeTypeOfSetUsed(Function> mappingFunction) { return pos -> Collections.newSetFromMap(new ConcurrentHashMap<>()); } - @Redirect(method = "getModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)Ljava/util/Map;", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/client/model/ModelDataManager;refreshModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)V")) + @Redirect(method = "getModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)Ljava/util/Map;", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/client/model/ModelDataManager;refreshModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)V"), remap = false) private static void onlyRefreshOnMainThread(Level toUpdate, ChunkPos pos) { // Only refresh model data on the main thread. This prevents calling getBlockEntity from worker threads // which could cause weird CMEs or other behavior. From ae561db9e3c10bb317a342f7f417bff17d35f4fa Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 26 Dec 2023 20:52:23 -0500 Subject: [PATCH 4/5] Update mixin for 1.19 model data changes --- .../ModelDataManagerMixin.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java index 45836c89..b0b5843e 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/bugfix/model_data_manager_cme/ModelDataManagerMixin.java @@ -3,8 +3,7 @@ package org.embeddedt.modernfix.forge.mixin.bugfix.model_data_manager_cme; import net.minecraft.client.Minecraft; import net.minecraft.core.BlockPos; import net.minecraft.world.level.ChunkPos; -import net.minecraft.world.level.Level; -import net.minecraftforge.client.model.ModelDataManager; +import net.minecraftforge.client.model.data.ModelDataManager; import org.embeddedt.modernfix.annotation.ClientOnlyMixin; import org.spongepowered.asm.mixin.Mixin; import org.spongepowered.asm.mixin.Shadow; @@ -23,21 +22,19 @@ import java.util.function.Function; @Mixin(ModelDataManager.class) @ClientOnlyMixin public abstract class ModelDataManagerMixin { - @Shadow protected static void refreshModelData(Level world, ChunkPos chunk) { - throw new AssertionError(); - } + @Shadow protected abstract void refreshAt(ChunkPos chunk); /** * Make the set of positions to refresh a real concurrent hash set rather than relying on synchronizedSet, * because the returned iterator won't be thread-safe otherwise. See https://github.com/AppliedEnergistics/Applied-Energistics-2/issues/7511 */ - @ModifyArg(method = "requestModelDataRefresh", at = @At(value = "INVOKE", target = "Ljava/util/Map;computeIfAbsent(Ljava/lang/Object;Ljava/util/function/Function;)Ljava/lang/Object;", ordinal = 0), index = 1) + @ModifyArg(method = "requestRefresh", at = @At(value = "INVOKE", target = "Ljava/util/Map;computeIfAbsent(Ljava/lang/Object;Ljava/util/function/Function;)Ljava/lang/Object;", ordinal = 0), index = 1, remap = false) private static Function> changeTypeOfSetUsed(Function> mappingFunction) { return pos -> Collections.newSetFromMap(new ConcurrentHashMap<>()); } - @Redirect(method = "getModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)Ljava/util/Map;", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/client/model/ModelDataManager;refreshModelData(Lnet/minecraft/world/level/Level;Lnet/minecraft/world/level/ChunkPos;)V")) - private static void onlyRefreshOnMainThread(Level toUpdate, ChunkPos pos) { + @Redirect(method = "getAt(Lnet/minecraft/world/level/ChunkPos;)Ljava/util/Map;", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/client/model/data/ModelDataManager;refreshAt(Lnet/minecraft/world/level/ChunkPos;)V"), remap = false) + private void onlyRefreshOnMainThread(ModelDataManager instance, ChunkPos pos) { // Only refresh model data on the main thread. This prevents calling getBlockEntity from worker threads // which could cause weird CMEs or other behavior. if(Minecraft.getInstance().isSameThread()) { @@ -46,7 +43,7 @@ public abstract class ModelDataManagerMixin { // try to access model data in neighboring chunks. for(int x = -1; x <= 1; x++) { for(int z = -1; z <= 1; z++) { - refreshModelData(toUpdate, new ChunkPos(pos.x + x, pos.z + z)); + refreshAt(new ChunkPos(pos.x + x, pos.z + z)); } } } From 152cdc4469cb7ae0032180d5a2c79681a3d573d4 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Wed, 27 Dec 2023 15:19:13 -0500 Subject: [PATCH 5/5] Disable ModelDataManager fixes if Rubidium is installed Since legacy Rubidium versions only retrieve model data on a worker thread, the data will probably never be refreshed, causing rendering issues. --- .../modernfix/core/config/ModernFixEarlyConfig.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java b/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java index 8a6e3942..f40c07b7 100644 --- a/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java +++ b/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java @@ -231,6 +231,7 @@ public class ModernFixEarlyConfig { } checkBlockstateCacheRebuilds(); + checkModelDataManager(); } private void checkBlockstateCacheRebuilds() { @@ -246,6 +247,16 @@ public class ModernFixEarlyConfig { } } + private void checkModelDataManager() { + if(!isFabric && modPresent("rubidium") && !modPresent("embeddium")) { + Option option = this.options.get("mixin.bugfix.model_data_manager_cme"); + if(option != null) { + LOGGER.warn("ModelDataManager bugfixes have been disabled to prevent broken rendering with Rubidium installed. Please migrate to Embeddium."); + option.addModOverride(false, "rubidium"); + } + } + } + private void disableIfModPresent(String configName, String... ids) { for(String id : ids) { if(!ModernFixPlatformHooks.INSTANCE.isEarlyLoadingNormally() || modPresent(id)) {