Fix bad conversion of block index using negative Y (#861)

src/main/java/net/minestom/server/utils/chunk/ChunkUtils.java

@@ -198,7 +198,12 @@ public final class ChunkUtils {
         z = z % Chunk.CHUNK_SIZE_Z;
 
         int index = x & 0xF; // 4 bits
-        index |= (y << 4) & 0x0FFFFFF0; // 24 bits
+        if(y > 0) {
+            index |= (y << 4) & 0x07FFFFF0; // 23 bits (24th bit is always 0 because y is positive)
+        } else {
+            index |= ((-y) << 4) & 0x7FFFFF0; // Make positive and use 23 bits
+            index |= 1 << 27; // Set negative sign at 24th bit
+        }
         index |= (z << 28) & 0xF0000000; // 4 bits
         return index;
     }
@@ -211,7 +216,7 @@ public final class ChunkUtils {
      */
     public static @NotNull Point getBlockPosition(int index, int chunkX, int chunkZ) {
         final int x = blockIndexToChunkPositionX(index) + Chunk.CHUNK_SIZE_X * chunkX;
-        final int y = index >>> 4 & 0xFF;
+        final int y = blockIndexToChunkPositionY(index);
         final int z = blockIndexToChunkPositionZ(index) + Chunk.CHUNK_SIZE_Z * chunkZ;
         return new Vec(x, y, z);
     }
@@ -233,7 +238,9 @@ public final class ChunkUtils {
      * @return the chunk position Y of the specified index
      */
     public static int blockIndexToChunkPositionY(int index) {
-        return (index >> 4) & 0x0FFFFFF; // 4-28 bits
+        int y = (index & 0x07FFFFF0) >>> 4;
+        if(((index >>> 27) & 1) == 1) y = -y; // Sign bit set, invert sign
+        return y; // 4-28 bits
     }
 
     /**

src/test/java/net/minestom/server/coordinate/CoordinateTest.java

@@ -1,11 +1,15 @@
 package net.minestom.server.coordinate;
 
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongSet;
+import net.minestom.server.instance.Chunk;
 import net.minestom.server.utils.chunk.ChunkUtils;
 import org.junit.jupiter.api.Test;
 
+import java.util.List;
+
 import static net.minestom.server.utils.chunk.ChunkUtils.*;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.*;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class CoordinateTest {
 
@@ -100,4 +104,68 @@ public class CoordinateTest {
         assertEquals(0, ChunkUtils.toSectionRelativeCoordinate(32));
         assertEquals(1, ChunkUtils.toSectionRelativeCoordinate(33));
     }
+
+    @Test
+    public void blockIndex() {
+        // Test if the block index is correctly converted back and forth
+
+        List<Vec> tempEquals = List.of(
+                // Zero vector with zero, positive and negative Y value
+                Vec.ZERO,
+                Vec.ZERO.withY(1),
+                Vec.ZERO.withY(-1),
+                // One vector with positive and negative Y value
+                Vec.ONE,
+                Vec.ONE.withY(-1),
+                // Vector with X/Z outside of chunk size
+                new Vec(Chunk.CHUNK_SIZE_X + 1, 20, Chunk.CHUNK_SIZE_Z + 1),
+                new Vec(Chunk.CHUNK_SIZE_X + 1, -20, Chunk.CHUNK_SIZE_Z + 1),
+                // Vector with negative X/Z block pos
+                new Vec(-1, 20, -1),
+                new Vec(-1, -20, -1),
+                // Check Y min and max value (23 bits, 2^23-1, -2^23+1)
+                new Vec(0, 8_388_607, 0),
+                new Vec(0, -8_388_607, 0)
+        );
+
+        for (Vec vec : tempEquals) {
+            assertEquals(getBlockPosition(getBlockIndex(vec.blockX(), vec.blockY(), vec.blockZ()),
+                    vec.chunkX(), vec.chunkZ()), vec);
+        }
+
+        // Test if the block index does convert to wrong values due to overflow
+
+        List<Vec> tempNotEquals = List.of(
+                // Above and below Y min and max value (> 2^23-1, < -2^23+1)
+                // Integer overflows into the 24th bit which is not copied into block index,
+                // so an error is expected here.
+                new Vec(0, 8_388_608, 0),
+                new Vec(0, -8_388_608, 0)
+        );
+
+        for (Vec vec : tempNotEquals) {
+            assertNotEquals(getBlockPosition(getBlockIndex(vec.blockX(), vec.blockY(), vec.blockZ()),
+                    vec.chunkX(), vec.chunkZ()), vec);
+        }
+    }
+
+    @Test
+    public void blockIndexDuplicate() {
+        LongSet temp = new LongOpenHashSet();
+
+        for (int x = 0; x < Chunk.CHUNK_SIZE_X; x++) {
+            for (int z = 0; z < Chunk.CHUNK_SIZE_Z; z++) {
+                for (int y = -64; y < 364; y++) {
+                    var vec = new Vec(x, y, z);
+                    var index = getBlockIndex(vec.blockX(), vec.blockY(), vec.blockZ());
+                    assertTrue(temp.add(index), "Duplicate block index found: " + index + " " + vec);
+                    assertEquals(getBlockPosition(index, vec.chunkX(), vec.chunkZ()), vec);
+
+                    assertEquals(blockIndexToChunkPositionX(index), x);
+                    assertEquals(blockIndexToChunkPositionY(index), y);
+                    assertEquals(blockIndexToChunkPositionZ(index), z);
+                }
+            }
+        }
+    }
 }