Repository: movingblocks/terasology
Issue: Region3i.EMPTY bug

There is a serious flaw in the way Region3i deals with empty regions.

For some reason (optimization?) Region3i has a public and static instance Region3i.EMPTY.
There is nothing that prevents users from accidentally changing the empty region.
What makes it even worse is that the factory functions sometimes just return Region3i.EMPTY, to save the costs of a copy constructor.

This code snippet can destroy the invariant of the empty region being empty:
```
Vector3i a = foo();
Vector3i b = bar();
Region3i region = Region3i.createFromMinMax(a, b);
region.expand(otherRegion); // Oops, Region3i.EMPTY is now equal to otherRegion
```
### How to confirm
This test fails if added to the Region3i tests.
```
@Test
public void testFactoryShouldNotReturnTheEmptySingleton() {
    Region3i constructed = Region3i.createFromMinMax(Vector3i.zero(), Vector3i.one().negate());
    assertFalse(constructed == Region3i.EMPTY); // This fails
}
```

### Suggested Solution
Remove Region3i.EMPTY, and no longer threat is as special instance.
Add Region3i.empty(), creating a new empty region each time it is called.

Changed files:
engine/src/main/java/org/terasology/world/block/regions/BlockRegionComponent.java
engine-tests/src/test/java/org/terasology/math/Region3iTest.java
engine/src/main/java/org/terasology/world/chunks/internal/ChunkRelevanceRegion.java
engine/src/main/java/org/terasology/math/Region3i.java
engine/src/main/java/org/terasology/rendering/world/RenderableWorldImpl.java

diff --git a/engine-tests/src/test/java/org/terasology/math/Region3iTest.java b/engine-tests/src/test/java/org/terasology/math/Region3iTest.java
index 056719183..2101f3ba8 100644
--- a/engine-tests/src/test/java/org/terasology/math/Region3iTest.java
+++ b/engine-tests/src/test/java/org/terasology/math/Region3iTest.java
@@ -33,10 +33,9 @@
 public class Region3iTest {
 
     @Test
-    public void testEmptyConstructor() {
-        Region3i region = Region3i.EMPTY;
-        assertEquals(new Vector3i(), region.size());
-        assertTrue(region.isEmpty());
+    public void testEmptyRegion() {
+        assertEquals(Region3i.empty().size(), Vector3i.zero());
+        assertTrue(Region3i.empty().isEmpty());
     }
 
     @Test
@@ -127,7 +126,7 @@ public void testSimpleIntersect() {
     public void testNonTouchingIntersect() {
         Region3i region1 = Region3i.createFromMinMax(new Vector3i(), new Vector3i(32, 32, 32));
         Region3i region2 = Region3i.createFromMinMax(new Vector3i(103, 103, 103), new Vector3i(170, 170, 170));
-        assertEquals(Region3i.EMPTY, region1.intersect(region2));
+        assertEquals(Region3i.empty(), region1.intersect(region2));
     }
 
     @Test
diff --git a/engine/src/main/java/org/terasology/math/Region3i.java b/engine/src/main/java/org/terasology/math/Region3i.java
index 11a706c23..700e0753d 100644
--- a/engine/src/main/java/org/terasology/math/Region3i.java
+++ b/engine/src/main/java/org/terasology/math/Region3i.java
@@ -28,7 +28,14 @@
  *
  */
 public final class Region3i implements Iterable<Vector3i> {
-    public static final Region3i EMPTY = new Region3i();
+
+    /**
+     * @deprecated As of 24 sep 2018, because it is error prone.
+     * Everyone can change this instance and destroy the invariant properties.
+     * Some methods used to return this instance silently on special cases.
+     */
+    @Deprecated
+    public static final Region3i EMPTY = Region3i.empty();
 
     private final Vector3i min = new Vector3i();
     private final Vector3i size = new Vector3i();
@@ -44,6 +51,14 @@ private Region3i(BaseVector3i min, BaseVector3i size) {
         this.size.set(size);
     }
 
+    /**
+     * An empty Region with size (0,0,0).
+     * @return An empty Region3i
+     */
+    public static Region3i empty() {
+        return new Region3i();
+    }
+
     /**
      * @param min the min point of the region
      * @param size the size of the region
@@ -51,7 +66,7 @@ private Region3i(BaseVector3i min, BaseVector3i size) {
      */
     public static Region3i createFromMinAndSize(BaseVector3i min, BaseVector3i size) {
         if (size.x() <= 0 || size.y() <= 0 || size.z() <= 0) {
-            return EMPTY;
+            return empty();
         }
         return new Region3i(min, size);
     }
@@ -118,7 +133,7 @@ public static Region3i createBounded(BaseVector3i a, BaseVector3i b) {
     public static Region3i createFromMinMax(BaseVector3i min, BaseVector3i max) {
         Vector3i size = new Vector3i(max.x() - min.x() + 1, max.y() - min.y() + 1, max.z() - min.z() + 1);
         if (size.x <= 0 || size.y <= 0 || size.z <= 0) {
-            return EMPTY;
+            return empty();
         }
         return new Region3i(min, size);
     }
diff --git a/engine/src/main/java/org/terasology/rendering/world/RenderableWorldImpl.java b/engine/src/main/java/org/terasology/rendering/world/RenderableWorldImpl.java
index 153119133..44a1f3634 100644
--- a/engine/src/main/java/org/terasology/rendering/world/RenderableWorldImpl.java
+++ b/engine/src/main/java/org/terasology/rendering/world/RenderableWorldImpl.java
@@ -65,7 +65,7 @@ class RenderableWorldImpl implements RenderableWorld {
     private ChunkTessellator chunkTessellator;
     private final ChunkMeshUpdateManager chunkMeshUpdateManager;
     private final List<RenderableChunk> chunksInProximityOfCamera = Lists.newArrayListWithCapacity(MAX_LOADABLE_CHUNKS);
-    private Region3i renderableRegion = Region3i.EMPTY;
+    private Region3i renderableRegion = Region3i.empty();
     private ViewDistance currentViewDistance;
     private RenderQueuesHelper renderQueues;
 
diff --git a/engine/src/main/java/org/terasology/world/block/regions/BlockRegionComponent.java b/engine/src/main/java/org/terasology/world/block/regions/BlockRegionComponent.java
index 4c006d86d..8338f8ba5 100644
--- a/engine/src/main/java/org/terasology/world/block/regions/BlockRegionComponent.java
+++ b/engine/src/main/java/org/terasology/world/block/regions/BlockRegionComponent.java
@@ -24,7 +24,7 @@
  */
 public class BlockRegionComponent implements Component {
     @Replicate
-    public Region3i region = Region3i.EMPTY;
+    public Region3i region = Region3i.empty();
     public boolean overrideBlockEntities = true;
 
     public BlockRegionComponent() {
diff --git a/engine/src/main/java/org/terasology/world/chunks/internal/ChunkRelevanceRegion.java b/engine/src/main/java/org/terasology/world/chunks/internal/ChunkRelevanceRegion.java
index 86bc73e08..abecb8a81 100644
--- a/engine/src/main/java/org/terasology/world/chunks/internal/ChunkRelevanceRegion.java
+++ b/engine/src/main/java/org/terasology/world/chunks/internal/ChunkRelevanceRegion.java
@@ -36,8 +36,8 @@ public class ChunkRelevanceRegion {
     private Vector3i relevanceDistance = new Vector3i();
     private boolean dirty;
     private Vector3i center = new Vector3i();
-    private Region3i currentRegion = Region3i.EMPTY;
-    private Region3i previousRegion = Region3i.EMPTY;
+    private Region3i currentRegion = Region3i.empty();
+    private Region3i previousRegion = Region3i.empty();
     private ChunkRegionListener listener;
 
     private Set<Vector3i> relevantChunks = Sets.newLinkedHashSet();
@@ -123,7 +123,7 @@ private Region3i calculateRegion() {
             Vector3i extents = new Vector3i(relevanceDistance.x / 2, relevanceDistance.y / 2, relevanceDistance.z / 2);
             return Region3i.createFromCenterExtents(ChunkMath.calcChunkPos(loc.getWorldPosition()), extents);
         }
-        return Region3i.EMPTY;
+        return Region3i.empty();
     }
 
     private Vector3i calculateCenter() {