From 41a9797791703d2d8c4783e62289cab6314ff958 Mon Sep 17 00:00:00 2001 From: Tom Martin Date: Mon, 23 Sep 2024 22:10:07 +0100 Subject: [PATCH] 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 --- .../se/llbit/chunky/renderer/scene/Scene.java | 21 ++++++--- chunky/src/java/se/llbit/math/Octree.java | 10 +++++ chunky/src/test/se/llbit/math/OctreeTest.java | 43 +++++++++++++++++++ .../src/test/se/llbit/math/QuickMathTest.java | 12 ++++++ .../src/test/se/llbit/testutil/TestUtils.java | 14 ++++++ lib/src/se/llbit/math/QuickMath.java | 14 ++++++ 6 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 chunky/src/test/se/llbit/math/OctreeTest.java create mode 100644 chunky/src/test/se/llbit/testutil/TestUtils.java diff --git a/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java b/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java index 54a69d051c..bfd39a54a1 100644 --- a/chunky/src/java/se/llbit/chunky/renderer/scene/Scene.java +++ b/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 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; } diff --git a/chunky/src/java/se/llbit/math/Octree.java b/chunky/src/java/se/llbit/math/Octree.java index 201bdde496..897180d839 100644 --- a/chunky/src/java/se/llbit/math/Octree.java +++ b/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); } diff --git a/chunky/src/test/se/llbit/math/OctreeTest.java b/chunky/src/test/se/llbit/math/OctreeTest.java new file mode 100644 index 0000000000..7bf743c1f1 --- /dev/null +++ b/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)"); + + } +} diff --git a/chunky/src/test/se/llbit/math/QuickMathTest.java b/chunky/src/test/se/llbit/math/QuickMathTest.java index 433df43787..036552840f 100644 --- a/chunky/src/test/se/llbit/math/QuickMathTest.java +++ b/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)); + } } diff --git a/chunky/src/test/se/llbit/testutil/TestUtils.java b/chunky/src/test/se/llbit/testutil/TestUtils.java new file mode 100644 index 0000000000..2bfe601e40 --- /dev/null +++ b/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 assertThrowsWithExpectedMessage(Class expectedType, Executable executable, String expectedMessage) { + T t = assertThrows(expectedType, executable); + assertEquals(t.getMessage(), expectedMessage, "Error message differs from expected"); + return t; + } +} diff --git a/lib/src/se/llbit/math/QuickMath.java b/lib/src/se/llbit/math/QuickMath.java index 62073c89a6..7120db46ea 100644 --- a/lib/src/se/llbit/math/QuickMath.java +++ b/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. + *

Examples:

+ *
    + *
  • 1 returns 0
    • + *
  • 34 returns 32
    • + *
  • 16 returns 16
    • + *
  • -1 returns -16
    • + *
+ */ + public static int prevMul16(int val) { + return val & ~0xf; + } }