smol cleanup: don't register packs if their options failed to register; clean up holders, fluent accessors

api/src/main/java/org/geysermc/geyser/api/pack/option/UrlFallbackOption.java

@@ -32,6 +32,7 @@ import org.geysermc.geyser.api.GeyserApi;
  * When a Bedrock client is unable to download a resource pack from a URL, Geyser will, by default,
  * serve the resource pack over raknet (as packs are served with the {@link org.geysermc.geyser.api.pack.PathPackCodec}).
  * This option can be used to disable that behavior, and disconnect the player instead.
+ * By default, {@link UrlFallbackOption#TRUE} is set.
  */
 public interface UrlFallbackOption extends ResourcePackOption<Boolean> {
 

core/src/main/java/org/geysermc/geyser/event/type/GeyserDefineResourcePacksEventImpl.java

@@ -63,10 +63,8 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
         }
 
         ResourcePackHolder holder = ResourcePackHolder.of(pack);
-        packs.put(uuid, holder);
-
-        // register options
         registerOption(holder, options);
+        packs.put(uuid, holder);
         return true;
     }
 
@@ -75,12 +73,12 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
         Objects.requireNonNull(uuid);
         Objects.requireNonNull(options);
 
-        ResourcePackHolder packHolder = packs.get(uuid);
+        ResourcePackHolder holder = packs.get(uuid);
-        if (packHolder == null) {
+        if (holder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to register options");
         }
 
-        registerOption(packHolder, options);
+        registerOption(holder, options);
     }
 
     @Override
@@ -88,7 +86,7 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
         Objects.requireNonNull(uuid);
         ResourcePackHolder packHolder = packs.get(uuid);
         if (packHolder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to provide options");
         }
 
         return packHolder.optionHolder().immutableValues();
@@ -101,7 +99,7 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
 
         ResourcePackHolder packHolder = packs.get(uuid);
         if (packHolder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to provide option");
         }
 
         return packHolder.optionHolder().get(type);
@@ -117,8 +115,7 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
             return;
         }
 
-        holder.optionHolder().add(options);
+        holder.optionHolder().validateAndAdd(holder.pack(), options);
-        holder.optionHolder().validateOptions(holder.pack());
     }
 
     private GeyserResourcePack validate(@NonNull ResourcePack resourcePack) {
@@ -126,7 +123,7 @@ public class GeyserDefineResourcePacksEventImpl extends GeyserDefineResourcePack
         if (resourcePack instanceof GeyserResourcePack geyserResourcePack) {
             return geyserResourcePack;
         } else {
-            throw new IllegalArgumentException("Unknown resource pack implementation: %s".
+            throw new IllegalArgumentException("unknown resource pack implementation: %s".
                 formatted(resourcePack.getClass().getSuperclass().getName()));
         }
     }

core/src/main/java/org/geysermc/geyser/event/type/SessionLoadResourcePacksEventImpl.java

@@ -55,14 +55,24 @@ import java.util.UUID;
 
 public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksEvent {
 
+    /**
+     * The packs for this Session. A {@link ResourcePackHolder} may contain resource pack options registered
+     * during the {@link org.geysermc.geyser.api.event.lifecycle.GeyserDefineResourcePacksEvent}.
+     */
     @Getter
     private final Map<UUID, ResourcePackHolder> packs;
-    private final Map<UUID, OptionHolder> options;
+
+    /**
+     * The additional, per-session options for the resource packs of this session.
+     * These options are prioritized over the "default" options registered
+     * in the {@link org.geysermc.geyser.api.event.lifecycle.GeyserDefineResourcePacksEvent}
+     */
+    private final Map<UUID, OptionHolder> sessionPackOptionOverrides;
 
     public SessionLoadResourcePacksEventImpl(GeyserSession session) {
         super(session);
         this.packs = new Object2ObjectLinkedOpenHashMap<>(Registries.RESOURCE_PACKS.get());
-        this.options = new Object2ObjectOpenHashMap<>();
+        this.sessionPackOptionOverrides = new Object2ObjectOpenHashMap<>();
     }
 
     @Override
@@ -84,20 +94,18 @@ public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksE
             return false;
         }
 
+        registerOption(pack, options);
         packs.put(uuid, ResourcePackHolder.of(pack));
-
-        // register options
-        registerOption(resourcePack, options);
         return true;
     }
 
     @Override
     public void registerOptions(@NonNull UUID uuid, @NonNull ResourcePackOption<?>... options) {
-        Objects.requireNonNull(uuid);
+        Objects.requireNonNull(uuid, "uuid cannot be null");
-        Objects.requireNonNull(options);
+        Objects.requireNonNull(options, "options cannot be null");
         ResourcePackHolder holder = packs.get(uuid);
         if (holder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to register options");
         }
 
         registerOption(holder.pack(), options);
@@ -108,10 +116,16 @@ public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksE
         Objects.requireNonNull(uuid);
         ResourcePackHolder packHolder = packs.get(uuid);
         if (packHolder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to provide options");
+        }
+
+        OptionHolder optionHolder = sessionPackOptionOverrides.get(uuid);
+        if (optionHolder == null) {
+            // Not creating a new option holder here since it would
+            // override the default priority option
+            return packHolder.optionHolder().immutableValues();
         }
 
-        OptionHolder optionHolder = options.getOrDefault(uuid, new OptionHolder());
         return optionHolder.immutableValues(packHolder.optionHolder());
     }
 
@@ -122,32 +136,29 @@ public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksE
 
         ResourcePackHolder packHolder = packs.get(uuid);
         if (packHolder == null) {
-            throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options");
+            throw new IllegalArgumentException("resource pack with uuid " + uuid + " not found, unable to provide option");
         }
 
-        OptionHolder holder = options.get(uuid);
+        @Nullable OptionHolder additionalOptions = sessionPackOptionOverrides.get(uuid);
         OptionHolder defaultHolder = packHolder.optionHolder();
         Objects.requireNonNull(defaultHolder); // should never be null
 
-        return OptionHolder.getOptionByType(type, holder, defaultHolder);
+        return OptionHolder.optionByType(type, additionalOptions, defaultHolder);
     }
 
     @Override
     public boolean unregister(@NonNull UUID uuid) {
-        options.remove(uuid);
+        sessionPackOptionOverrides.remove(uuid);
         return packs.remove(uuid) != null;
     }
 
-    private void registerOption(@NonNull ResourcePack resourcePack, @Nullable ResourcePackOption<?>... options) {
+    private void registerOption(@NonNull GeyserResourcePack pack, @Nullable ResourcePackOption<?>... options) {
         if (options == null) {
             return;
         }
 
-        GeyserResourcePack pack = validate(resourcePack);
+        OptionHolder holder = this.sessionPackOptionOverrides.computeIfAbsent(pack.uuid(), $ -> new OptionHolder());
-
+        holder.validateAndAdd(pack, options);
-        OptionHolder holder = this.options.computeIfAbsent(pack.uuid(), $ -> new OptionHolder());
-        holder.add(options);
-        holder.validateOptions(pack);
     }
 
     // Methods used internally for e.g. ordered packs, or resource pack entries
@@ -190,10 +201,11 @@ public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksE
 
         for (ResourcePackHolder holder : this.packs.values()) {
             GeyserResourcePack pack = holder.pack();
-            ResourcePackManifest.Header header = pack.manifest().header();
             if (pack.codec() instanceof UrlPackCodec urlPackCodec) {
+                ResourcePackManifest.Header header = pack.manifest().header();
                 entries.add(new ResourcePacksInfoPacket.CDNEntry(
-                    header.uuid() + "_" + header.version(), urlPackCodec.url()));
+                    header.uuid() + "_" + header.version(), urlPackCodec.url())
+                );
             }
         }
 
@@ -202,20 +214,20 @@ public class SessionLoadResourcePacksEventImpl extends SessionLoadResourcePacksE
 
     // Helper methods to get the options for a ResourcePack
 
-    public <T> T getValue(UUID uuid, ResourcePackOption.Type type, T defaultValue) {
+    public <T> T value(UUID uuid, ResourcePackOption.Type type, T defaultValue) {
-        OptionHolder holder = options.get(uuid);
+        OptionHolder holder = sessionPackOptionOverrides.get(uuid);
         OptionHolder defaultHolder = packs.get(uuid).optionHolder();
         Objects.requireNonNull(defaultHolder); // should never be null
 
-        return OptionHolder.getWithFallbacks(type, holder, defaultHolder, defaultValue);
+        return OptionHolder.valueOrFallback(type, holder, defaultHolder, defaultValue);
     }
 
     private double priority(GeyserResourcePack pack) {
-        return getValue(pack.uuid(), ResourcePackOption.Type.PRIORITY, 5);
+        return value(pack.uuid(), ResourcePackOption.Type.PRIORITY, PriorityOption.NORMAL.value());
     }
 
     private String subpackName(GeyserResourcePack pack) {
-        return getValue(pack.uuid(), ResourcePackOption.Type.SUBPACK, "");
+        return value(pack.uuid(), ResourcePackOption.Type.SUBPACK, "");
     }
 
     // Helper method to validate a pack

core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java

@@ -316,7 +316,7 @@ public class UpstreamPacketHandler extends LoggingPacketHandler {
         if (codec instanceof UrlPackCodec urlPackCodec) {
             ResourcePackLoader.testRemotePack(session, urlPackCodec, packet.getPackId(), packet.getPackVersion());
 
-            if (!resourcePackLoadEvent.getValue(pack.uuid(), ResourcePackOption.Type.FALLBACK, true)) {
+            if (!resourcePackLoadEvent.value(pack.uuid(), ResourcePackOption.Type.FALLBACK, true)) {
                 session.disconnect("Unable to provide downloaded resource pack. Contact an administrator!");
                 return PacketSignal.HANDLED;
             }

core/src/main/java/org/geysermc/geyser/pack/ResourcePackHolder.java

@@ -27,6 +27,7 @@ package org.geysermc.geyser.pack;
 
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.geysermc.geyser.api.pack.ResourcePack;
+import org.geysermc.geyser.api.pack.option.PriorityOption;
 import org.geysermc.geyser.pack.option.OptionHolder;
 
 public record ResourcePackHolder(
@@ -35,7 +36,7 @@ public record ResourcePackHolder(
 ) {
 
     public static ResourcePackHolder of(GeyserResourcePack pack) {
-        return new ResourcePackHolder(pack, new OptionHolder());
+        return new ResourcePackHolder(pack, new OptionHolder(PriorityOption.NORMAL));
     }
 
     public ResourcePack resourcePack() {

core/src/main/java/org/geysermc/geyser/pack/option/OptionHolder.java

@@ -39,93 +39,90 @@ import java.util.Map;
 
 public class OptionHolder extends HashMap<ResourcePackOption.Type, ResourcePackOption<?>> {
 
-    public void add(ResourcePackOption<?> option) {
+    public OptionHolder() {
+        super();
+    }
+
+    // Used when adding resource packs initially to ensure that a priority option is always set
+    // It is however NOT used for session-options, as then the "normal" prio might override
+    // the resource pack option
+    public OptionHolder(PriorityOption option) {
+        super();
+        put(option.type(), option);
+    }
+
+    public void validateAndAdd(ResourcePack pack, ResourcePackOption<?>... options) {
+        for (ResourcePackOption<?> option : options) {
+            // Validate before adding
+            option.validate(pack);
+
+            // Ensure that we do not have duplicate types.
             if (super.containsKey(option.type())) {
                 super.replace(option.type(), option);
             } else {
                 super.put(option.type(), option);
             }
         }
-
-    public void add(ResourcePackOption<?>... options) {
-        for (ResourcePackOption<?> option : options) {
-            add(option);
-        }
     }
 
     @SuppressWarnings("unchecked")
-    public static <T> T getWithFallbacks(ResourcePackOption.@NonNull Type type,
+    public static <T> T valueOrFallback(ResourcePackOption.@NonNull Type type,
-                                         @Nullable OptionHolder holder,
+                                        @Nullable OptionHolder sessionPackOptions,
-                                         @NonNull OptionHolder defaultHolder,
+                                        @NonNull OptionHolder resourcePackOptions,
                                         @NonNull T defaultValue) {
         ResourcePackOption<?> option;
 
-        // First: the optionHolder's option, if it exists
+        // First: the session's options, if they exist
-        if (holder != null) {
+        if (sessionPackOptions != null) {
-            option = holder.get(type);
+            option = sessionPackOptions.get(type);
             if (option != null) {
-                return ((ResourcePackOption<T>) option).value();
+                return (T) option.value();
             }
         }
 
-        // Second: check the default optionHolder for the option, if it exists
+        // Second: check the resource pack options
-        option = defaultHolder.get(type);
+        option = resourcePackOptions.get(type);
         if (option != null) {
-            return ((ResourcePackOption<T>) option).value();
+            return (T) option.value();
         }
 
-        // Finally: Fallback to default
+        // Finally: return default
         return defaultValue;
     }
 
-    public static ResourcePackOption<?> getOptionByType(ResourcePackOption.@NonNull Type type,
+    public static @Nullable ResourcePackOption<?> optionByType(ResourcePackOption.@NonNull Type type,
-                                         @Nullable OptionHolder holder,
+                                                     @Nullable OptionHolder sessionPackOptions,
-                                         @NonNull OptionHolder defaultHolder) {
+                                                     @NonNull OptionHolder resourcePackOptions) {
-        ResourcePackOption<?> option;
 
-        // First: the optionHolder's option, if it exists
+        // First: the session-specific options, if these exist
-        if (holder != null) {
+        if (sessionPackOptions != null) {
-            option = holder.get(type);
+            ResourcePackOption<?> option = sessionPackOptions.get(type);
             if (option != null) {
                 return option;
             }
         }
 
-        // Second: check the default optionHolder for the option, if it exists;
+        // Second: check the default holder for the option, if it exists;
         // Or return null if the option isn't set.
-        option = defaultHolder.get(type);
+        return resourcePackOptions.get(type);
-        return option;
     }
 
     public void remove(ResourcePackOption<?> option) {
         super.remove(option.type());
     }
 
-    public OptionHolder() {
-        super();
-        add(PriorityOption.NORMAL);
-    }
-
-    public void validateOptions(ResourcePack pack) {
-        values().forEach(option -> option.validate(pack));
-    }
-
     /**
-     * @return the options of this option optionHolder in an immutable collection
+     * @return the options of this holder in an immutable collection
      */
     public Collection<ResourcePackOption<?>> immutableValues() {
         return Collections.unmodifiableCollection(values());
     }
 
     /**
-     * @return the options of this option optionHolder, with fallbacks to options of a {@link GeyserResourcePack}
+     * @return the options of this option holder, with fallbacks to options of a {@link GeyserResourcePack}
      * if they're not already overridden here
      */
     public Collection<ResourcePackOption<?>> immutableValues(OptionHolder defaultValues) {
-        if (defaultValues.isEmpty()) {
-            return immutableValues();
-        }
-
         // Create a map to hold the combined values
         Map<ResourcePackOption.Type, ResourcePackOption<?>> combinedOptions = new HashMap<>(this);