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]>

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();

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);

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;
     }
 }

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;
         }
     }
 }

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 = "disconnect(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.

src/main/java/org/orecruncher/dsurround/processing/AreaBlockEffects.java

@@ -44,13 +44,9 @@ public AreaBlockEffects(IBlockLibrary blockLibrary, IAudioPlayer audioPlayer, Co
     @Override
     public void process(final Player player) {
         this.blockUpdateCount = 0;
-        if (!this.isConnected)
-            return;
 
-        // TODO: Reports that this reduces some crashes.  Not sure how since the connected
-        // would prevent this logic from executing because it sets the isConnected
-        // flag.
-        if (player == null)
+        // Possible that a client connected to a server, but is being transferred (BungeeCord)
+        if (!this.isConnected || !GameUtils.isInGame())
             return;
 
         if (this.effectSystems != null)
@@ -77,21 +73,22 @@ public void onConnect() {
     @Override
     public void onDisconnect() {
         this.isConnected = false;
-
         this.locus = null;
         this.effectSystems = null;
     }
 
     private void clear(ResourceUtilities resourceUtilities, IReloadEvent.Scope scope) {
-        if (this.effectSystems != null)
+        // Possible that a client connected to a server, but is being transferred (BungeeCord)
+        if (this.effectSystems != null && GameUtils.isInGame())
             this.effectSystems.resetFullScan();
     }
 
     private void blockUpdates(Collection<BlockPos> blockPositions) {
         // Need to pump the updates through to the effect system. The cuboid scanner
         // will handle the details for filtering and applying updates via blockScan().
         this.blockUpdateCount = blockPositions.size();
-        if (this.effectSystems != null)
+        // Possible that a client connected to a server, but is being transferred (BungeeCord)
+        if (this.effectSystems != null && GameUtils.isInGame())
             this.effectSystems.onBlockUpdates(blockPositions);
     }
 

src/main/java/org/orecruncher/dsurround/processing/Handlers.java

@@ -82,7 +82,7 @@ private void init() {
     private void onConnect(Minecraft client) {
         try {
             this.tasking.execute(() -> {
-                this.logger.info("Client connecting...");
+                this.logger.info("Handlers connecting...");
                 if (this.isConnected) {
                     this.logger.warn("Attempt to connect when already connected; disconnecting first");
                     this.onDisconnect(client);

src/main/java/org/orecruncher/dsurround/sound/SoundInstanceHandler.java

@@ -110,14 +110,19 @@ public static boolean inRange(final Vec3 listener, final SoundInstance sound, fi
         if (sound instanceof ElytraOnPlayerSoundInstance)
             return true;
 
+        // If for some reason the sound is at origin let it through. Some mods submit sound instances attached to a
+        // location, but do not initialize the location until it starts ticking.
+        if (sound.getX() == 0 && sound.getY() == 0 && sound.getZ() == 0)
+            return true;
+
         // Make sure a sound is assigned so that the volume check can work
         sound.resolve(GameUtils.getSoundManager());
 
         // If it is a loud sound, let it through
         if (sound.getVolume() > 1F)
             return true;
 
-        // Get the max distance of the sound range.  Pad is added because a player may move into hearing
+        // Get the max sound range. Pad is added because a player may move into hearing
         // range before the sound terminates.
         int distSq = sound.getSound().getAttenuationDistance() + pad;
         distSq *= distSq;

src/main/resources/assets/dsurround/dsconfigs/tags/blocks/effects/straw_step.json

@@ -2,6 +2,7 @@
   "replace": false,
   "values": [
     "minecraft:sugar_cane",
-    "minecraft:dead_bush"
+    "minecraft:dead_bush",
+    "minecraft:big_dripleaf_stem"
   ]
 }

src/main/resources/assets/dsurround/lang/en_us.json

@@ -80,7 +80,7 @@
   "dsurround.config.soundSystem.cullInterval": "Sound Culling Interval",
   "dsurround.config.soundSystem.cullInterval.tooltip": "Ticks between culled sound events (0 to disable culling)",
   "dsurround.config.soundSystem.enableSoundPruning": "Sound Pruning",
-  "dsurround.config.soundSystem.enableSoundPruning.tooltip": "Enables/disables cancellation of sound that a player will not hear. Does not apply to global, repeatable, or WEATHER sounds.",
+  "dsurround.config.soundSystem.enableSoundPruning.tooltip": "Enables/disables cancellation of sound that a player will not hear. Does not apply to global, non-attenuated, or WEATHER sounds.",
   "dsurround.config.soundOptions": "Sound Options",
   "dsurround.config.soundOptions.tooltip": "Options for defining how Dynamic Surroundings generate sound effects",
   "dsurround.config.soundOptions.ambientVolumeScaling": "Ambient sound scaling factor",

src/main/resources/dsurround.mixins.json

@@ -33,7 +33,6 @@
     "core.MixinRaycastContextAccessor",
     "core.MixinSoundOptionsScreen",
     "core.MixinWorld",
-    "events.MixinClientPacketListener",
     "events.MixinMinecraftClient"
   ],
   "server": [],