From a46893ac3c7c580b5e172c4edea8a939a49bfb3b Mon Sep 17 00:00:00 2001 From: Jason Mitchell Date: Tue, 21 Jul 2026 20:56:48 -0700 Subject: [PATCH] Additional thread saftey for CCL From discord report in beta-testing - https://discord.com/channels/181078474394566657/522098956491030558/1529326945764905091 & https://discord.com/channels/181078474394566657/522098956491030558/1529309902294745212 --- addon.gradle | 7 ++ dependencies.gradle | 3 + .../lib/lighting/SimpleBrightnessModel.java | 7 +- .../codechicken/lib/render/CCRenderState.java | 78 +++++++++++--- .../render/CCRenderStateThreadingTest.java | 102 ++++++++++++++++++ 5 files changed, 182 insertions(+), 15 deletions(-) create mode 100644 src/test/java/codechicken/lib/render/CCRenderStateThreadingTest.java diff --git a/addon.gradle b/addon.gradle index 8d81619..b79e4c0 100644 --- a/addon.gradle +++ b/addon.gradle @@ -6,3 +6,10 @@ minecraft { } } } + +test { + useJUnitPlatform() + testLogging { + events "passed", "skipped", "failed" + } +} diff --git a/dependencies.gradle b/dependencies.gradle index 2767713..e96d9a0 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -1,4 +1,7 @@ // Add your dependencies here dependencies { + testImplementation(platform('org.junit:junit-bom:5.14.1')) + testImplementation('org.junit.jupiter:junit-jupiter') + testRuntimeOnly('org.junit.platform:junit-platform-launcher') } diff --git a/src/main/java/codechicken/lib/lighting/SimpleBrightnessModel.java b/src/main/java/codechicken/lib/lighting/SimpleBrightnessModel.java index 7c5d15d..a69132d 100644 --- a/src/main/java/codechicken/lib/lighting/SimpleBrightnessModel.java +++ b/src/main/java/codechicken/lib/lighting/SimpleBrightnessModel.java @@ -12,7 +12,12 @@ public class SimpleBrightnessModel implements CCRenderState.IVertexOperation { public static final int operationIndex = CCRenderState.registerOperation(); - public static SimpleBrightnessModel instance = new SimpleBrightnessModel(); + private static final ThreadLocal instances = ThreadLocal + .withInitial(SimpleBrightnessModel::new); + + public static SimpleBrightnessModel instance() { + return instances.get(); + } public IBlockAccess access; public BlockCoord pos = new BlockCoord(); diff --git a/src/main/java/codechicken/lib/render/CCRenderState.java b/src/main/java/codechicken/lib/render/CCRenderState.java index 1eb74f1..e4adf41 100644 --- a/src/main/java/codechicken/lib/render/CCRenderState.java +++ b/src/main/java/codechicken/lib/render/CCRenderState.java @@ -1,6 +1,7 @@ package codechicken.lib.render; import java.util.ArrayList; +import java.util.HashMap; import net.minecraft.client.Minecraft; import net.minecraft.client.renderer.OpenGlHelper; @@ -38,9 +39,9 @@ public static CCRenderState instance() { return instances.get(); } - private static int nextOperationIndex; + private static volatile int nextOperationIndex; - public static int registerOperation() { + public static synchronized int registerOperation() { return nextOperationIndex++; } @@ -87,15 +88,52 @@ default void operate(CCRenderState state) { int operationID(); } - private static ArrayList> vertexAttributes = new ArrayList<>(); + public static final class AttributeKey { - private static int registerVertexAttribute(VertexAttribute attr) { - vertexAttributes.add(attr); - return vertexAttributes.size() - 1; + private static final HashMap> nameMap = new HashMap<>(); + private static final ArrayList> attributeKeys = new ArrayList<>(); + private static int nextLegacyId; + + public final String name; + public final int attributeKeyIndex; + public final int operationIndex; + private VertexAttribute canonical; + + private AttributeKey(String name) { + this.name = name; + this.attributeKeyIndex = attributeKeys.size(); + this.operationIndex = registerOperation(); + attributeKeys.add(this); + nameMap.put(name, this); + } + + public static synchronized AttributeKey create(String name) { + if (nameMap.containsKey(name)) + throw new IllegalArgumentException("Duplicate registration of attribute with name: " + name); + return new AttributeKey<>(name); + } + + static synchronized AttributeKey createLegacy() { + return new AttributeKey<>("legacy#" + nextLegacyId++); + } + + synchronized void setCanonical(VertexAttribute attr) { + if (canonical == null) canonical = attr; + } + + static synchronized AttributeKey get(int index) { + return attributeKeys.get(index); + } } + private static final AttributeKey NORMAL_KEY = AttributeKey.create("normal"); + private static final AttributeKey COLOUR_KEY = AttributeKey.create("colour"); + private static final AttributeKey LIGHTING_KEY = AttributeKey.create("lighting"); + private static final AttributeKey SIDE_KEY = AttributeKey.create("side"); + private static final AttributeKey LC_KEY = AttributeKey.create("lc"); + public static VertexAttribute getAttribute(int index) { - return vertexAttributes.get(index); + return AttributeKey.get(index).canonical; } /** @@ -106,14 +144,26 @@ public static VertexAttribute getAttribute(int index) { */ public abstract static class VertexAttribute implements IVertexOperation { - public final int attributeIndex = registerVertexAttribute(this); - private final int operationIndex = registerOperation(); + public final int attributeIndex; + private final int operationIndex; /** * Set to true when the attrute is part of the pipeline. Should only be managed by CCRenderState when * constructing the pipeline */ public boolean active = false; + protected VertexAttribute(AttributeKey key) { + attributeIndex = key.attributeKeyIndex; + operationIndex = key.operationIndex; + key.setCanonical(this); + } + + /** @deprecated keyless attributes get a per-construction index that is threadsafe */ + @Deprecated + protected VertexAttribute() { + this(AttributeKey.createLegacy()); + } + /** * Construct a new array for storage of vertex attrutes in a model */ @@ -189,7 +239,7 @@ public static VertexAttribute lightingAttrib() { return instances.get().lightingAttrib; } - public VertexAttribute normalAttrib = new VertexAttribute<>() { + public VertexAttribute normalAttrib = new VertexAttribute<>(NORMAL_KEY) { private Vector3[] normalRef; @@ -217,7 +267,7 @@ public void operate(CCRenderState state) { else state.setNormalInstance(Rotation.axes[state.side]); } }; - public VertexAttribute colourAttrib = new VertexAttribute<>() { + public VertexAttribute colourAttrib = new VertexAttribute<>(COLOUR_KEY) { private int[] colourRef; @@ -239,7 +289,7 @@ public void operate(CCRenderState state) { else state.setColourInstance(state.baseColour); } }; - public VertexAttribute lightingAttrib = new VertexAttribute<>() { + public VertexAttribute lightingAttrib = new VertexAttribute<>(LIGHTING_KEY) { private int[] colourRef; @@ -265,7 +315,7 @@ public void operate(CCRenderState state) { state.setColourInstance(ColourRGBA.multiply(state.colour, colourRef[state.vertexIndex])); } }; - public VertexAttribute sideAttrib = new VertexAttribute<>() { + public VertexAttribute sideAttrib = new VertexAttribute<>(SIDE_KEY) { private int[] sideRef; @@ -292,7 +342,7 @@ public void operate(CCRenderState state) { /** * Uses the position of the lightmatrix to compute LC if not provided */ - public VertexAttribute lightCoordAttrib = new VertexAttribute<>() { + public VertexAttribute lightCoordAttrib = new VertexAttribute<>(LC_KEY) { private LC[] lcRef; private final Vector3 vec = new Vector3(); // for computation diff --git a/src/test/java/codechicken/lib/render/CCRenderStateThreadingTest.java b/src/test/java/codechicken/lib/render/CCRenderStateThreadingTest.java new file mode 100644 index 0000000..6962fcc --- /dev/null +++ b/src/test/java/codechicken/lib/render/CCRenderStateThreadingTest.java @@ -0,0 +1,102 @@ +package codechicken.lib.render; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +import org.junit.jupiter.api.Test; + +import codechicken.lib.vec.Vector3; + +public class CCRenderStateThreadingTest { + + private static int[] attribIndices() { + return new int[] { CCRenderState.normalAttrib().attributeIndex, CCRenderState.colourAttrib().attributeIndex, + CCRenderState.lightingAttrib().attributeIndex, CCRenderState.sideAttrib().attributeIndex, + CCRenderState.lightCoordAttrib().attributeIndex }; + } + + @Test + public void attributeIndicesStableAcrossThreads() throws Exception { + int[] main = attribIndices(); + assertEquals(5, Arrays.stream(main).distinct().count()); + + ExecutorService pool = Executors.newFixedThreadPool(8); + try { + List> futures = new ArrayList<>(); + for (int i = 0; i < 8; i++) futures.add(pool.submit(CCRenderStateThreadingTest::attribIndices)); + for (Future f : futures) assertArrayEquals(main, f.get(10, TimeUnit.SECONDS)); + } finally { + pool.shutdownNow(); + } + } + + @Test + public void modelBakedOnOneThreadIsReadableOnAnother() throws Exception { + ExecutorService bakeThread = Executors.newSingleThreadExecutor(); + ExecutorService renderThread = Executors.newSingleThreadExecutor(); + try { + CCModel model = bakeThread.submit(() -> { + CCModel m = CCModel.quadModel(4); + m.verts[0] = new Vertex5(0, 1, 0, 0, 0); + m.verts[1] = new Vertex5(0, 1, 1, 0, 1); + m.verts[2] = new Vertex5(1, 1, 1, 1, 1); + m.verts[3] = new Vertex5(1, 1, 0, 1, 0); + return m.computeNormals(); + }).get(10, TimeUnit.SECONDS); + + int side = renderThread.submit(() -> { + assertTrue( + model.hasAttribute(CCRenderState.normalAttrib()), + "normals baked on another thread not visible under this thread's attributeIndex"); + Vector3[] normals = model.getAttributes(CCRenderState.normalAttrib()); + assertNotNull(normals); + return CCModel.findSide(normals[0]); + }).get(10, TimeUnit.SECONDS); + + assertEquals(1, side); + assertEquals(1, CCModel.findSide(model.getAttributes(CCRenderState.normalAttrib())[0])); + } finally { + bakeThread.shutdownNow(); + renderThread.shutdownNow(); + } + } + + @Test + public void concurrentFirstUseKeepsIndicesConsistent() throws Exception { + int threads = 16; + CyclicBarrier barrier = new CyclicBarrier(threads); + ExecutorService pool = Executors.newFixedThreadPool(threads); + try { + List> futures = new ArrayList<>(); + for (int i = 0; i < threads; i++) futures.add(pool.submit(() -> { + barrier.await(10, TimeUnit.SECONDS); + return attribIndices(); + })); + int[] expected = attribIndices(); + for (Future f : futures) assertArrayEquals(expected, f.get(10, TimeUnit.SECONDS)); + } finally { + pool.shutdownNow(); + } + } + + @Test + public void getAttributeReturnsCanonicalForIndex() { + int normalIndex = CCRenderState.normalAttrib().attributeIndex; + CCRenderState.VertexAttribute canonical = CCRenderState.getAttribute(normalIndex); + assertNotNull(canonical); + assertEquals(normalIndex, canonical.attributeIndex); + assertSame(canonical, CCRenderState.getAttribute(normalIndex)); + } +}