Additional thread saftey for CCL (#50)

addon.gradle

@@ -6,3 +6,10 @@ minecraft {
         }
     }
 }
+
+test {
+    useJUnitPlatform()
+    testLogging {
+        events "passed", "skipped", "failed"
+    }
+}

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')
 }

src/main/java/codechicken/lib/lighting/SimpleBrightnessModel.java

@@ -12,7 +12,12 @@ import codechicken.lib.vec.BlockCoord;
 public class SimpleBrightnessModel implements CCRenderState.IVertexOperation {
 
     public static final int operationIndex = CCRenderState.registerOperation();
-    public static SimpleBrightnessModel instance = new SimpleBrightnessModel();
+    private static final ThreadLocal<SimpleBrightnessModel> instances = ThreadLocal
+            .withInitial(SimpleBrightnessModel::new);
+
+    public static SimpleBrightnessModel instance() {
+        return instances.get();
+    }
 
     public IBlockAccess access;
     public BlockCoord pos = new BlockCoord();

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 class CCRenderState {
         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 @@ public class CCRenderState {
         int operationID();
     }
 
-    private static ArrayList<VertexAttribute<?>> vertexAttributes = new ArrayList<>();
+    public static final class AttributeKey<T> {
 
-    private static int registerVertexAttribute(VertexAttribute<?> attr) {
-        vertexAttributes.add(attr);
-        return vertexAttributes.size() - 1;
+        private static final HashMap<String, AttributeKey<?>> nameMap = new HashMap<>();
+        private static final ArrayList<AttributeKey<?>> attributeKeys = new ArrayList<>();
+        private static int nextLegacyId;
+
+        public final String name;
+        public final int attributeKeyIndex;
+        public final int operationIndex;
+        private VertexAttribute<T> 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 <T> AttributeKey<T> create(String name) {
+            if (nameMap.containsKey(name))
+                throw new IllegalArgumentException("Duplicate registration of attribute with name: " + name);
+            return new AttributeKey<>(name);
+        }
+
+        static synchronized <T> AttributeKey<T> createLegacy() {
+            return new AttributeKey<>("legacy#" + nextLegacyId++);
+        }
+
+        synchronized void setCanonical(VertexAttribute<T> attr) {
+            if (canonical == null) canonical = attr;
+        }
+
+        static synchronized AttributeKey<?> get(int index) {
+            return attributeKeys.get(index);
+        }
     }
 
+    private static final AttributeKey<Vector3[]> NORMAL_KEY = AttributeKey.create("normal");
+    private static final AttributeKey<int[]> COLOUR_KEY = AttributeKey.create("colour");
+    private static final AttributeKey<int[]> LIGHTING_KEY = AttributeKey.create("lighting");
+    private static final AttributeKey<int[]> SIDE_KEY = AttributeKey.create("side");
+    private static final AttributeKey<LC[]> 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 class CCRenderState {
      */
     public abstract static class VertexAttribute<T> 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<T> 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 class CCRenderState {
         return instances.get().lightingAttrib;
     }
 
-    public VertexAttribute<Vector3[]> normalAttrib = new VertexAttribute<>() {
+    public VertexAttribute<Vector3[]> normalAttrib = new VertexAttribute<>(NORMAL_KEY) {
 
         private Vector3[] normalRef;
 
@@ -217,7 +267,7 @@ public class CCRenderState {
             else state.setNormalInstance(Rotation.axes[state.side]);
         }
     };
-    public VertexAttribute<int[]> colourAttrib = new VertexAttribute<>() {
+    public VertexAttribute<int[]> colourAttrib = new VertexAttribute<>(COLOUR_KEY) {
 
         private int[] colourRef;
 
@@ -239,7 +289,7 @@ public class CCRenderState {
             else state.setColourInstance(state.baseColour);
         }
     };
-    public VertexAttribute<int[]> lightingAttrib = new VertexAttribute<>() {
+    public VertexAttribute<int[]> lightingAttrib = new VertexAttribute<>(LIGHTING_KEY) {
 
         private int[] colourRef;
 
@@ -265,7 +315,7 @@ public class CCRenderState {
             state.setColourInstance(ColourRGBA.multiply(state.colour, colourRef[state.vertexIndex]));
         }
     };
-    public VertexAttribute<int[]> sideAttrib = new VertexAttribute<>() {
+    public VertexAttribute<int[]> sideAttrib = new VertexAttribute<>(SIDE_KEY) {
 
         private int[] sideRef;
 
@@ -292,7 +342,7 @@ public class CCRenderState {
     /**
      * Uses the position of the lightmatrix to compute LC if not provided
      */
-    public VertexAttribute<LC[]> lightCoordAttrib = new VertexAttribute<>() {
+    public VertexAttribute<LC[]> lightCoordAttrib = new VertexAttribute<>(LC_KEY) {
 
         private LC[] lcRef;
         private final Vector3 vec = new Vector3(); // for computation

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<Future<int[]>> futures = new ArrayList<>();
+            for (int i = 0; i < 8; i++) futures.add(pool.submit(CCRenderStateThreadingTest::attribIndices));
+            for (Future<int[]> 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<Future<int[]>> 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<int[]> 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));
+    }
+}