From 5ca9485f0b5bb0f03b23db172c87fcecf046d810 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Wed, 5 Jul 2023 21:52:02 -0400 Subject: [PATCH 1/8] Add null check for ClassInfo objects --- .../java/org/embeddedt/modernfix/util/ClassInfoManager.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/src/main/java/org/embeddedt/modernfix/util/ClassInfoManager.java b/common/src/main/java/org/embeddedt/modernfix/util/ClassInfoManager.java index 2f458dca..8efaa4f1 100644 --- a/common/src/main/java/org/embeddedt/modernfix/util/ClassInfoManager.java +++ b/common/src/main/java/org/embeddedt/modernfix/util/ClassInfoManager.java @@ -49,6 +49,8 @@ public class ClassInfoManager { if(entry.getKey().equals("java/lang/Object")) return false; ClassInfo mixinClz = entry.getValue(); + if(mixinClz == null) + return true; try { if(mixinClz.isMixin()) { // clear classNode in MixinInfo.State From 582f17c0e7549b7dada66ee10f8c38af42f276e3 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 14:42:51 -0400 Subject: [PATCH 2/8] Update Loom to prepare for unit tests --- .gitignore | 1 + build.gradle | 2 +- fabric/build.gradle | 23 ++++++++++++--- .../modernfix/blocks/BlockStateCacheTest.java | 28 +++++++++++++++++++ forge/build.gradle | 6 ++-- gradle.properties | 2 ++ gradle/wrapper/gradle-wrapper.properties | 2 +- 7 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java diff --git a/.gitignore b/.gitignore index c70bdc40..3241d732 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ libs media classes/ .architectury-transformer/ +fabric/fabricloader.log # Changelog CHANGELOG.md diff --git a/build.gradle b/build.gradle index ecff6d38..d9368c1b 100644 --- a/build.gradle +++ b/build.gradle @@ -1,6 +1,6 @@ plugins { id "architectury-plugin" version "3.4-SNAPSHOT" - id "dev.architectury.loom" version "1.1-SNAPSHOT" apply false + id "dev.architectury.loom" version "1.2-SNAPSHOT" apply false id "maven-publish" id 'com.matthewprenger.cursegradle' version '1.4.0' apply false id 'com.palantir.git-version' version '1.0.0' diff --git a/fabric/build.gradle b/fabric/build.gradle index 69002e2e..02c657f9 100644 --- a/fabric/build.gradle +++ b/fabric/build.gradle @@ -26,6 +26,8 @@ configurations { dependencies { modImplementation "net.fabricmc:fabric-loader:${rootProject.fabric_loader_version}" + testImplementation "net.fabricmc:fabric-loader-junit:${rootProject.fabric_loader_version}" + modIncludeImplementation(fabricApi.module("fabric-api-base", rootProject.fabric_api_version)) { exclude group: 'net.fabricmc', module: 'fabric-loader' } modIncludeImplementation(fabricApi.module("fabric-lifecycle-events-v1", rootProject.fabric_api_version)) { exclude group: 'net.fabricmc', module: 'fabric-loader' } modIncludeImplementation(fabricApi.module("fabric-screen-api-v1", rootProject.fabric_api_version)) { exclude group: 'net.fabricmc', module: 'fabric-loader' } @@ -38,8 +40,21 @@ dependencies { // Remove the next line if you don't want to depend on the API // modApi "me.shedaniel:architectury-fabric:${rootProject.architectury_version}" - common(project(path: ":common", configuration: "namedElements")) { transitive false } + testImplementation(common(project(path: ":common", configuration: "namedElements"))) { transitive false } shadowCommon(project(path: ":common", configuration: "transformProductionFabric")) { transitive false } + + testImplementation(platform("org.junit:junit-bom:${project.junit_version}")) + testImplementation("org.junit.jupiter:junit-jupiter-api") + testImplementation("org.junit.jupiter:junit-jupiter-params") + testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine") + testImplementation("org.junit.platform:junit-platform-launcher") + testImplementation("org.assertj:assertj-core:3.19.0") + testImplementation("com.google.guava:guava-testlib:21.0") + testImplementation("org.mockito:mockito-junit-jupiter:5.3.1") +} + +test { + useJUnitPlatform() } processResources { @@ -54,18 +69,18 @@ shadowJar { exclude "architectury.common.json" configurations = [project.configurations.shadowCommon] - classifier "dev-shadow" + archiveClassifier.set("dev-shadow") } remapJar { injectAccessWidener = true input.set shadowJar.archiveFile dependsOn shadowJar - classifier null + archiveClassifier.set(null) } jar { - classifier "dev" + archiveClassifier.set("dev") } components.java { diff --git a/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java b/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java new file mode 100644 index 00000000..f45fb0c7 --- /dev/null +++ b/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java @@ -0,0 +1,28 @@ +/* +package org.embeddedt.modernfix.blocks; + +import net.minecraft.DetectedVersion; +import net.minecraft.core.Registry; +import net.minecraft.resources.ResourceLocation; +import net.minecraft.server.Bootstrap; +import net.minecraft.world.item.Items; +import org.junit.Test; +import org.junit.jupiter.api.BeforeAll; + +import static org.junit.jupiter.api.Assertions.*; + +public class BlockStateCacheTest { + @BeforeAll + public static void setup() { + DetectedVersion.tryDetectVersion(); + Bootstrap.bootStrap(); + } + + @Test + public void testItemRegistry() { + ResourceLocation location = Registry.ITEM.getKey(Items.DIAMOND); + assertEquals(location.toString(), "minecraft:diamond"); + } +} + + */ \ No newline at end of file diff --git a/forge/build.gradle b/forge/build.gradle index 18e81c6e..3868a69d 100644 --- a/forge/build.gradle +++ b/forge/build.gradle @@ -69,17 +69,17 @@ shadowJar { exclude "architectury.common.json" configurations = [project.configurations.shadowCommon] - classifier "dev-shadow" + archiveClassifier.set("dev-shadow") } remapJar { input.set shadowJar.archiveFile dependsOn shadowJar - classifier null + archiveClassifier.set(null) } jar { - classifier "dev" + archiveClassifier.set("dev") manifest { attributes([ "Specification-Title" : "modernfix", diff --git a/gradle.properties b/gradle.properties index 2df38d00..e496e4ef 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,6 +1,8 @@ # Done to increase the memory available to gradle. org.gradle.jvmargs=-Xmx2G +junit_version=5.10.0-M1 + mod_id=modernfix minecraft_version=1.16.5 enabled_platforms=fabric,forge diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index aa991fce..fae08049 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,5 +1,5 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-7.4.2-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-bin.zip zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists From e02382931005bda664af00fc4e183593d5adc32d Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 15:11:09 -0400 Subject: [PATCH 3/8] Upgrade Fabric Loader to 0.14.21 --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index e496e4ef..507af8cf 100644 --- a/gradle.properties +++ b/gradle.properties @@ -16,7 +16,7 @@ kubejs_version=1605.3.19-build.299 ctm_version=MC1.16.1-1.1.2.6 supported_minecraft_versions=1.16.4,1.16.5 -fabric_loader_version=0.14.18 +fabric_loader_version=0.14.21 fabric_api_version=0.42.0+1.16 modmenu_version=1.16.23 From 03b23957827c42d5df5a11f3d07f807c5343e87e Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 18:42:50 -0400 Subject: [PATCH 4/8] Add custom agent to work around https://github.com/FabricMC/fabric-loader/issues/817 --- .gitignore | 1 + build.gradle | 2 +- fabric/build.gradle | 16 ++++ settings.gradle | 1 + test_agent/build.gradle | 86 +++++++++++++++++++ .../embeddedt/modernfix/testing/Agent.java | 61 +++++++++++++ .../modernfix/testing/AgentHooks.java | 17 ++++ 7 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 test_agent/build.gradle create mode 100644 test_agent/src/main/java/org/embeddedt/modernfix/testing/Agent.java create mode 100644 test_agent/src/main/java/org/embeddedt/modernfix/testing/AgentHooks.java diff --git a/.gitignore b/.gitignore index 3241d732..f427fdd2 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ media classes/ .architectury-transformer/ fabric/fabricloader.log +fabric/test_run # Changelog CHANGELOG.md diff --git a/build.gradle b/build.gradle index d9368c1b..d383f731 100644 --- a/build.gradle +++ b/build.gradle @@ -93,7 +93,7 @@ allprojects { } } -subprojects { +configure(subprojects.findAll {it.name == "common" || it.name == "forge" || it.name == "fabric"}) { apply plugin: "dev.architectury.loom" loom { diff --git a/fabric/build.gradle b/fabric/build.gradle index 02c657f9..1b813a9e 100644 --- a/fabric/build.gradle +++ b/fabric/build.gradle @@ -22,6 +22,10 @@ configurations { include.extendsFrom modIncludeImplementation modImplementation.extendsFrom modIncludeImplementation + + testAgent { + canBeConsumed = false + } } dependencies { @@ -51,10 +55,22 @@ dependencies { testImplementation("org.assertj:assertj-core:3.19.0") testImplementation("com.google.guava:guava-testlib:21.0") testImplementation("org.mockito:mockito-junit-jupiter:5.3.1") + + testAgent(project("path": ":test_agent", "configuration": "agentJar")) } test { useJUnitPlatform() + def runDir = file('test_run') + doFirst() { + runDir.mkdir() + } + workingDir = runDir + + // inject our custom agent to fix #817 + FileCollection agentFile = configurations.getByName("testAgent") + jvmArgs "-javaagent:${agentFile.singleFile.absolutePath}" + dependsOn(agentFile) } processResources { diff --git a/settings.gradle b/settings.gradle index 8a77caf9..4afff011 100644 --- a/settings.gradle +++ b/settings.gradle @@ -7,6 +7,7 @@ pluginManagement { } } +include("test_agent") include("common") include("fabric") include("forge") diff --git a/test_agent/build.gradle b/test_agent/build.gradle new file mode 100644 index 00000000..2141b87d --- /dev/null +++ b/test_agent/build.gradle @@ -0,0 +1,86 @@ +plugins { + //id 'com.github.johnrengelman.shadow' version '7.1.2' + id 'java' +} + +group 'org.embeddedt' +archivesBaseName = 'modernfix-test-agent' +version '1.0' + +sourceCompatibility = '1.8' +targetCompatibility = '1.8' + +repositories { + mavenCentral() + mavenLocal() + maven { + name = 'forge' + url = 'https://maven.minecraftforge.net/' + } +} + +/* + +shadowJar { + relocate 'net.bytebuddy.agent', 'org.embeddedt.modernfix.testing.shadow.bytebuddyagent' + relocate 'org.objectweb.asm', 'org.embeddedt.modernfix.testing.shadow.asm' +} + + + +shadowJar { + project.configurations.implementation.canBeResolved = true + configurations = [project.configurations.implementation] +} + */ +dependencies { + compileOnly "net.fabricmc:fabric-loader:${rootProject.fabric_loader_version}" + implementation "org.ow2.asm:asm-tree:9.1" + implementation "org.ow2.asm:asm-commons:9.1" + implementation "org.ow2.asm:asm-util:9.1" + + //implementation('net.bytebuddy:byte-buddy-agent:1.12.22') +} + +tasks.withType(JavaCompile) { + // ensure that the encoding is set to UTF-8, no matter what the system default is + // this fixes some edge cases with special characters not displaying correctly + // see http://yodaconditions.net/blog/fix-for-java-file-encoding-problems-with-gradle.html + // If Javadoc is generated, this must be specified in that task too. + options.encoding = "UTF-8" +} + +jar { + manifest { + attributes( + "Premain-Class": "org.embeddedt.modernfix.testing.Agent", + "Can-Redefine-Classes": false, + "Can-Set-Native-Method-Prefix": false + ) + } +} +/* + +shadowJar { + archiveBaseName.set('modernfix-test-agent') + archiveClassifier.set('') + archiveVersion.set('v1') +} + + */ + +configurations { + agentJar { + canBeConsumed = true + canBeResolved = false + } +} + +artifacts { + agentJar(jar) +} +/* +project.tasks.shadowJar.dependsOn build +defaultTasks 'shadowJar' + + */ \ No newline at end of file diff --git a/test_agent/src/main/java/org/embeddedt/modernfix/testing/Agent.java b/test_agent/src/main/java/org/embeddedt/modernfix/testing/Agent.java new file mode 100644 index 00000000..b7b69479 --- /dev/null +++ b/test_agent/src/main/java/org/embeddedt/modernfix/testing/Agent.java @@ -0,0 +1,61 @@ +package org.embeddedt.modernfix.testing; + +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.MethodInsnNode; +import org.objectweb.asm.tree.MethodNode; + +import java.lang.instrument.ClassFileTransformer; +import java.lang.instrument.IllegalClassFormatException; +import java.lang.instrument.Instrumentation; +import java.security.ProtectionDomain; +import java.util.ListIterator; + +public class Agent { + /** + * Simple agent that transforms Fabric Loader to never mark game JARs as system libraries. + * + * Ugly, but usable workaround for issue #817 + * on the Loader bug tracker. + */ + public static void premain(String args, Instrumentation instrumentation) { + instrumentation.addTransformer(new ClassFileTransformer() { + @Override + public byte[] transform(ClassLoader loader, String className, Class classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) throws IllegalClassFormatException { + if(className.equals("net/fabricmc/loader/impl/game/LibClassifier")) { + ClassNode node = new ClassNode(); + ClassReader reader = new ClassReader(classfileBuffer); + reader.accept(node, 0); + for(MethodNode m : node.methods) { + if(m.name.equals("")) { + ListIterator iter = m.instructions.iterator(); + int addMatches = 0; + while(iter.hasNext()) { + AbstractInsnNode n = iter.next(); + if(n instanceof MethodInsnNode) { + MethodInsnNode invokeNode = (MethodInsnNode)n; + if(invokeNode.name.equals("add") && invokeNode.owner.equals("java/util/Set") && invokeNode.desc.equals("(Ljava/lang/Object;)Z")) { + addMatches++; + if(addMatches == 2) { + iter.set(new MethodInsnNode(Opcodes.INVOKESTATIC, "org/embeddedt/modernfix/testing/AgentHooks", "addLibraryWithCheck", "(Ljava/util/Set;Ljava/lang/Object;)Z", false)); + break; + } + } + } + } + } + } + ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES | ClassWriter.COMPUTE_MAXS); + node.accept(writer); + byte[] finalArray = writer.toByteArray(); + //dumpDebugClass(className, finalArray); + return finalArray; + } + return classfileBuffer; + } + }); + } +} diff --git a/test_agent/src/main/java/org/embeddedt/modernfix/testing/AgentHooks.java b/test_agent/src/main/java/org/embeddedt/modernfix/testing/AgentHooks.java new file mode 100644 index 00000000..05b8de12 --- /dev/null +++ b/test_agent/src/main/java/org/embeddedt/modernfix/testing/AgentHooks.java @@ -0,0 +1,17 @@ +package org.embeddedt.modernfix.testing; + +import java.nio.file.Path; +import java.util.Set; + +@SuppressWarnings("unused") +public class AgentHooks { + @SuppressWarnings({"unchecked", "rawtypes" }) + public static boolean addLibraryWithCheck(Set pathSet, Object path) { + boolean shouldAdd; + if(path instanceof Path) { + shouldAdd = !((Path)path).toString().contains("minecraft-merged"); + } else + shouldAdd = true; + return shouldAdd && pathSet.add(path); + } +} From ff39e9022ba5d3d12f971757c4c0f5f643915395 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 19:26:44 -0400 Subject: [PATCH 5/8] Get testing working, add tests for blockstate cache rebuilds --- .../BlockStateBaseMixin.java | 5 ++ .../core/config/ModernFixEarlyConfig.java | 24 ++++---- .../embeddedt/modernfix/duck/IBlockState.java | 1 + fabric/build.gradle | 6 +- .../modernfix/ModernFixPreLaunchFabric.java | 4 ++ .../block/state/BlockStateCacheTest.java | 58 +++++++++++++++++++ .../modernfix/blocks/BlockStateCacheTest.java | 28 --------- .../testing/util/BootstrapMinecraft.java | 14 +++++ .../util/BootstrapMinecraftExtension.java | 21 +++++++ 9 files changed, 120 insertions(+), 41 deletions(-) create mode 100644 fabric/src/test/java/net/minecraft/world/level/block/state/BlockStateCacheTest.java delete mode 100644 fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java create mode 100644 fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraft.java create mode 100644 fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java diff --git a/common/src/main/java/org/embeddedt/modernfix/common/mixin/perf/reduce_blockstate_cache_rebuilds/BlockStateBaseMixin.java b/common/src/main/java/org/embeddedt/modernfix/common/mixin/perf/reduce_blockstate_cache_rebuilds/BlockStateBaseMixin.java index 576159a3..db024b63 100644 --- a/common/src/main/java/org/embeddedt/modernfix/common/mixin/perf/reduce_blockstate_cache_rebuilds/BlockStateBaseMixin.java +++ b/common/src/main/java/org/embeddedt/modernfix/common/mixin/perf/reduce_blockstate_cache_rebuilds/BlockStateBaseMixin.java @@ -25,6 +25,11 @@ public abstract class BlockStateBaseMixin implements IBlockState { cacheInvalid = true; } + @Override + public boolean isCacheInvalid() { + return cacheInvalid; + } + private BlockBehaviour.BlockStateBase.Cache generateCache(BlockBehaviour.BlockStateBase base) { if(cacheInvalid) { // Ensure that only one block's cache is built at a time 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 fa0783ea..ad237a34 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 @@ -316,19 +316,21 @@ public class ModernFixEarlyConfig { public static ModernFixEarlyConfig load(File file) { ModernFixEarlyConfig config = new ModernFixEarlyConfig(file); Properties props = new Properties(); - if(file.exists()) { - try (FileInputStream fin = new FileInputStream(file)){ - props.load(fin); - } catch (IOException e) { - throw new RuntimeException("Could not load config file", e); + if(!Boolean.getBoolean("modernfix.ignoreConfigForTesting")) { + if(file.exists()) { + try (FileInputStream fin = new FileInputStream(file)){ + props.load(fin); + } catch (IOException e) { + throw new RuntimeException("Could not load config file", e); + } + config.readProperties(props); } - config.readProperties(props); - } - try { - config.save(); - } catch (IOException e) { - LOGGER.warn("Could not write configuration file", e); + try { + config.save(); + } catch (IOException e) { + LOGGER.warn("Could not write configuration file", e); + } } return config; diff --git a/common/src/main/java/org/embeddedt/modernfix/duck/IBlockState.java b/common/src/main/java/org/embeddedt/modernfix/duck/IBlockState.java index d396f128..ea38b50e 100644 --- a/common/src/main/java/org/embeddedt/modernfix/duck/IBlockState.java +++ b/common/src/main/java/org/embeddedt/modernfix/duck/IBlockState.java @@ -3,4 +3,5 @@ package org.embeddedt.modernfix.duck; public interface IBlockState { void clearCache(); + boolean isCacheInvalid(); } diff --git a/fabric/build.gradle b/fabric/build.gradle index 1b813a9e..a17fb87f 100644 --- a/fabric/build.gradle +++ b/fabric/build.gradle @@ -1,5 +1,6 @@ plugins { id "com.github.johnrengelman.shadow" version "7.1.2" + id 'com.adarshr.test-logger' version '3.2.0' } architectury { @@ -44,8 +45,8 @@ dependencies { // Remove the next line if you don't want to depend on the API // modApi "me.shedaniel:architectury-fabric:${rootProject.architectury_version}" - testImplementation(common(project(path: ":common", configuration: "namedElements"))) { transitive false } - shadowCommon(project(path: ":common", configuration: "transformProductionFabric")) { transitive false } + common(project(path: ":common", configuration: "namedElements")) { transitive false } + testImplementation(shadowCommon(project(path: ":common", configuration: "transformProductionFabric"))) { transitive false } testImplementation(platform("org.junit:junit-bom:${project.junit_version}")) testImplementation("org.junit.jupiter:junit-jupiter-api") @@ -66,6 +67,7 @@ test { runDir.mkdir() } workingDir = runDir + systemProperty 'modernfix.ignoreConfigForTesting', 'true' // inject our custom agent to fix #817 FileCollection agentFile = configurations.getByName("testAgent") diff --git a/fabric/src/main/java/org/embeddedt/modernfix/ModernFixPreLaunchFabric.java b/fabric/src/main/java/org/embeddedt/modernfix/ModernFixPreLaunchFabric.java index 8251d6d7..068cad51 100644 --- a/fabric/src/main/java/org/embeddedt/modernfix/ModernFixPreLaunchFabric.java +++ b/fabric/src/main/java/org/embeddedt/modernfix/ModernFixPreLaunchFabric.java @@ -9,6 +9,10 @@ import org.embeddedt.modernfix.util.CommonModUtil; public class ModernFixPreLaunchFabric implements PreLaunchEntrypoint { @Override public void onPreLaunch() { + if(ModernFixMixinPlugin.instance == null) { + System.err.println("Mixin plugin not loaded yet"); + return; + } if(ModernFixMixinPlugin.instance.isOptionEnabled("feature.spark_profile_launch.OnFabric")) { CommonModUtil.runWithoutCrash(() -> SparkLaunchProfiler.start("launch"), "Failed to start profiler"); } diff --git a/fabric/src/test/java/net/minecraft/world/level/block/state/BlockStateCacheTest.java b/fabric/src/test/java/net/minecraft/world/level/block/state/BlockStateCacheTest.java new file mode 100644 index 00000000..c6884fe2 --- /dev/null +++ b/fabric/src/test/java/net/minecraft/world/level/block/state/BlockStateCacheTest.java @@ -0,0 +1,58 @@ +package net.minecraft.world.level.block.state; + +import net.minecraft.core.BlockPos; +import net.minecraft.world.level.EmptyBlockGetter; +import net.minecraft.world.level.block.Blocks; +import org.embeddedt.modernfix.duck.IBlockState; +import org.embeddedt.modernfix.testing.util.BootstrapMinecraft; +import org.junit.jupiter.api.*; + +import static org.junit.jupiter.api.Assertions.*; + +@BootstrapMinecraft +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +public class BlockStateCacheTest { + @BeforeEach + public void rebuildCache() { + Blocks.rebuildCache(); + } + + /** + * Initially, the cache should be invalid, and null. + */ + @Test + @Order(1) + public void testCacheNullInitially() { + BlockState stoneBlock = Blocks.STONE.defaultBlockState(); + assertTrue(((IBlockState)stoneBlock).isCacheInvalid()); + assertNull(stoneBlock.cache); + } + + /** + * When an API that needs the cache is called, it should be built and the invalid flag + * becomes false. + */ + @Test + @Order(2) + public void testCacheBuiltByRequest() { + BlockState stoneBlock = Blocks.STONE.defaultBlockState(); + stoneBlock.getCollisionShape(EmptyBlockGetter.INSTANCE, BlockPos.ZERO); + assertFalse(((IBlockState)stoneBlock).isCacheInvalid()); + assertNotNull(stoneBlock.cache); + } + + /** + * When a second rebuild occurs, the invalid flag should be set to true, but the old cache + * is not set to null, in order to prevent NPEs if a second thread is accessing the cache + * when this takes place. + */ + @Test + @Order(3) + public void testCacheInvalidatedByLateRebuild() { + BlockState stoneBlock = Blocks.STONE.defaultBlockState(); + stoneBlock.getCollisionShape(EmptyBlockGetter.INSTANCE, BlockPos.ZERO); + Blocks.rebuildCache(); + assertTrue(((IBlockState)stoneBlock).isCacheInvalid()); + assertNotNull(stoneBlock.cache); + } +} diff --git a/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java b/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java deleted file mode 100644 index f45fb0c7..00000000 --- a/fabric/src/test/java/org/embeddedt/modernfix/blocks/BlockStateCacheTest.java +++ /dev/null @@ -1,28 +0,0 @@ -/* -package org.embeddedt.modernfix.blocks; - -import net.minecraft.DetectedVersion; -import net.minecraft.core.Registry; -import net.minecraft.resources.ResourceLocation; -import net.minecraft.server.Bootstrap; -import net.minecraft.world.item.Items; -import org.junit.Test; -import org.junit.jupiter.api.BeforeAll; - -import static org.junit.jupiter.api.Assertions.*; - -public class BlockStateCacheTest { - @BeforeAll - public static void setup() { - DetectedVersion.tryDetectVersion(); - Bootstrap.bootStrap(); - } - - @Test - public void testItemRegistry() { - ResourceLocation location = Registry.ITEM.getKey(Items.DIAMOND); - assertEquals(location.toString(), "minecraft:diamond"); - } -} - - */ \ No newline at end of file diff --git a/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraft.java b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraft.java new file mode 100644 index 00000000..a1710398 --- /dev/null +++ b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraft.java @@ -0,0 +1,14 @@ +package org.embeddedt.modernfix.testing.util; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import org.junit.jupiter.api.extension.ExtendWith; + +@Retention(RetentionPolicy.RUNTIME) +@Target(ElementType.TYPE) +@ExtendWith({ BootstrapMinecraftExtension.class }) +public @interface BootstrapMinecraft { +} diff --git a/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java new file mode 100644 index 00000000..0971888f --- /dev/null +++ b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java @@ -0,0 +1,21 @@ +package org.embeddedt.modernfix.testing.util; + +import net.minecraft.DetectedVersion; +import net.minecraft.server.Bootstrap; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.Extension; +import org.junit.jupiter.api.extension.ExtensionContext; + +public class BootstrapMinecraftExtension implements Extension, BeforeAllCallback, AfterAllCallback { + @Override + public void beforeAll(ExtensionContext context) throws Exception { + DetectedVersion.tryDetectVersion(); + Bootstrap.bootStrap(); + } + + @Override + public void afterAll(ExtensionContext context) throws Exception { + + } +} From db4f6738ee162788cae893371cbf68f9cc0587e1 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 19:35:36 -0400 Subject: [PATCH 6/8] Credit AE2 for JUnit bootstrap extension design --- .../modernfix/testing/util/BootstrapMinecraftExtension.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java index 0971888f..a2417e3c 100644 --- a/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java +++ b/fabric/src/test/java/org/embeddedt/modernfix/testing/util/BootstrapMinecraftExtension.java @@ -7,6 +7,9 @@ import org.junit.jupiter.api.extension.BeforeAllCallback; import org.junit.jupiter.api.extension.Extension; import org.junit.jupiter.api.extension.ExtensionContext; +/** + * Simple extension to run vanilla bootstrap, inspired by AE2. + */ public class BootstrapMinecraftExtension implements Extension, BeforeAllCallback, AfterAllCallback { @Override public void beforeAll(ExtensionContext context) throws Exception { From 1b6880ed9fbe3f1a10ae7c5452746698999ebc14 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 20:43:35 -0400 Subject: [PATCH 7/8] Make modelRegistry.keySet() more accurate on Forge when dynamic resources is on --- .../dynresources/ModelBakeEventHelper.java | 51 +++++++++++++++++++ .../ForgeHooksClientMixin.java | 22 ++++++++ .../ctm/TextureMetadataHandlerMixin.java | 7 ++- 3 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java create mode 100644 forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java new file mode 100644 index 00000000..51ad6bf9 --- /dev/null +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java @@ -0,0 +1,51 @@ +package org.embeddedt.modernfix.forge.dynresources; + +import com.google.common.collect.ForwardingMap; +import net.minecraft.client.resources.model.BakedModel; +import net.minecraft.resources.ResourceLocation; +import net.minecraft.world.item.Item; +import net.minecraft.world.level.block.Block; +import net.minecraft.world.level.block.state.BlockState; +import net.minecraftforge.registries.ForgeRegistries; +import org.embeddedt.modernfix.dynamicresources.ModelLocationCache; +import org.jetbrains.annotations.Nullable; + +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +public class ModelBakeEventHelper { + public static Map wrapRegistry(Map modelRegistry) { + Set topLevelModelLocations = new HashSet<>(modelRegistry.keySet()); + for(Block block : ForgeRegistries.BLOCKS) { + for(BlockState state : block.getStateDefinition().getPossibleStates()) { + topLevelModelLocations.add(ModelLocationCache.get(state)); + } + } + for(Item item : ForgeRegistries.ITEMS) { + topLevelModelLocations.add(ModelLocationCache.get(item)); + } + return new ForwardingMap() { + @Override + protected Map delegate() { + return modelRegistry; + } + + @Override + public Set keySet() { + return topLevelModelLocations; + } + + @Override + public boolean containsKey(@Nullable Object key) { + return topLevelModelLocations.contains(key) || super.containsKey(key); + } + + @Override + public BakedModel put(ResourceLocation key, BakedModel value) { + topLevelModelLocations.add(key); + return super.put(key, value); + } + }; + } +} diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java new file mode 100644 index 00000000..1e130b7f --- /dev/null +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java @@ -0,0 +1,22 @@ +package org.embeddedt.modernfix.forge.mixin.perf.dynamic_resources; + +import net.minecraft.client.resources.model.BakedModel; +import net.minecraft.resources.ResourceLocation; +import net.minecraftforge.client.ForgeHooksClient; +import org.embeddedt.modernfix.forge.dynresources.ModelBakeEventHelper; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.ModifyVariable; + +import java.util.Map; + +@Mixin(ForgeHooksClient.class) +public class ForgeHooksClientMixin { + /** + * Generate a more realistic keySet that contains every item and block model location, to help with mod compat. + */ + @ModifyVariable(method = "onModelBake", at = @At("HEAD"), ordinal = 0, argsOnly = true) + private static Map generateModelKeySet(Map modelRegistry) { + return ModelBakeEventHelper.wrapRegistry(modelRegistry); + } +} diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ctm/TextureMetadataHandlerMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ctm/TextureMetadataHandlerMixin.java index c7ecb7ab..8d79ddba 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ctm/TextureMetadataHandlerMixin.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ctm/TextureMetadataHandlerMixin.java @@ -12,7 +12,6 @@ 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.Inject; -import org.spongepowered.asm.mixin.injection.Redirect; import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; import team.chisel.ctm.CTM; import team.chisel.ctm.client.model.AbstractCTMBakedModel; @@ -36,9 +35,9 @@ public abstract class TextureMetadataHandlerMixin implements ModernFixClientInte ModernFixClient.CLIENT_INTEGRATIONS.add(this); } - @Redirect(method = "onModelBake", at = @At(value = "INVOKE", target = "Lnet/minecraft/client/resources/model/BakedModel;isCustomRenderer()Z")) - private boolean checkModelValid(BakedModel model) { - return model == null || model.isCustomRenderer(); + @Inject(method = "onModelBake", at = @At("HEAD"), cancellable = true, remap = false) + private void noIteration(CallbackInfo ci) { + ci.cancel(); } @Override From 16d317af975231d23e8c14d40a21d65e015bd3ce Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 6 Jul 2023 21:12:59 -0400 Subject: [PATCH 8/8] Only provide each mod its own model list in ModelBakeEvent --- .../dynresources/ModelBakeEventHelper.java | 51 +++++++++++++++---- .../ForgeHooksClientMixin.java | 28 ++++++++-- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java index 51ad6bf9..c2b3de68 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java @@ -1,22 +1,37 @@ package org.embeddedt.modernfix.forge.dynresources; import com.google.common.collect.ForwardingMap; +import com.google.common.collect.Sets; +import com.google.common.graph.GraphBuilder; +import com.google.common.graph.MutableGraph; import net.minecraft.client.resources.model.BakedModel; import net.minecraft.resources.ResourceLocation; import net.minecraft.world.item.Item; import net.minecraft.world.level.block.Block; import net.minecraft.world.level.block.state.BlockState; +import net.minecraftforge.fml.ModContainer; +import net.minecraftforge.fml.ModList; +import net.minecraftforge.forgespi.language.IModInfo; import net.minecraftforge.registries.ForgeRegistries; import org.embeddedt.modernfix.dynamicresources.ModelLocationCache; import org.jetbrains.annotations.Nullable; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; +/** + * Stores a list of all known default block/item models in the game, and provides a namespaced version + * of the model registry that emulates vanilla keySet behavior. + */ public class ModelBakeEventHelper { - public static Map wrapRegistry(Map modelRegistry) { - Set topLevelModelLocations = new HashSet<>(modelRegistry.keySet()); + private final Map modelRegistry; + private final Set topLevelModelLocations; + private final MutableGraph dependencyGraph; + public ModelBakeEventHelper(Map modelRegistry) { + this.modelRegistry = modelRegistry; + this.topLevelModelLocations = new HashSet<>(modelRegistry.keySet()); for(Block block : ForgeRegistries.BLOCKS) { for(BlockState state : block.getStateDefinition().getPossibleStates()) { topLevelModelLocations.add(ModelLocationCache.get(state)); @@ -25,6 +40,28 @@ public class ModelBakeEventHelper { for(Item item : ForgeRegistries.ITEMS) { topLevelModelLocations.add(ModelLocationCache.get(item)); } + this.dependencyGraph = GraphBuilder.undirected().build(); + ModList.get().forEachModContainer((id, mc) -> { + this.dependencyGraph.addNode(id); + }); + for(String id : this.dependencyGraph.nodes()) { + Optional mContainer = ModList.get().getModContainerById(id); + if(mContainer.isPresent()) { + for(IModInfo.ModVersion version : mContainer.get().getModInfo().getDependencies()) { + this.dependencyGraph.putEdge(id, version.getModId()); + } + } + } + } + + public Map wrapRegistry(String modId) { + final Set modIdsToInclude = new HashSet<>(); + modIdsToInclude.add(modId); + try { + modIdsToInclude.addAll(this.dependencyGraph.adjacentNodes(modId)); + } catch(IllegalArgumentException ignored) { /* sanity check */ } + modIdsToInclude.remove("minecraft"); + Set ourModelLocations = Sets.filter(this.topLevelModelLocations, loc -> modIdsToInclude.contains(loc.getNamespace())); return new ForwardingMap() { @Override protected Map delegate() { @@ -33,18 +70,12 @@ public class ModelBakeEventHelper { @Override public Set keySet() { - return topLevelModelLocations; + return ourModelLocations; } @Override public boolean containsKey(@Nullable Object key) { - return topLevelModelLocations.contains(key) || super.containsKey(key); - } - - @Override - public BakedModel put(ResourceLocation key, BakedModel value) { - topLevelModelLocations.add(key); - return super.put(key, value); + return ourModelLocations.contains(key) || super.containsKey(key); } }; } diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java index 1e130b7f..887a9e04 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java @@ -3,11 +3,18 @@ package org.embeddedt.modernfix.forge.mixin.perf.dynamic_resources; import net.minecraft.client.resources.model.BakedModel; import net.minecraft.resources.ResourceLocation; import net.minecraftforge.client.ForgeHooksClient; +import net.minecraftforge.client.event.ModelBakeEvent; +import net.minecraftforge.eventbus.api.Event; +import net.minecraftforge.fml.ModContainer; +import net.minecraftforge.fml.ModList; +import net.minecraftforge.fml.ModLoader; +import net.minecraftforge.fml.common.ObfuscationReflectionHelper; import org.embeddedt.modernfix.forge.dynresources.ModelBakeEventHelper; import org.spongepowered.asm.mixin.Mixin; import org.spongepowered.asm.mixin.injection.At; -import org.spongepowered.asm.mixin.injection.ModifyVariable; +import org.spongepowered.asm.mixin.injection.Redirect; +import java.lang.reflect.Method; import java.util.Map; @Mixin(ForgeHooksClient.class) @@ -15,8 +22,21 @@ public class ForgeHooksClientMixin { /** * Generate a more realistic keySet that contains every item and block model location, to help with mod compat. */ - @ModifyVariable(method = "onModelBake", at = @At("HEAD"), ordinal = 0, argsOnly = true) - private static Map generateModelKeySet(Map modelRegistry) { - return ModelBakeEventHelper.wrapRegistry(modelRegistry); + @Redirect(method = "onModelBake", at = @At(value = "INVOKE", target = "Lnet/minecraftforge/fml/ModLoader;postEvent(Lnet/minecraftforge/eventbus/api/Event;)V")) + private static void postNamespacedKeySetEvent(ModLoader loader, Event event) { + if(!ModLoader.isLoadingStateValid()) + return; + ModelBakeEvent bakeEvent = ((ModelBakeEvent)event); + ModelBakeEventHelper helper = new ModelBakeEventHelper(bakeEvent.getModelRegistry()); + Method acceptEv = ObfuscationReflectionHelper.findMethod(ModContainer.class, "acceptEvent", Event.class); + ModList.get().forEachModContainer((id, mc) -> { + Map newRegistry = helper.wrapRegistry(id); + ModelBakeEvent postedEvent = new ModelBakeEvent(bakeEvent.getModelManager(), newRegistry, bakeEvent.getModelLoader()); + try { + acceptEv.invoke(mc, postedEvent); + } catch(ReflectiveOperationException e) { + e.printStackTrace(); + } + }); } }