Fix StellarSky crash and corrupted player model (#2315)

glsm/src/main/java/com/gtnewhorizons/angelica/glsm/DisplayListIDAllocator.java

@@ -1,7 +1,7 @@
 package com.gtnewhorizons.angelica.glsm;
 
 /*
- * Ported from Mesa's util_idalloc (src/util/u_idalloc.c).
+ * Ported from Mesa's util_idalloc and util_idalloc_sparse (src/util/u_idalloc.c).
  *
  * Original copyright and license:
  *
@@ -32,17 +32,15 @@ package com.gtnewhorizons.angelica.glsm;
  */
 
 /**
- * Bitmap ID allocator for display list IDs.
- *
- * <p>Bitmap-based: each bit in the {@code data} array represents one ID.
- * Bit set = allocated, bit clear = free. ID 0 is reserved (never returned).
+ * Sparse bitmap ID allocator for display list IDs; ID 0 is reserved (never returned).
  */
 public class DisplayListIDAllocator {
 
-    private int[] data;
-    private int numElements;       // number of uint32 words allocated
-    private int numSetElements;    // highest word index with any bit set + 1
-    private int lowestFreeIdx;     // optimization: skip fully-allocated words
+    private static final int SEGMENT_SHIFT = 22;
+    private static final int SEGMENT_MASK = (1 << SEGMENT_SHIFT) - 1;
+    private static final int WORDS_PER_SEGMENT = (1 << SEGMENT_SHIFT) / 32;
+
+    private final Segment[] segments = new Segment[512];
 
     public DisplayListIDAllocator() {
         this(32); // 1024 IDs initially
@@ -50,138 +48,191 @@ public class DisplayListIDAllocator {
 
     public DisplayListIDAllocator(int initialWords) {
         if (initialWords < 1) initialWords = 1;
-        this.data = new int[initialWords];
-        this.numElements = initialWords;
-        this.numSetElements = 0;
-        this.lowestFreeIdx = 0;
+        final Segment first = new Segment(initialWords);
         // Reserve ID 0 (never returned), matching Mesa's skip_zero pattern
-        data[0] = 1;
-        numSetElements = 1;
+        first.data[0] = 1;
+        first.numSetElements = 1;
+        segments[0] = first;
+    }
+
+    private Segment segment(int index) {
+        Segment s = segments[index];
+        if (s == null) {
+            s = new Segment(1);
+            segments[index] = s;
+        }
+        return s;
     }
 
     public synchronized int allocRange(int count) {
         if (count <= 0) throw new IllegalArgumentException("count must be > 0");
-        if (count == 1) return allocSingle();
-        return allocMultiple(count);
-    }
-
-    private int allocSingle() {
-        for (int i = lowestFreeIdx; i < numElements; i++) {
-            if (data[i] == 0xFFFFFFFF) continue;
-
-            int bit = Integer.numberOfTrailingZeros(~data[i]);
-            data[i] |= 1 << bit;
-            lowestFreeIdx = i;
-            numSetElements = Math.max(numSetElements, i + 1);
-            return i * 32 + bit;
-        }
-
-        // No slots available, grow. Record old end, then mark first bit there.
-        int oldNumElements = numElements;
-        resize(Math.max(numElements, 1) * 2);
-        lowestFreeIdx = oldNumElements;
-        data[oldNumElements] |= 1;
-        numSetElements = Math.max(numSetElements, oldNumElements + 1);
-        return oldNumElements * 32;
-    }
-
-    private int allocMultiple(int num) {
-        int numWords = divRoundUp(num, 32);
-        int base = findFreeBlock(lowestFreeIdx);
-        boolean found = false;
-
-        while (!found) {
-            int i;
-            for (i = base; i < numElements && i - base < numWords && data[i] == 0; i++)
-                ;
-
-            if (i - base == numWords) {
-                found = true;
-            } else if (i == numElements) {
-                // Hit end — base is valid: data[base..end] are zero, resize extends the run into newly zeroed words.
-                resize(numElements * 2 + numWords);
-                found = true;
-            } else {
-                // Non-zero word at i, skip past it
-                base = i + 1;
+        if (count == 1) {
+            for (int i = 0; i < segments.length; i++) {
+                final Segment s = segment(i);
+                if (s.lowestFreeIdx < WORDS_PER_SEGMENT) return (i << SEGMENT_SHIFT) | s.allocSingle();
             }
+            throw new IllegalStateException("all display list IDs are in use");
         }
-
-        // Mark bits as used
-        int fullWords = num / 32;
-        int remainder = num % 32;
-        for (int i = base; i < base + fullWords; i++) {
-            data[i] = 0xFFFFFFFF;
+        final int numWords = Segment.divRoundUp(count, 32);
+        for (int i = 0; i < segments.length; i++) {
+            final Segment s = segment(i);
+            if (s.lowestFreeIdx + numWords > WORDS_PER_SEGMENT) continue;
+            final int base = s.allocMultiple(count);
+            if (base + count <= 1 << SEGMENT_SHIFT) return (i << SEGMENT_SHIFT) | base;
+            for (int j = 0; j < count; j++) s.free(base + j);
         }
-        if (remainder != 0) {
-            data[base + fullWords] |= (1 << remainder) - 1;
-        }
-
-        if (lowestFreeIdx == base) {
-            lowestFreeIdx = base + num / 32;
-        }
-        numSetElements = Math.max(numSetElements, base + numWords);
-
-        return base * 32;
+        throw new IllegalStateException("no free consecutive range of " + count + " display list IDs");
     }
 
-    private int findFreeBlock(int start) {
-        for (int i = start; i < numElements; i++) {
-            if (data[i] == 0) return i;
-        }
-        return numElements;
+    public synchronized void reserve(int id) {
+        if (id <= 0) return;
+        segment(id >>> SEGMENT_SHIFT).reserve(id & SEGMENT_MASK);
     }
 
     public synchronized void free(int id) {
         if (id <= 0) return; // ID 0 is reserved
-        int idx = id / 32;
-        if (idx >= numElements) return;
-
-        lowestFreeIdx = Math.min(idx, lowestFreeIdx);
-        data[idx] &= ~(1 << (id % 32));
-
-        if (numSetElements == idx + 1) {
-            while (numSetElements > 0 && data[numSetElements - 1] == 0) {
-                numSetElements--;
-            }
-        }
+        final Segment s = segments[id >>> SEGMENT_SHIFT];
+        if (s != null) s.free(id & SEGMENT_MASK);
     }
 
     public synchronized void freeRange(int base, int count) {
         if (count <= 0 || base <= 0) return;
 
-        int startIdx = base / 32;
-        if (startIdx >= numElements) return;
-
-        int end = base + count;
-        for (int id = base; id < end; id++) {
-            int idx = id / 32;
-            if (idx >= numElements) break;
-            data[idx] &= ~(1 << (id % 32));
-        }
-
-        lowestFreeIdx = Math.min(startIdx, lowestFreeIdx);
-        while (numSetElements > 0 && data[numSetElements - 1] == 0) {
-            numSetElements--;
+        final long last = Math.min((long) base + count - 1, Integer.MAX_VALUE);
+        long id = base;
+        while (id <= last) {
+            final int index = (int) (id >>> SEGMENT_SHIFT);
+            final Segment s = segments[index];
+            if (s == null || (id & SEGMENT_MASK) >= (long) s.numElements * 32) {
+                id = (long) (index + 1) << SEGMENT_SHIFT;
+                continue;
+            }
+            s.free((int) id & SEGMENT_MASK);
+            id++;
         }
     }
 
     public synchronized boolean isAllocated(int id) {
         if (id <= 0) return false;
-        int idx = id / 32;
-        if (idx >= numSetElements) return false;
-        return (data[idx] & (1 << (id % 32))) != 0;
+        final Segment s = segments[id >>> SEGMENT_SHIFT];
+        return s != null && s.isAllocated(id & SEGMENT_MASK);
     }
 
-    private void resize(int newNumElements) {
-        if (newNumElements <= numElements) return;
-        int[] newData = new int[newNumElements];
-        System.arraycopy(data, 0, newData, 0, numElements);
-        data = newData;
-        numElements = newNumElements;
-    }
+    private static final class Segment {
+        private int[] data;
+        private int numElements;       // number of uint32 words allocated
+        private int numSetElements;    // highest word index with any bit set + 1
+        private int lowestFreeIdx;     // optimization: skip fully-allocated words
 
-    private static int divRoundUp(int a, int b) {
-        return (a + b - 1) / b;
+        Segment(int initialWords) {
+            this.data = new int[initialWords];
+            this.numElements = initialWords;
+        }
+
+        int allocSingle() {
+            for (int i = lowestFreeIdx; i < numElements; i++) {
+                if (data[i] == 0xFFFFFFFF) continue;
+
+                int bit = Integer.numberOfTrailingZeros(~data[i]);
+                data[i] |= 1 << bit;
+                lowestFreeIdx = i;
+                numSetElements = Math.max(numSetElements, i + 1);
+                return i * 32 + bit;
+            }
+
+            // No slots available, grow. Record old end, then mark first bit there.
+            int oldNumElements = numElements;
+            resize(Math.max(numElements, 1) * 2);
+            lowestFreeIdx = oldNumElements;
+            data[oldNumElements] |= 1;
+            numSetElements = Math.max(numSetElements, oldNumElements + 1);
+            return oldNumElements * 32;
+        }
+
+        int allocMultiple(int num) {
+            int numWords = divRoundUp(num, 32);
+            int base = findFreeBlock(lowestFreeIdx);
+            boolean found = false;
+
+            while (!found) {
+                int i;
+                for (i = base; i < numElements && i - base < numWords && data[i] == 0; i++)
+                    ;
+
+                if (i - base == numWords) {
+                    found = true;
+                } else if (i == numElements) {
+                    // Hit end — base is valid: data[base..end] are zero, resize extends the run into newly zeroed words.
+                    resize(numElements * 2 + numWords);
+                    found = true;
+                } else {
+                    // Non-zero word at i, skip past it
+                    base = i + 1;
+                }
+            }
+
+            // Mark bits as used
+            int fullWords = num / 32;
+            int remainder = num % 32;
+            for (int i = base; i < base + fullWords; i++) {
+                data[i] = 0xFFFFFFFF;
+            }
+            if (remainder != 0) {
+                data[base + fullWords] |= (1 << remainder) - 1;
+            }
+
+            if (lowestFreeIdx == base) {
+                lowestFreeIdx = base + num / 32;
+            }
+            numSetElements = Math.max(numSetElements, base + numWords);
+
+            return base * 32;
+        }
+
+        int findFreeBlock(int start) {
+            for (int i = start; i < numElements; i++) {
+                if (data[i] == 0) return i;
+            }
+            return numElements;
+        }
+
+        void free(int id) {
+            int idx = id / 32;
+            if (idx >= numElements) return;
+
+            lowestFreeIdx = Math.min(idx, lowestFreeIdx);
+            data[idx] &= ~(1 << (id % 32));
+
+            if (numSetElements == idx + 1) {
+                while (numSetElements > 0 && data[numSetElements - 1] == 0) {
+                    numSetElements--;
+                }
+            }
+        }
+
+        boolean isAllocated(int id) {
+            int idx = id / 32;
+            if (idx >= numSetElements) return false;
+            return (data[idx] & (1 << (id % 32))) != 0;
+        }
+
+        void reserve(int id) {
+            final int idx = id / 32;
+            if (idx >= numElements) resize((idx + 1) * 2);
+            data[idx] |= 1 << (id % 32);
+            numSetElements = Math.max(numSetElements, idx + 1);
+        }
+
+        private void resize(int newNumElements) {
+            if (newNumElements <= numElements) return;
+            int[] newData = new int[newNumElements];
+            System.arraycopy(data, 0, newData, 0, numElements);
+            data = newData;
+            numElements = newNumElements;
+        }
+
+        private static int divRoundUp(int a, int b) {
+            return (a + b - 1) / b;
+        }
     }
 }

glsm/src/main/java/com/gtnewhorizons/angelica/glsm/DisplayListManager.java

@@ -1046,9 +1046,12 @@ public class DisplayListManager {
         }
         final boolean locked = GLStateManager.acquireDrawLock();
         try {
-            final CompiledDisplayList previous = displayListCache.put(glListId, compiled);
-            if (previous != null && previous != CompiledDisplayList.EMPTY && previous != compiled) {
-                previous.delete();
+            synchronized (GLSMDraws.displayListIdAllocator) {
+                final CompiledDisplayList previous = displayListCache.put(glListId, compiled);
+                if (previous != null && previous != CompiledDisplayList.EMPTY && previous != compiled) {
+                    previous.delete();
+                }
+                GLSMDraws.displayListIdAllocator.reserve(glListId);
             }
         } finally {
             if (locked) GLStateManager.releaseDrawLock();

glsm/src/main/java/com/gtnewhorizons/angelica/glsm/GLSMDraws.java

@@ -481,7 +481,7 @@ public final class GLSMDraws {
         DisplayListManager.glEndList();
     }
 
-    private static final DisplayListIDAllocator displayListIdAllocator = new DisplayListIDAllocator();
+    static final DisplayListIDAllocator displayListIdAllocator = new DisplayListIDAllocator();
 
     public static int glGenLists(int range) {
         rejectExitCapture();

glsm/src/main/java/com/gtnewhorizons/angelica/glsm/recording/DisplayListVBO.java

@@ -3,6 +3,7 @@ package com.gtnewhorizons.angelica.glsm.recording;
 import com.gtnewhorizon.gtnhlib.client.renderer.vao.IVertexArrayObject;
 import com.gtnewhorizons.angelica.glsm.GLSMBuffers;
 import com.gtnewhorizons.angelica.glsm.GLSMDraws;
+import com.gtnewhorizons.angelica.glsm.GLSMVertexInput;
 import com.gtnewhorizons.angelica.glsm.hooks.ImmediateExtendedAttribHandler;
 import com.gtnewhorizons.angelica.glsm.streaming.LineQuads;
 import org.lwjgl.opengl.GL11;
@@ -99,26 +100,31 @@ public final class DisplayListVBO {
 
         public void render() {
             if (vao == null) return;
-            if (lineQuads != null && GLSMDraws.ffpWidensLineQuads()) {
-                renderLineQuads();
-                return;
-            }
-            vao.bind();
-            if (pendingExtVbo != 0) {
-                GLSMBuffers.glBindBuffer(GL15.GL_ARRAY_BUFFER, pendingExtVbo);
-                ImmediateExtendedAttribHandler.setupExtAttribPointers(0L, ImmediateExtendedAttribHandler.EXT_STRIDE);
-                GLSMBuffers.glBindBuffer(GL15.GL_ARRAY_BUFFER, 0);
-                pendingExtVbo = 0;
-            }
-            if (count <= 0) { vao.unbind(); return; }
-            VAOManager.setCurrentVertexFlags(vertexFlags);
-            FfpExtendedAttribs.beginInternalDraw();
+            final int prevVAO = GLSMVertexInput.getBoundVAO();
             try {
-                vao.draw(drawMode, start, count);
+                if (lineQuads != null && GLSMDraws.ffpWidensLineQuads()) {
+                    renderLineQuads();
+                    return;
+                }
+                vao.bind();
+                if (pendingExtVbo != 0) {
+                    GLSMBuffers.glBindBuffer(GL15.GL_ARRAY_BUFFER, pendingExtVbo);
+                    ImmediateExtendedAttribHandler.setupExtAttribPointers(0L, ImmediateExtendedAttribHandler.EXT_STRIDE);
+                    GLSMBuffers.glBindBuffer(GL15.GL_ARRAY_BUFFER, 0);
+                    pendingExtVbo = 0;
+                }
+                if (count <= 0) { vao.unbind(); return; }
+                VAOManager.setCurrentVertexFlags(vertexFlags);
+                FfpExtendedAttribs.beginInternalDraw();
+                try {
+                    vao.draw(drawMode, start, count);
+                } finally {
+                    FfpExtendedAttribs.endInternalDraw();
+                }
+                vao.unbind();
             } finally {
-                FfpExtendedAttribs.endInternalDraw();
+                GLSMVertexInput.glBindVertexArray(prevVAO);
             }
-            vao.unbind();
         }
 
         private void renderLineQuads() {

glsm/src/main/java/com/gtnewhorizons/angelica/glsm/recording/commands/BatchedIndexedDrawCmd.java

@@ -1,6 +1,7 @@
 package com.gtnewhorizons.angelica.glsm.recording.commands;
 
 import com.gtnewhorizons.angelica.glsm.GLSMDraws;
+import com.gtnewhorizons.angelica.glsm.GLSMVertexInput;
 
 public final class BatchedIndexedDrawCmd implements DisplayListCommand {
     private IndexedDrawBatch batch;
@@ -25,8 +26,13 @@ public final class BatchedIndexedDrawCmd implements DisplayListCommand {
     @Override
     public void execute() {
         if (batch == null) return;
-        batch.bind();
-        GLSMDraws.glDrawElements(drawMode, indexCount, indexType, indexByteOffset);
+        final int prevVAO = GLSMVertexInput.getBoundVAO();
+        try {
+            batch.bind();
+            GLSMDraws.glDrawElements(drawMode, indexCount, indexType, indexByteOffset);
+        } finally {
+            GLSMVertexInput.glBindVertexArray(prevVAO);
+        }
     }
 
     @Override

glsm/src/test/java/com/gtnewhorizons/angelica/glsm/DisplayListIdAllocatorTest.java

@@ -241,4 +241,28 @@ class DisplayListIdAllocatorTest {
         int base2 = alloc.allocRange(64);
         assertEquals(base1, base2, "Freed range should be reused");
     }
+
+    @Test
+    void reserveRoutesByIdWithoutDisturbingLowIds() {
+        final DisplayListIDAllocator alloc = new DisplayListIDAllocator();
+        alloc.reserve(-5);
+        alloc.reserve(0);
+        alloc.reserve(Integer.MAX_VALUE);
+        assertTrue(alloc.isAllocated(Integer.MAX_VALUE));
+        assertFalse(alloc.isAllocated(Integer.MAX_VALUE & ((1 << 22) - 1)));
+        assertEquals(1, alloc.allocRange(1));
+        alloc.free(Integer.MAX_VALUE);
+        assertFalse(alloc.isAllocated(Integer.MAX_VALUE));
+    }
+
+    @Test
+    void rangeAllocationDoesNotSpanSegments() {
+        final DisplayListIDAllocator alloc = new DisplayListIDAllocator();
+        assertEquals(32, alloc.allocRange((1 << 22) - 64));
+        assertEquals(1 << 22, alloc.allocRange(64));
+        assertFalse(alloc.isAllocated((1 << 22) - 32));
+        assertTrue(alloc.isAllocated(1 << 22));
+        alloc.freeRange((1 << 22) - 1, 2);
+        assertFalse(alloc.isAllocated(1 << 22));
+    }
 }

glsm/src/test/java/com/gtnewhorizons/angelica/glsm/GLSM_ClientArrays_GLTest.java

@@ -531,6 +531,12 @@ public class GLSM_ClientArrays_GLTest {
             GLSMDraws.glNewList(list, GL11.GL_COMPILE);
             GLSMAttribStack.glPushClientAttrib(GL11.GL_CLIENT_VERTEX_ARRAY_BIT);
             GLSMAttribStack.glPopClientAttrib();
+            GLSMDraws.glBegin(GL11.GL_QUADS);
+            GLSMDraws.glVertex2f(-1f, -1f);
+            GLSMDraws.glVertex2f(1f, -1f);
+            GLSMDraws.glVertex2f(1f, 1f);
+            GLSMDraws.glVertex2f(-1f, 1f);
+            GLSMDraws.glEnd();
             GLSMDraws.glEndList();
 
             GLSMVertexInput.glBindVertexArray(other);

glsm/src/test/java/com/gtnewhorizons/angelica/glsm/GLSM_DisplayList_Stress_Test.java

@@ -14,6 +14,7 @@ import static com.gtnewhorizons.angelica.glsm.DisplayListTestHelper.assertModelv
 import static com.gtnewhorizons.angelica.glsm.DisplayListTestHelper.modelviewAfterCall;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -209,6 +210,25 @@ class GLSM_DisplayList_Stress_Test {
         assertEquals(10.0f, modelviewAfterCall(list).get(12), 0.0001f, "Recompilation should translate by 10");
     }
 
+    @Test
+    void compiledNameOutsideGeneratedRangeIsNotReissued() {
+        final int base = GLSMDraws.glGenLists(1);
+        int extra = base + 1;
+        while (GLSMDraws.glIsList(extra)) extra++;
+        testLists.add(base);
+        testLists.add(extra);
+
+        GLSMDraws.glNewList(extra, GL11.GL_COMPILE);
+        GLSMMatrices.glTranslatef(5.0f, 0.0f, 0.0f);
+        GLSMDraws.glEndList();
+        final int next = GLSMDraws.glGenLists(1);
+        testLists.add(next);
+        assertNotEquals(extra, next, "glGenLists must not reissue a name installed by glEndList");
+
+        GLSMDraws.glDeleteLists(extra, 1);
+        assertFalse(GLSMDraws.glIsList(extra), "glDeleteLists must release a reserved name");
+    }
+
     @Test
     void testComplexTransformSequence() {
         // Test a complex sequence of transforms that could expose numerical issues

glsm/src/test/java/com/gtnewhorizons/angelica/glsm/recording/IndexedDrawReplayTest.java

@@ -13,6 +13,7 @@ import com.gtnewhorizons.angelica.glsm.recording.support.DisplayListTestFixture;
 import org.junit.jupiter.api.Test;
 import org.lwjgl.opengl.GL11;
 import org.lwjgl.opengl.GL15;
+import org.lwjgl.opengl.GL30;
 
 import java.nio.ByteBuffer;
 import java.util.List;
@@ -112,7 +113,7 @@ class IndexedDrawReplayTest extends DisplayListTestFixture {
     }
 
     @Test
-    void execute_doesNotRestoreVAO_backToBackDraws() {
+    void execute_restoresRecordedVao_backToBackDraws() {
         final int vbo = defaultQuadVbo();
         final int ebo = uploadEbo(GL11.GL_UNSIGNED_SHORT, 0, 1, 2, 3);
         final int vao = setupPosColorVao(vbo);
@@ -137,7 +138,8 @@ class IndexedDrawReplayTest extends DisplayListTestFixture {
         assertSame(batchOf(cmds.getFirst()), batchOf(cmds.get(1)));
         final int sharedVAO = vaoOf(batchOf(cmds.getFirst()));
         assertNotEquals(0, sharedVAO);
-        assertEquals(sharedVAO, GLSMVertexInput.getBoundVAO());
+        assertEquals(vao, GLSMVertexInput.getBoundVAO());
+        assertEquals(vao, GL11.glGetInteger(GL30.GL_VERTEX_ARRAY_BINDING));
         GLSMVertexInput.glBindVertexArray(0);
     }