Fix blocks overflowing over the top of octree bounds (#1768)

* Fix blocks overflowing over the top of octree bounds

This was due to an incorrect maxDimension calculation

* Add helper method to get the previous multiple of 16

* Add input parameter assertions for Octree#setCube

* Fix off-by-one in setCube assertions

* Add clarifying comments to calculateOctreeOrigin

chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java

@@ -76,6 +76,8 @@
 import java.util.zip.GZIPInputStream;
 import java.util.zip.GZIPOutputStream;
 
+import static se.llbit.math.QuickMath.prevMul16;
+
 /**
  * Encapsulates scene and render state.
  *
@@ -1443,18 +1445,27 @@ private int calculateOctreeOrigin(Collection<ChunkPosition> chunksToLoad, boolea
     zmin *= 16;
     zmax *= 16;
 
-    int maxDimension = Math.max(yMax - yMin, Math.max(xmax - xmin, zmax - zmin));
-    int requiredDepth = QuickMath.log2(QuickMath.nextPow2(maxDimension));
+    int requiredDepth;
+    if(centerOctree) { // legacy codepath (for octree data version 5 and below).
+      int maxDimension = Math.max(yMax - yMin, Math.max(xmax - xmin, zmax - zmin));
+      requiredDepth = QuickMath.log2(QuickMath.nextPow2(maxDimension));
 
-    if(centerOctree) {
       int xroom = (1 << requiredDepth) - (xmax - xmin);
       int yroom = (1 << requiredDepth) - (yMax - yMin);
       int zroom = (1 << requiredDepth) - (zmax - zmin);
 
       origin.set(xmin - xroom / 2, -yroom / 2, zmin - zroom / 2);
     } else {
-      // Note: Math.floorDiv rather than integer division for round toward -infinity
-      origin.set(xmin, Math.floorDiv(yMin, 16) * 16, zmin);
+      // The yMin of the octree is being rounded down to the nearest multiple of 16 in an effort to increase the
+      // performance of Octree#setCube by keeping octree nodes aligned to ChunkSections within a chunk.
+      // This does result in slightly larger octrees (up to 15 blocks taller) than required in scenes where the
+      // loaded area is taller than it is wide.
+      int yMin16 = prevMul16(yMin);
+
+      int maxDimension = Math.max(yMax - yMin16, Math.max(xmax - xmin, zmax - zmin));
+      requiredDepth = QuickMath.log2(QuickMath.nextPow2(maxDimension));
+
+      origin.set(xmin, yMin16, zmin);
     }
     return requiredDepth;
   }

chunky/src/java/se/llbit/math/Octree.java

@@ -770,6 +770,16 @@ public void endFinalization() {
   }
 
   public void setCube(int cubeDepth, int[] types, int x, int y, int z) {
+    assert x >= 0 && y >= 0 && z >= 0 : "setCube position must not be negative (" + x + "," + y + "," + z + ")";
+
+    int cubeSize = 1 << cubeDepth;
+    int octreeSize = 1 << implementation.getDepth();
+    assert x + cubeSize <= octreeSize : "setCube x (" + x + "," + (x + cubeSize) + ") out of bounds for octree (0," + (octreeSize - 1) + ")";
+    assert y + cubeSize <= octreeSize : "setCube y (" + y + "," + (y + cubeSize) + ") out of bounds for octree (0," + (octreeSize - 1) + ")";
+    assert z + cubeSize <= octreeSize : "setCube z (" + z + "," + (z + cubeSize) + ") out of bounds for octree (0," + (octreeSize - 1) + ")";
+    int blocksInCube = cubeSize * cubeSize * cubeSize;
+    assert types.length == blocksInCube : "setCube types has length " + types.length + " expected " + blocksInCube;
+
     implementation.setCube(cubeDepth, types, x, y, z);
   }
 

chunky/src/test/se/llbit/math/OctreeTest.java

@@ -0,0 +1,43 @@
+package se.llbit.math;
+
+import org.junit.jupiter.api.Test;
+
+import static se.llbit.testutil.TestUtils.assertThrowsWithExpectedMessage;
+
+public class OctreeTest {
+  @Test
+  public void test() {
+    int depth = 5;
+    int cubeDepth = 3;
+
+    Octree octree = new Octree(new PackedOctree(depth));
+
+    int cubeSize = 1 << cubeDepth;
+    int[] types = new int[cubeSize * cubeSize * cubeSize];
+    int cubesWithinOctree = (1 << depth) - cubeSize; // the number of times the cube can fit along each dimension of the octree
+    for (int x = 0; x < cubesWithinOctree; x++) {
+      for (int y = 0; y < cubesWithinOctree; y++) {
+        for (int z = 0; z < cubesWithinOctree; z++) {
+          octree.setCube(cubeDepth, types, x, y, z);
+        }
+      }
+    }
+
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 25, 0, 0),
+      "setCube x (25,33) out of bounds for octree (0,31)");
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 0, 25, 0),
+      "setCube y (25,33) out of bounds for octree (0,31)");
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 0, 0, 25),
+      "setCube z (25,33) out of bounds for octree (0,31)");
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 64, 13, 23),
+      "setCube x (64,72) out of bounds for octree (0,31)");
+
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, -1, 22, 1),
+      "setCube position must not be negative (-1,22,1)");
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 5, -123, 23),
+      "setCube position must not be negative (5,-123,23)");
+    assertThrowsWithExpectedMessage(AssertionError.class, () -> octree.setCube(cubeDepth, types, 17, 9, -32),
+      "setCube position must not be negative (17,9,-32)");
+
+  }
+}

chunky/src/test/se/llbit/math/QuickMathTest.java

@@ -79,4 +79,16 @@ public void testAbs_1() {
     assertEquals(0.0, QuickMath.abs(0.0));
     //assertEquals(0.0, QuickMath.abs(-0.0)); This actually fails... might want to fix // TODO: abs(-0) returns -0
 	}
+
+  @Test
+  public void testPreviousMultipleOf16() {
+    assertEquals(-1293856, QuickMath.prevMul16(-1293856));
+    assertEquals(12395856, QuickMath.prevMul16(12395867));
+    assertEquals(16, QuickMath.prevMul16(16));
+    assertEquals(32, QuickMath.prevMul16(32));
+    assertEquals(64, QuickMath.prevMul16(64));
+    assertEquals(0, QuickMath.prevMul16(6));
+    assertEquals(-16, QuickMath.prevMul16(-1));
+    assertEquals(-128, QuickMath.prevMul16(-123));
+  }
 }

chunky/src/test/se/llbit/testutil/TestUtils.java

@@ -0,0 +1,14 @@
+package se.llbit.testutil;
+
+import org.junit.jupiter.api.function.Executable;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class TestUtils {
+  public static <T extends Throwable> T assertThrowsWithExpectedMessage(Class<T> expectedType, Executable executable, String expectedMessage) {
+    T t = assertThrows(expectedType, executable);
+    assertEquals(t.getMessage(), expectedMessage, "Error message differs from expected");
+    return t;
+  }
+}

lib/src/se/llbit/math/QuickMath.java

@@ -175,4 +175,18 @@ public static int gcd(int a, int b) {
     }
     return a;
   }
+
+  /**
+   * Previous multiple of 16 of val (Or given value, if a multiple of 16). Respects negative numbers.
+   * <h4>Examples:</h4>
+   * <ul>
+   * <li>1 returns 0</li>
+   * <li>34 returns 32</li>
+   * <li>16 returns 16</li>
+   * <li>-1 returns -16</li>
+   * </ul>
+   */
+  public static int prevMul16(int val) {
+    return val & ~0xf;
+  }
 }