From 3bf5da14764717afb5feb372983752522f049a2d Mon Sep 17 00:00:00 2001 From: onebeastchris Date: Wed, 18 Sep 2024 20:55:19 +0800 Subject: [PATCH] smol cleanup: don't register packs if their options failed to register; clean up holders, fluent accessors --- .../api/pack/option/UrlFallbackOption.java | 1 + .../GeyserDefineResourcePacksEventImpl.java | 21 ++--- .../SessionLoadResourcePacksEventImpl.java | 66 ++++++++------ .../geyser/network/UpstreamPacketHandler.java | 2 +- .../geyser/pack/ResourcePackHolder.java | 3 +- .../geyser/pack/option/OptionHolder.java | 87 +++++++++---------- 6 files changed, 94 insertions(+), 86 deletions(-) diff --git a/api/src/main/java/org/geysermc/geyser/api/pack/option/UrlFallbackOption.java b/api/src/main/java/org/geysermc/geyser/api/pack/option/UrlFallbackOption.java index 49e67f39f..37268a59b 100644 --- a/api/src/main/java/org/geysermc/geyser/api/pack/option/UrlFallbackOption.java +++ b/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 { diff --git a/core/src/main/java/org/geysermc/geyser/event/type/GeyserDefineResourcePacksEventImpl.java b/core/src/main/java/org/geysermc/geyser/event/type/GeyserDefineResourcePacksEventImpl.java index 5429767ea..3ddf66e4a 100644 --- a/core/src/main/java/org/geysermc/geyser/event/type/GeyserDefineResourcePacksEventImpl.java +++ b/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); - if (packHolder == null) { - throw new IllegalArgumentException("ResourcePack with " + uuid + " not found, unable to provide options"); + ResourcePackHolder holder = packs.get(uuid); + if (holder == null) { + 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().validateOptions(holder.pack()); + holder.optionHolder().validateAndAdd(holder.pack(), options); } 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())); } } diff --git a/core/src/main/java/org/geysermc/geyser/event/type/SessionLoadResourcePacksEventImpl.java b/core/src/main/java/org/geysermc/geyser/event/type/SessionLoadResourcePacksEventImpl.java index c13bbde92..49c5b01e2 100644 --- a/core/src/main/java/org/geysermc/geyser/event/type/SessionLoadResourcePacksEventImpl.java +++ b/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 packs; - private final Map 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 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(options); + Objects.requireNonNull(uuid, "uuid cannot be null"); + 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.options.computeIfAbsent(pack.uuid(), $ -> new OptionHolder()); - holder.add(options); - holder.validateOptions(pack); + OptionHolder holder = this.sessionPackOptionOverrides.computeIfAbsent(pack.uuid(), $ -> new OptionHolder()); + holder.validateAndAdd(pack, options); } // 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 getValue(UUID uuid, ResourcePackOption.Type type, T defaultValue) { - OptionHolder holder = options.get(uuid); + public T value(UUID uuid, ResourcePackOption.Type type, T defaultValue) { + 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 diff --git a/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java b/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java index e40bbbc7d..f526ad9cb 100644 --- a/core/src/main/java/org/geysermc/geyser/network/UpstreamPacketHandler.java +++ b/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; } diff --git a/core/src/main/java/org/geysermc/geyser/pack/ResourcePackHolder.java b/core/src/main/java/org/geysermc/geyser/pack/ResourcePackHolder.java index 2eb7e5b97..f475127e7 100644 --- a/core/src/main/java/org/geysermc/geyser/pack/ResourcePackHolder.java +++ b/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() { diff --git a/core/src/main/java/org/geysermc/geyser/pack/option/OptionHolder.java b/core/src/main/java/org/geysermc/geyser/pack/option/OptionHolder.java index 958ab9413..f6ec741df 100644 --- a/core/src/main/java/org/geysermc/geyser/pack/option/OptionHolder.java +++ b/core/src/main/java/org/geysermc/geyser/pack/option/OptionHolder.java @@ -39,93 +39,90 @@ import java.util.Map; public class OptionHolder extends HashMap> { - public void add(ResourcePackOption option) { - if (super.containsKey(option.type())) { - super.replace(option.type(), option); - } else { - super.put(option.type(), option); - } + public OptionHolder() { + super(); } - public void add(ResourcePackOption... options) { + // 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) { - add(option); + // 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); + } } } @SuppressWarnings("unchecked") - public static T getWithFallbacks(ResourcePackOption.@NonNull Type type, - @Nullable OptionHolder holder, - @NonNull OptionHolder defaultHolder, - @NonNull T defaultValue) { + public static T valueOrFallback(ResourcePackOption.@NonNull Type type, + @Nullable OptionHolder sessionPackOptions, + @NonNull OptionHolder resourcePackOptions, + @NonNull T defaultValue) { ResourcePackOption option; - // First: the optionHolder's option, if it exists - if (holder != null) { - option = holder.get(type); + // First: the session's options, if they exist + if (sessionPackOptions != null) { + option = sessionPackOptions.get(type); if (option != null) { - return ((ResourcePackOption) option).value(); + return (T) option.value(); } } - // Second: check the default optionHolder for the option, if it exists - option = defaultHolder.get(type); + // Second: check the resource pack options + option = resourcePackOptions.get(type); if (option != null) { - return ((ResourcePackOption) option).value(); + return (T) option.value(); } - // Finally: Fallback to default + // Finally: return default return defaultValue; } - public static ResourcePackOption getOptionByType(ResourcePackOption.@NonNull Type type, - @Nullable OptionHolder holder, - @NonNull OptionHolder defaultHolder) { - ResourcePackOption option; + public static @Nullable ResourcePackOption optionByType(ResourcePackOption.@NonNull Type type, + @Nullable OptionHolder sessionPackOptions, + @NonNull OptionHolder resourcePackOptions) { - // First: the optionHolder's option, if it exists - if (holder != null) { - option = holder.get(type); + // First: the session-specific options, if these exist + if (sessionPackOptions != null) { + 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 option; + return resourcePackOptions.get(type); } 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> 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> immutableValues(OptionHolder defaultValues) { - if (defaultValues.isEmpty()) { - return immutableValues(); - } - // Create a map to hold the combined values Map> combinedOptions = new HashMap<>(this);