Backport changes (#88)

* Changes to improve compatibility with Bungee servers (#85) * Change handling of server connect processing * Connect/disconnect based on localplayer instance * Keyhole sound pruning for miscconfigured sound origins --------- Co-authored-by: OreCruncher <[email protected]> * Address concurrent access to randomizer * Update docs --------- Co-authored-by: OreCruncher <[email protected]>
OreCruncher · Feb 17, 2024 · d72df39 · d72df39
1 parent 317515b
commit d72df39
Show file tree

Hide file tree

Showing 17 changed files with 152 additions and 85 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,14 @@
+> ### DynamicSurroundings-Fabric-1.20.1-0.3.3
+**Requirements**
+* JAVA 17+ (I am using Adoptium https://adoptium.net/)
+* Fabric Loader >=0.14.22
+* Fabric API >=0.91.0+1.20.1
+* 100% client side; no server side deployment needed
+
+**Fixes**
+* Better support for BungeeCord server operations, specifically server transfer
+* Fix concurrent access exception on randomizer instances
+
 > ### DynamicSurroundings-Fabric-1.20.1-0.3.2
 **Requirements**
 * JAVA 17+ (I am using Adoptium https://adoptium.net/)

diff --git a/gradle.properties b/gradle.properties
@@ -11,7 +11,7 @@ loader_version=0.14.22
 
 # Mod Properties
 mod_id=dsurround
-mod_version=0.3.2
+mod_version=0.3.3
 maven_group=org.orecruncher
 archives_base_name=DynamicSurroundings-Fabric
 

diff --git a/src/main/java/org/orecruncher/dsurround/config/libraries/impl/TagLibrary.java b/src/main/java/org/orecruncher/dsurround/config/libraries/impl/TagLibrary.java
@@ -40,16 +40,16 @@ public class TagLibrary implements ITagLibrary {
 
     private final IModLog logger;
     private final ISystemClock systemClock;
-
     private final Map<TagKey<?>, Collection<ResourceLocation>> tagCache;
-    private ClientTagLoader tagLoader;
+    private final ClientTagLoader tagLoader;
 
     private boolean isConnected;
 
     public TagLibrary(IModLog logger, ISystemClock systemClock) {
         this.logger = logger;
         this.systemClock = systemClock;
         this.tagCache = new Reference2ObjectOpenHashMap<>();
+        this.tagLoader = new ClientTagLoader(ResourceUtilities.createForCurrentState(), this.logger, this.systemClock);
 
         // Need to clear the tag caches on disconnect. It's possible that
         // cached biome information will change with the next connection.
@@ -144,11 +144,7 @@ public Stream<String> dump() {
 
     @Override
     public void reload(ResourceUtilities resourceUtilities, IReloadEvent.Scope scope) {
-        if (resourceUtilities != null)
-            this.tagLoader = new ClientTagLoader(resourceUtilities, this.logger, this.systemClock);
-
-        if (scope == IReloadEvent.Scope.RESOURCES)
-            return;
+        this.tagLoader.setResourceUtilities(resourceUtilities);
 
         this.logger.info("[TagLibrary] Cache has %d elements", this.tagCache.size());
 
@@ -198,9 +194,6 @@ private void onDisconnect(Minecraft client) {
     }
 
     private void initializeTagCache() {
-        if (this.tagLoader == null)
-            return;
-
         // Bootstrap the tag cache. We do this by zipping through our tags
         // and forcing the cache to initialize.
         var stopwatch = this.systemClock.getStopwatch();

diff --git a/src/main/java/org/orecruncher/dsurround/eventing/ClientState.java b/src/main/java/org/orecruncher/dsurround/eventing/ClientState.java
@@ -2,7 +2,9 @@
 
 import net.minecraft.client.Minecraft;
 import net.minecraft.core.RegistryAccess;
+import org.orecruncher.dsurround.lib.Library;
 import org.orecruncher.dsurround.lib.events.EventingFactory;
+import org.orecruncher.dsurround.lib.events.HandlerPriority;
 import org.orecruncher.dsurround.lib.events.IPhasedEvent;
 
 /**
@@ -76,6 +78,35 @@ public final class ClientState {
     private ClientState() {
     }
 
+
+    static {
+        // Connection detection is the first thing that processes, period.
+        TICK_START.register(ClientState::connectionDetector, HandlerPriority.VERY_HIGH);
+    }
+
+    private static boolean isConnected = false;
+    private static void connectionDetector(Minecraft client) {
+        // Basically, the logic will toggle isConnected based on whether a player instance
+        // is present in the Minecraft client instance. Since this is a 100% client side,
+        // the presence of the player instance can be used as a signal as to when the
+        // client successfully connects to a server. If the player instance goes away, such
+        // as a disconnect or a BungeeCord server transfer, the disconnect event will be fired
+        // so dependent logic can clean up.
+        if (isConnected) {
+            if (client.player == null) {
+                isConnected = false;
+                Library.LOGGER.info("Player instance no longer present");
+                ON_DISCONNECT.raise().onDisconnect(client);
+            }
+        } else {
+            if (client.player != null) {
+                isConnected = true;
+                Library.LOGGER.info("Player instance is now present");
+                ON_CONNECT.raise().onConnect(client);
+            }
+        }
+    }
+
     @FunctionalInterface
     public interface IClientStarted {
         void onStart(Minecraft client);

diff --git a/src/main/java/org/orecruncher/dsurround/lib/MinecraftServerType.java b/src/main/java/org/orecruncher/dsurround/lib/MinecraftServerType.java
@@ -5,7 +5,7 @@ public enum MinecraftServerType {
     PAPER(false),
     FABRIC(true),
     FORGE(true),
-    OTHER_MODDED(true);
+    OTHER(false);
 
     private final boolean isModded;
 
@@ -27,6 +27,6 @@ public static MinecraftServerType fromBrand(String serverBrand) {
             return FORGE;
         if ("fabric".equals(brand))
             return FABRIC;
-        return OTHER_MODDED;
+        return OTHER;
     }
 }
diff --git a/src/main/java/org/orecruncher/dsurround/lib/random/Randomizer.java b/src/main/java/org/orecruncher/dsurround/lib/random/Randomizer.java
@@ -1,5 +1,8 @@
 package org.orecruncher.dsurround.lib.random;
 
+import net.minecraft.util.RandomSource;
+import net.minecraft.world.level.levelgen.PositionalRandomFactory;
+import org.jetbrains.annotations.NotNull;
 import org.orecruncher.dsurround.lib.Library;
 
 import java.util.function.Supplier;
@@ -11,26 +14,73 @@
  * the number of randoms generated per tick, I sweat these details.
  */
 @SuppressWarnings("unused")
-public class Randomizer {
+public final class Randomizer implements IRandomizer {
     private static final Supplier<IRandomizer> DEFAULT_RANDOMIZER = Randomizer::getRandomizer;
-    private static final ThreadLocal<IRandomizer> SHARED = ThreadLocal.withInitial(DEFAULT_RANDOMIZER);
-
+    private static final ThreadLocal<IRandomizer> THREAD_LOCAL = ThreadLocal.withInitial(DEFAULT_RANDOMIZER);
     /**
-     * Instantiates a new instance of the default randomizer.
+     * Reusable instance that wraps a ThreadLocal. Guards against multiple threads trying to utilize the same
+     * concrete randomizer.
      */
-    public static IRandomizer create() {
-        return DEFAULT_RANDOMIZER.get();
-    }
+    private static final IRandomizer SHARED = new Randomizer();
 
     /**
      * Returns a shared instance of the default randomizer.
      */
     public static IRandomizer current() {
-        return SHARED.get();
+        return SHARED;
     }
 
     private Randomizer() {
+    }
+
+    @Override
+    public @NotNull RandomSource fork() {
+        return THREAD_LOCAL.get().fork();
+    }
+
+    @Override
+    public @NotNull PositionalRandomFactory forkPositional() {
+        return THREAD_LOCAL.get().forkPositional();
+    }
+
+    @Override
+    public void setSeed(long l) {
+        THREAD_LOCAL.get().setSeed(l);
+    }
+
+    @Override
+    public int nextInt() {
+        return THREAD_LOCAL.get().nextInt();
+    }
+
+    @Override
+    public int nextInt(int i) {
+        return THREAD_LOCAL.get().nextInt(i);
+    }
+
+    @Override
+    public long nextLong() {
+        return THREAD_LOCAL.get().nextLong();
+    }
+
+    @Override
+    public boolean nextBoolean() {
+        return THREAD_LOCAL.get().nextBoolean();
+    }
+
+    @Override
+    public float nextFloat() {
+        return THREAD_LOCAL.get().nextFloat();
+    }
+
+    @Override
+    public double nextDouble() {
+        return THREAD_LOCAL.get().nextDouble();
+    }
 
+    @Override
+    public double nextGaussian() {
+        return THREAD_LOCAL.get().nextGaussian();
     }
 
     private static IRandomizer getRandomizer() {

diff --git a/src/main/java/org/orecruncher/dsurround/lib/resources/ClientTagLoader.java b/src/main/java/org/orecruncher/dsurround/lib/resources/ClientTagLoader.java
@@ -4,7 +4,6 @@
 import com.google.common.collect.ImmutableSet;
 import it.unimi.dsi.fastutil.objects.Reference2ObjectOpenHashMap;
 import net.minecraft.core.Holder;
-import net.minecraft.core.Registry;
 import net.minecraft.resources.ResourceLocation;
 import net.minecraft.tags.TagEntry;
 import net.minecraft.tags.TagKey;
@@ -24,14 +23,15 @@
 @SuppressWarnings("unused")
 public class ClientTagLoader {
 
-    private final ResourceUtilities resourceUtilities;
     private final IModLog logger;
     private final ISystemClock systemClock;
     private final Map<TagKey<?>, TagData<?>> tagCache = new Reference2ObjectOpenHashMap<>(256);
 
     private MinecraftServerType serverType;
+    @NotNull
+    private ResourceUtilities resourceUtilities;
 
-    public ClientTagLoader(ResourceUtilities resourceUtilities, IModLog logger, ISystemClock systemClock) {
+    public ClientTagLoader(@NotNull ResourceUtilities resourceUtilities, IModLog logger, ISystemClock systemClock) {
         this.resourceUtilities = resourceUtilities;
         this.logger = logger;
         this.systemClock = systemClock;
@@ -47,11 +47,23 @@ public <T> Collection<ResourceLocation> getCompleteIds(TagKey<T> tagKey) {
     }
 
     public void clear() {
+        this.logger.debug(RESOURCE_LOADING, "Clearing client tag loader");
         this.tagCache.clear();
     }
 
+    public void setResourceUtilities(ResourceUtilities resourceUtilities) {
+        if (this.resourceUtilities != resourceUtilities) {
+            this.resourceUtilities = resourceUtilities;
+            this.clear();
+        }
+    }
+
     public void setServerType(MinecraftServerType serverType) {
-        this.serverType = serverType;
+        // If the server type is changing, clear the cache
+        if (this.serverType != serverType) {
+            this.clear();
+            this.serverType = serverType;
+        }
     }
 
     private <T> TagData<T> getTagData(TagKey<T> tagKey, Set<TagKey<?>> visited) {
@@ -85,8 +97,7 @@ private <T> TagData<T> loadTagData(TagKey<T> tagKey, Set<TagKey<?>> visited) {
 
         // If we can take a shortcut by looking up tag membership in the registries, do so. It's
         // faster than scanning resources directly.
-        var registry = RegistryUtils.getRegistry(tagKey.registry()).orElseThrow();
-        var holderSet = this.shortcutLookup(tagKey, registry);
+        var holderSet = this.shortcutLookup(tagKey);
         if (holderSet.isPresent()) {
             var data = holderSet.get();
             this.logger.debug(RESOURCE_LOADING, "%s - Shortcut lookup", tagKey);
@@ -113,6 +124,7 @@ private <T> TagData<T> loadTagData(TagKey<T> tagKey, Set<TagKey<?>> visited) {
                     public @NotNull ResourceLocation element(@NotNull ResourceLocation id) {
                         return id;
                     }
+
                     @Nullable
                     @Override
                     public Collection<ResourceLocation> tag(@NotNull ResourceLocation id) {
@@ -144,15 +156,20 @@ public Collection<ResourceLocation> tag(@NotNull ResourceLocation id) {
         return new TagData<>(ImmutableSet.copyOf(completeIds));
     }
 
-    private <T> Optional<Iterable<Holder<T>>> shortcutLookup(TagKey<T> tagKey, Registry<T> registry) {
+    private <T> Optional<Iterable<Holder<T>>> shortcutLookup(TagKey<T> tagKey) {
         var namespace = tagKey.location().getNamespace();
         // If its one of our tags, we always do the long lookup. They aren't really tags.
         if (namespace.equals(Library.MOD_ID))
             return Optional.empty();
         // If it is a modded server, or the namespace is minecraft, tag information should be
         // present in the local registries.
         if (this.serverType.isModded() || "minecraft".equals(namespace)) {
-            var holderSet = registry.getTag(tagKey);
+            // If for some reason the registry cannot be found, do a slow scan. I have seen this with
+            // BungeeCord when a player transitions between servers.
+            var registry = RegistryUtils.getRegistry(tagKey.registry());
+            if (registry.isEmpty())
+                return Optional.empty();
+            var holderSet = registry.get().getTag(tagKey);
             if (holderSet.isPresent())
                 return Optional.of(holderSet.get());
             return Optional.of(ImmutableList.of());
@@ -170,7 +187,7 @@ public static <T> TagData<T> empty() {
 
         @SuppressWarnings("unchecked")
         public static <T> TagData<T> cast(TagData<?> tagData) {
-            return (TagData<T>)tagData;
+            return (TagData<T>) tagData;
         }
     }
 }
diff --git a/src/main/java/org/orecruncher/dsurround/mixins/events/MixinClientPacketListener.java b/src/main/java/org/orecruncher/dsurround/mixins/events/MixinClientPacketListener.java
diff --git a/src/main/java/org/orecruncher/dsurround/mixins/events/MixinMinecraftClient.java b/src/main/java/org/orecruncher/dsurround/mixins/events/MixinMinecraftClient.java
@@ -3,7 +3,6 @@
 import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
 import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
 import net.minecraft.client.Minecraft;
-import net.minecraft.client.gui.screens.Screen;
 import net.minecraft.client.player.LocalPlayer;
 import net.minecraft.world.entity.player.Abilities;
 import org.orecruncher.dsurround.eventing.ClientState;
@@ -40,15 +39,6 @@ private void dsurround_starting(CallbackInfo ci) {
         ClientState.STARTED.raise().onStart((Minecraft) (Object) this);
     }
 
-    /**
-     * When Minecraft disconnects from a server disconnect() gets called to clean up state.
-     */
-    @Inject(method = "clearLevel(Lnet/minecraft/client/gui/screens/Screen;)V", at = @At("RETURN"))
-    private void dsurround_leave(Screen screen, CallbackInfo ci) {
-        var self = (Minecraft) (Object) this;
-        ClientState.ON_DISCONNECT.raise().onDisconnect(self);
-    }
-
     /**
      * Hooks getting player abilities when checking whether to play situational music or the standard
      * creative Minecraft music when the player is in creative mode and in a dimension other than the Nether.