From 50c0f01fb295a5904ab4b31c7f53c157773338b1 Mon Sep 17 00:00:00 2001
From: TheMode <themode@outlook.fr>
Date: Sat, 26 Feb 2022 00:10:26 +0100
Subject: [PATCH] View engine improvements (#715)

---
 .../net/minestom/server/entity/Entity.java    |  33 +-----
 .../EntityView.java}                          | 102 +++++++-----------
 .../net/minestom/server/instance/Chunk.java   |  24 +----
 .../minestom/server/instance/ChunkView.java   |  64 +++++++++++
 .../minestom/server/utils/ViewEngineTest.java |  45 --------
 5 files changed, 111 insertions(+), 157 deletions(-)
 rename src/main/java/net/minestom/server/{utils/ViewEngine.java => entity/EntityView.java} (79%)
 create mode 100644 src/main/java/net/minestom/server/instance/ChunkView.java
 delete mode 100644 src/test/java/net/minestom/server/utils/ViewEngineTest.java

diff --git a/src/main/java/net/minestom/server/entity/Entity.java b/src/main/java/net/minestom/server/entity/Entity.java
index bf3ec3fbf..4b8ee7b45 100644
--- a/src/main/java/net/minestom/server/entity/Entity.java
+++ b/src/main/java/net/minestom/server/entity/Entity.java
@@ -40,7 +40,6 @@ import net.minestom.server.timer.Schedulable;
 import net.minestom.server.timer.Scheduler;
 import net.minestom.server.timer.TaskSchedule;
 import net.minestom.server.utils.PacketUtils;
-import net.minestom.server.utils.ViewEngine;
 import net.minestom.server.utils.async.AsyncUtils;
 import net.minestom.server.utils.block.BlockIterator;
 import net.minestom.server.utils.chunk.ChunkUtils;
@@ -135,36 +134,8 @@ public class Entity implements Viewable, Tickable, Schedulable, TagHandler, Perm
         }
     };
 
-    protected final ViewEngine viewEngine = new ViewEngine(this,
-            player -> {
-                // Add viewable
-                var lock1 = player.getEntityId() < getEntityId() ? player : this;
-                var lock2 = lock1 == this ? player : this;
-                synchronized (lock1.viewEngine.mutex()) {
-                    synchronized (lock2.viewEngine.mutex()) {
-                        if (!Entity.this.viewEngine.viewableOption.predicate(player) ||
-                                !player.viewEngine.viewerOption.predicate(this)) return;
-                        Entity.this.viewEngine.viewableOption.register(player);
-                        player.viewEngine.viewerOption.register(this);
-                    }
-                }
-                updateNewViewer(player);
-            },
-            player -> {
-                // Remove viewable
-                var lock1 = player.getEntityId() < getEntityId() ? player : this;
-                var lock2 = lock1 == this ? player : this;
-                synchronized (lock1.viewEngine.mutex()) {
-                    synchronized (lock2.viewEngine.mutex()) {
-                        Entity.this.viewEngine.viewableOption.unregister(player);
-                        player.viewEngine.viewerOption.unregister(this);
-                    }
-                }
-                updateOldViewer(player);
-            },
-            this instanceof Player player ? entity -> entity.viewEngine.viewableOption.addition.accept(player) : null,
-            this instanceof Player player ? entity -> entity.viewEngine.viewableOption.removal.accept(player) : null);
-    protected final Set<Player> viewers = viewEngine.asSet();
+    protected final EntityView viewEngine = new EntityView(this);
+    protected final Set<Player> viewers = viewEngine.set;
     private final MutableNBTCompound nbtCompound = new MutableNBTCompound();
     private final Scheduler scheduler = Scheduler.newScheduler();
     private final Set<Permission> permissions = new CopyOnWriteArraySet<>();
diff --git a/src/main/java/net/minestom/server/utils/ViewEngine.java b/src/main/java/net/minestom/server/entity/EntityView.java
similarity index 79%
rename from src/main/java/net/minestom/server/utils/ViewEngine.java
rename to src/main/java/net/minestom/server/entity/EntityView.java
index fc91e5257..9d54f4d7e 100644
--- a/src/main/java/net/minestom/server/utils/ViewEngine.java
+++ b/src/main/java/net/minestom/server/entity/EntityView.java
@@ -1,17 +1,14 @@
-package net.minestom.server.utils;
+package net.minestom.server.entity;
 
 import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
 import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
 import it.unimi.dsi.fastutil.ints.IntSet;
 import net.minestom.server.MinecraftServer;
 import net.minestom.server.coordinate.Point;
-import net.minestom.server.entity.Entity;
-import net.minestom.server.entity.Player;
 import net.minestom.server.instance.EntityTracker;
 import net.minestom.server.instance.Instance;
 import net.minestom.server.instance.InstanceContainer;
 import net.minestom.server.instance.SharedInstance;
-import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -20,13 +17,9 @@ import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 import java.util.function.Consumer;
 import java.util.function.Predicate;
 
-/**
- * Defines which players are able to see this element.
- */
-@ApiStatus.Internal
-public final class ViewEngine {
+final class EntityView {
+    private static final int RANGE = MinecraftServer.getEntityViewDistance() * 16;
     private final Entity entity;
-    private final int range;
     private final Set<Player> manualViewers = new HashSet<>();
 
     // Decide if this entity should be viewable to X players
@@ -34,32 +27,42 @@ public final class ViewEngine {
     // Decide if this entity should view X entities
     public final Option<Entity> viewerOption;
 
-    private final Set<Player> set;
+    final Set<Player> set = new SetImpl();
     private final Object mutex = this;
 
     private volatile TrackedLocation trackedLocation;
 
-    public ViewEngine(@Nullable Entity entity,
-                      Consumer<Player> autoViewableAddition, Consumer<Player> autoViewableRemoval,
-                      Consumer<Entity> autoViewerAddition, Consumer<Entity> autoViewerRemoval) {
+    public EntityView(Entity entity) {
         this.entity = entity;
-        if (entity != null) {
-            this.range = MinecraftServer.getEntityViewDistance() * 16;
-            this.set = new EntitySet();
-        } else {
-            this.range = MinecraftServer.getChunkViewDistance() * 16;
-            this.set = new ChunkSet();
-        }
-        this.viewableOption = new Option<>(EntityTracker.Target.PLAYERS, Entity::autoViewEntities, autoViewableAddition, autoViewableRemoval);
-        this.viewerOption = new Option<>(EntityTracker.Target.ENTITIES, Entity::isAutoViewable, autoViewerAddition, autoViewerRemoval);
-    }
-
-    public ViewEngine(@Nullable Entity entity) {
-        this(entity, null, null, null, null);
-    }
-
-    public ViewEngine() {
-        this(null);
+        this.viewableOption = new Option<>(EntityTracker.Target.PLAYERS, Entity::autoViewEntities,
+                player -> {
+                    // Add viewable
+                    var lock1 = player.getEntityId() < entity.getEntityId() ? player : entity;
+                    var lock2 = lock1 == entity ? player : entity;
+                    synchronized (lock1.viewEngine.mutex) {
+                        synchronized (lock2.viewEngine.mutex) {
+                            if (!entity.viewEngine.viewableOption.predicate(player) ||
+                                    !player.viewEngine.viewerOption.predicate(entity)) return;
+                            entity.viewEngine.viewableOption.register(player);
+                            player.viewEngine.viewerOption.register(entity);
+                        }
+                    }
+                    entity.updateNewViewer(player);
+                }, player -> {
+            // Remove viewable
+            var lock1 = player.getEntityId() < entity.getEntityId() ? player : entity;
+            var lock2 = lock1 == entity ? player : entity;
+            synchronized (lock1.viewEngine.mutex) {
+                synchronized (lock2.viewEngine.mutex) {
+                    entity.viewEngine.viewableOption.unregister(player);
+                    player.viewEngine.viewerOption.unregister(entity);
+                }
+            }
+            entity.updateOldViewer(player);
+        });
+        this.viewerOption = new Option<>(EntityTracker.Target.ENTITIES, Entity::isAutoViewable,
+                entity instanceof Player player ? e -> e.viewEngine.viewableOption.addition.accept(player) : null,
+                entity instanceof Player player ? e -> e.viewEngine.viewableOption.removal.accept(player) : null);
     }
 
     public void updateTracker(@Nullable Instance instance, @NotNull Point point) {
@@ -124,17 +127,9 @@ public final class ViewEngine {
         }
     }
 
-    public Object mutex() {
-        return mutex;
-    }
-
-    public Set<Player> asSet() {
-        return set;
-    }
-
     public final class Option<T extends Entity> {
         @SuppressWarnings("rawtypes")
-        private static final AtomicIntegerFieldUpdater<Option> UPDATER = AtomicIntegerFieldUpdater.newUpdater(Option.class, "auto");
+        private static final AtomicIntegerFieldUpdater<EntityView.Option> UPDATER = AtomicIntegerFieldUpdater.newUpdater(EntityView.Option.class, "auto");
         // Entities that should be tracked from this option
         private final EntityTracker.Target<T> target;
         // The condition that must be met for this option to be considered auto.
@@ -212,7 +207,7 @@ public final class ViewEngine {
         private void update(Predicate<T> visibilityPredicate,
                             Consumer<T> action) {
             references().forEach(entity -> {
-                if (entity == ViewEngine.this.entity || !visibilityPredicate.test(entity)) return;
+                if (entity == EntityView.this.entity || !visibilityPredicate.test(entity)) return;
                 if (entity instanceof Player player && manualViewers.contains(player)) return;
                 if (entity.getVehicle() != null) return;
                 action.accept(entity);
@@ -222,21 +217,21 @@ public final class ViewEngine {
         private int lastSize;
 
         private Collection<T> references() {
-            final TrackedLocation trackedLocation = ViewEngine.this.trackedLocation;
+            final TrackedLocation trackedLocation = EntityView.this.trackedLocation;
             if (trackedLocation == null) return List.of();
             final Instance instance = trackedLocation.instance();
             final Point point = trackedLocation.point();
 
             Int2ObjectOpenHashMap<T> entityMap = new Int2ObjectOpenHashMap<>(lastSize);
             // Current Instance
-            instance.getEntityTracker().nearbyEntities(point, range, target,
+            instance.getEntityTracker().nearbyEntities(point, RANGE, target,
                     (entity) -> entityMap.putIfAbsent(entity.getEntityId(), entity));
             // Shared Instances
             if (instance instanceof InstanceContainer container) {
                 final List<SharedInstance> shared = container.getSharedInstances();
                 if (!shared.isEmpty()) {
                     for (var sharedInstance : shared) {
-                        sharedInstance.getEntityTracker().nearbyEntities(point, range, target,
+                        sharedInstance.getEntityTracker().nearbyEntities(point, RANGE, target,
                                 (entity) -> entityMap.putIfAbsent(entity.getEntityId(), entity));
                     }
                 }
@@ -246,24 +241,7 @@ public final class ViewEngine {
         }
     }
 
-    final class ChunkSet extends AbstractSet<Player> {
-        @Override
-        public @NotNull Iterator<Player> iterator() {
-            return viewableOption.references().iterator();
-        }
-
-        @Override
-        public int size() {
-            return viewableOption.references().size();
-        }
-
-        @Override
-        public void forEach(Consumer<? super Player> action) {
-            viewableOption.references().forEach(action);
-        }
-    }
-
-    final class EntitySet extends AbstractSet<Player> {
+    final class SetImpl extends AbstractSet<Player> {
         @Override
         public @NotNull Iterator<Player> iterator() {
             synchronized (mutex) {
diff --git a/src/main/java/net/minestom/server/instance/Chunk.java b/src/main/java/net/minestom/server/instance/Chunk.java
index 6570debe4..3f7abca94 100644
--- a/src/main/java/net/minestom/server/instance/Chunk.java
+++ b/src/main/java/net/minestom/server/instance/Chunk.java
@@ -10,7 +10,6 @@ import net.minestom.server.instance.block.Block;
 import net.minestom.server.network.packet.server.play.ChunkDataPacket;
 import net.minestom.server.tag.Tag;
 import net.minestom.server.tag.TagHandler;
-import net.minestom.server.utils.ViewEngine;
 import net.minestom.server.utils.chunk.ChunkSupplier;
 import net.minestom.server.utils.chunk.ChunkUtils;
 import net.minestom.server.world.biomes.Biome;
@@ -51,7 +50,7 @@ public abstract class Chunk implements Block.Getter, Block.Setter, Biome.Getter,
     private boolean readOnly;
 
     protected volatile boolean loaded = true;
-    private final ViewEngine viewers = new ViewEngine();
+    private final ChunkView viewers;
 
     // Path finding
     protected PFColumnarSpace columnarSpace;
@@ -67,8 +66,7 @@ public abstract class Chunk implements Block.Getter, Block.Setter, Biome.Getter,
         this.shouldGenerate = shouldGenerate;
         this.minSection = instance.getDimensionType().getMinY() / CHUNK_SECTION_SIZE;
         this.maxSection = (instance.getDimensionType().getMinY() + instance.getDimensionType().getHeight()) / CHUNK_SECTION_SIZE;
-
-        this.viewers.updateTracker(instance, toPosition());
+        this.viewers = new ChunkView(instance, toPosition());
     }
 
     /**
@@ -268,31 +266,19 @@ public abstract class Chunk implements Block.Getter, Block.Setter, Biome.Getter,
         return getClass().getSimpleName() + "[" + chunkX + ":" + chunkZ + "]";
     }
 
-    /**
-     * Adds the player to the viewing collection. Chunk packet must be sent manually.
-     *
-     * @param player the viewer to add
-     * @return true if the player has just been added to the viewer collection
-     */
     @Override
     public boolean addViewer(@NotNull Player player) {
-        return viewers.manualAdd(player);
+        throw new UnsupportedOperationException("Chunk does not support manual viewers");
     }
 
-    /**
-     * Removes the player from the viewing collection. Chunk packet must be sent manually.
-     *
-     * @param player the viewer to remove
-     * @return true if the player has just been removed to the viewer collection
-     */
     @Override
     public boolean removeViewer(@NotNull Player player) {
-        return viewers.manualRemove(player);
+        throw new UnsupportedOperationException("Chunk does not support manual viewers");
     }
 
     @Override
     public @NotNull Set<Player> getViewers() {
-        return viewers.asSet();
+        return viewers.set;
     }
 
     @Override
diff --git a/src/main/java/net/minestom/server/instance/ChunkView.java b/src/main/java/net/minestom/server/instance/ChunkView.java
new file mode 100644
index 000000000..6d3d8510f
--- /dev/null
+++ b/src/main/java/net/minestom/server/instance/ChunkView.java
@@ -0,0 +1,64 @@
+package net.minestom.server.instance;
+
+import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap;
+import net.minestom.server.MinecraftServer;
+import net.minestom.server.coordinate.Point;
+import net.minestom.server.entity.Player;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.*;
+import java.util.function.Consumer;
+
+final class ChunkView {
+    private static final int RANGE = MinecraftServer.getChunkViewDistance() * 16;
+    private final Instance instance;
+    private final Point point;
+    final Set<Player> set = new SetImpl();
+
+    private int lastReferenceCount;
+
+    ChunkView(Instance instance, Point point) {
+        this.instance = instance;
+        this.point = point;
+    }
+
+    private Collection<Player> references() {
+        final Instance instance = this.instance;
+        final Point point = this.point;
+        final var target = EntityTracker.Target.PLAYERS;
+
+        Int2ObjectOpenHashMap<Player> entityMap = new Int2ObjectOpenHashMap<>(lastReferenceCount);
+        // Current Instance
+        instance.getEntityTracker().nearbyEntities(point, RANGE, target,
+                (entity) -> entityMap.putIfAbsent(entity.getEntityId(), entity));
+        // Shared Instances
+        if (instance instanceof InstanceContainer container) {
+            final List<SharedInstance> shared = container.getSharedInstances();
+            if (!shared.isEmpty()) {
+                for (var sharedInstance : shared) {
+                    sharedInstance.getEntityTracker().nearbyEntities(point, RANGE, target,
+                            (entity) -> entityMap.putIfAbsent(entity.getEntityId(), entity));
+                }
+            }
+        }
+        this.lastReferenceCount = entityMap.size();
+        return entityMap.values();
+    }
+
+    final class SetImpl extends AbstractSet<Player> {
+        @Override
+        public @NotNull Iterator<Player> iterator() {
+            return references().iterator();
+        }
+
+        @Override
+        public int size() {
+            return references().size();
+        }
+
+        @Override
+        public void forEach(Consumer<? super Player> action) {
+            references().forEach(action);
+        }
+    }
+}
diff --git a/src/test/java/net/minestom/server/utils/ViewEngineTest.java b/src/test/java/net/minestom/server/utils/ViewEngineTest.java
deleted file mode 100644
index da8eedd28..000000000
--- a/src/test/java/net/minestom/server/utils/ViewEngineTest.java
+++ /dev/null
@@ -1,45 +0,0 @@
-package net.minestom.server.utils;
-
-import net.minestom.server.entity.Entity;
-import net.minestom.server.entity.Player;
-import org.junit.jupiter.api.Test;
-
-import java.util.UUID;
-
-import static org.junit.jupiter.api.Assertions.*;
-
-public class ViewEngineTest {
-
-    @Test
-    public void empty() {
-        ViewEngine viewEngine = new ViewEngine();
-        var set = viewEngine.asSet();
-        assertEquals(0, set.size());
-        // forEach
-        set.forEach(player -> fail("The engine should have no entity"));
-        // Iterator
-        var iterator = set.iterator();
-        assertFalse(iterator.hasNext());
-        // Array
-        assertArrayEquals(new Entity[0], set.toArray());
-    }
-
-    @Test
-    public void playerEmpty() {
-        Player owner = createPlayer();
-        ViewEngine viewEngine = new ViewEngine(owner);
-        var set = viewEngine.asSet();
-        assertEquals(0, set.size());
-        // forEach
-        set.forEach(player -> fail("The engine should have no entity"));
-        // Iterator
-        var iterator = set.iterator();
-        assertFalse(iterator.hasNext());
-        // Array
-        assertArrayEquals(new Entity[0], set.toArray());
-    }
-
-    static Player createPlayer() {
-        return new Player(UUID.randomUUID(), "test", null);
-    }
-}