Redesign CreativeTab collection event to be a lot more straight forward. (#9198)

src/main/java/net/minecraftforge/client/ForgeHooksClient.java

@@ -6,10 +6,6 @@
 package net.minecraftforge.client;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Maps;
-import com.google.common.graph.ElementOrder;
-import com.google.common.graph.GraphBuilder;
-import com.google.common.graph.MutableGraph;
 import com.mojang.blaze3d.platform.NativeImage;
 import com.mojang.blaze3d.platform.Window;
 import com.mojang.blaze3d.shaders.FogShape;
@@ -74,7 +70,6 @@ import net.minecraft.client.sounds.SoundEngine;
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
 import net.minecraft.locale.Language;
-import net.minecraft.nbt.CompoundTag;
 import net.minecraft.network.Connection;
 import net.minecraft.network.chat.ChatType;
 import net.minecraft.network.chat.CommonComponents;
@@ -94,9 +89,11 @@ import net.minecraft.world.entity.HumanoidArm;
 import net.minecraft.world.entity.LivingEntity;
 import net.minecraft.world.entity.ai.attributes.AttributeInstance;
 import net.minecraft.world.entity.player.Player;
+import net.minecraft.world.flag.FeatureFlagSet;
 import net.minecraft.world.inventory.tooltip.TooltipComponent;
 import net.minecraft.world.item.CreativeModeTab;
 import net.minecraft.world.item.ItemStack;
+import net.minecraft.world.item.ItemStackLinkedSet;
 import net.minecraft.world.item.crafting.RecipeManager;
 import net.minecraft.world.level.BlockAndTintGetter;
 import net.minecraft.world.level.GameType;
@@ -144,7 +141,7 @@ import net.minecraftforge.client.textures.ForgeTextureMetadata;
 import net.minecraftforge.common.ForgeI18n;
 import net.minecraftforge.common.ForgeMod;
 import net.minecraftforge.common.MinecraftForge;
-import net.minecraftforge.common.util.ItemStackMap;
+import net.minecraftforge.common.util.MutableHashedLinkedMap;
 import net.minecraftforge.event.CreativeModeTabEvent;
 import net.minecraftforge.eventbus.api.Event;
 import net.minecraftforge.eventbus.api.SubscribeEvent;
@@ -153,8 +150,6 @@ import net.minecraftforge.fml.ModList;
 import net.minecraftforge.fml.ModLoader;
 import net.minecraftforge.fml.VersionChecker;
 import net.minecraftforge.fml.common.Mod;
-import net.minecraftforge.fml.loading.toposort.CyclePresentException;
-import net.minecraftforge.fml.loading.toposort.TopologicalSort;
 import net.minecraftforge.network.NetworkConstants;
 import net.minecraftforge.network.NetworkRegistry;
 import net.minecraftforge.registries.GameData;
@@ -176,7 +171,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -1189,106 +1183,26 @@ public class ForgeHooksClient
         return new ResourceLocation(loc.getNamespace(), normalised);
     }
 
-    public static CreativeModeTab.DisplayItemsGenerator onCreativeModeTabBuildContents(CreativeModeTab tab, CreativeModeTab.DisplayItemsGenerator originalGenerator) {
-        Set<CreativeModeTabEvent.DisplayItemsAdapter> customGenerators = new HashSet<>();
-        CreativeModeTabEvent.BuildContents event = ModLoader.get().postEventWithReturn(new net.minecraftforge.event.CreativeModeTabEvent.BuildContents(tab, customGenerators));
-
-        record ModificationEntry(CreativeModeTab.TabVisibility tabVisibility, ItemStack before, ItemStack after) {}
-
-        return (featureFlagSet, output, hasPermissions) -> {
-            final Map<ItemStack, CreativeModeTab.TabVisibility> originalEntries = ItemStackMap.createTypeAndTagLinkedMap();
-            final Map<ItemStack, ModificationEntry> entries = ItemStackMap.createTypeAndTagMap();
-
-            originalGenerator.accept(featureFlagSet, originalEntries::put, hasPermissions);
-            customGenerators.forEach(generator -> generator.accept(featureFlagSet, (stack, visibility, before, after) -> {
-                if (stack.getCount() != 1)
-                    throw new IllegalArgumentException("The stack count must be 1");
-
-                entries.put(stack, new ModificationEntry(visibility, before, after));
-            }, hasPermissions));
-
-            // We switch to the stacks tag here, as we need to be able to compare and hash them for the graph!
-            // We could not handle this with an open hashset since the builder does not support this internally.
-            final MutableGraph<CompoundTag> graph = GraphBuilder.directed().nodeOrder(ElementOrder.insertion()).build();
-            final Map<CompoundTag, ItemStack> tagToStack = Maps.newHashMap();
-
-            CompoundTag lastStackTag = null;
-            for (ItemStack stack : originalEntries.keySet())
-            {
-                final CompoundTag tag = stack.serializeNBT();
-                tagToStack.put(tag, stack);
-                if (!stack.isEmpty())
-                {
-                    graph.addNode(tag);
-                }
-                if (lastStackTag != null)
-                {
-                    graph.putEdge(lastStackTag, tag);
-                }
-                lastStackTag = tag;
+    public static void onCreativeModeTabBuildContents(CreativeModeTab tab, CreativeModeTab.DisplayItemsGenerator originalGenerator, FeatureFlagSet flags, CreativeModeTab.Output output, boolean hasPermisions)
+    {
+        final var entries = new MutableHashedLinkedMap<ItemStack, CreativeModeTab.TabVisibility>(ItemStackLinkedSet.TYPE_AND_TAG,
+            (key, left, right) -> {
+                //throw new IllegalStateException("Accidentally adding the same item stack twice " + key.getDisplayName().getString() + " to a Creative Mode Tab: " + tab.getDisplayName().getString());
+                // Vanilla adds enchanting books twice in both visibilities.
+                // This is just code cleanliness for them. For us lets just increase the visibility and merge the entries.
+                return CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS;
             }
+        );
 
-            CompoundTag finalLastStackTag = lastStackTag;
-            entries.forEach((stack, modificationEntry) -> {
-                final CompoundTag tag = stack.serializeNBT();
-                if (!stack.isEmpty())
-                {
-                    graph.addNode(tag);
-                    tagToStack.put(tag, stack);
-                }
-                if (!modificationEntry.after.isEmpty())
-                {
-                    final CompoundTag afterTag = modificationEntry.after.serializeNBT();
-                    graph.putEdge(afterTag, tag);
-                    tagToStack.put(afterTag, modificationEntry.after);
-                }
-                else if (finalLastStackTag != null)
-                {
-                    // If the user does not specify an after, we assume it is the last stack and put it after that.
-                    graph.putEdge(finalLastStackTag, tag);
-                }
-                if (!modificationEntry.before.isEmpty())
-                {
-                    final CompoundTag beforeTag = modificationEntry.before.serializeNBT();
-                    graph.putEdge(tag, beforeTag);
-                    tagToStack.put(beforeTag, modificationEntry.before);
-                }
-            });
+        originalGenerator.accept(flags, (stack, vis) -> {
+            if (stack.getCount() != 1)
+                throw new IllegalArgumentException("The stack count must be 1");
+            entries.put(stack, vis);
+        }, hasPermisions);
 
-            try
-            {
-                List<CompoundTag> tierList = TopologicalSort.topologicalSort(graph, null);
+        ModLoader.get().postEvent(new CreativeModeTabEvent.BuildContents(tab, flags, hasPermisions, entries));
 
-                // Now we unwind the tags again so that they are sorted neatly.
-                for (CompoundTag tag : tierList)
-                {
-                    final ItemStack stack = tagToStack.get(tag);
-                    if (originalEntries.containsKey(stack))
-                    {
-                        output.accept(stack, originalEntries.get(stack));
-                    }
-                    else if (entries.containsKey(stack))
-                    {
-                        output.accept(stack, entries.get(stack).tabVisibility);
-                    }
-                }
-            }
-            catch (CyclePresentException cyclePresentException)
-            {
-                LOGGER.error("Failed to inject custom items into creative mode tab {} due to a cycle in the dependency graph", tab, cyclePresentException);
-                LOGGER.warn("Detected cycles:");
-                for (Set<Object> cycle : cyclePresentException.getCycles())
-                {
-                    graph.edges().forEach(edge -> {
-                        if (cycle.contains(edge.nodeU()) && cycle.contains(edge.nodeV()))
-                        {
-                            LOGGER.warn(" - {} -> {}", tagToStack.get(edge.nodeU()), tagToStack.get(edge.nodeV()));
-                        }
-                    });
-                }
-                LOGGER.error("Falling back to default!");
-                originalGenerator.accept(featureFlagSet, output, hasPermissions);
-            }
-        };
+        for (var entry : entries)
+            output.accept(entry.getKey(), entry.getValue());
     }
 }

src/main/java/net/minecraftforge/common/util/MutableHashedLinkedMap.java

@@ -0,0 +1,317 @@
+/*
+ * Copyright (c) Forge Development LLC and contributors
+ * SPDX-License-Identifier: LGPL-2.1-only
+ */
+
+package net.minecraftforge.common.util;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.NoSuchElementException;
+
+import org.jetbrains.annotations.Nullable;
+
+import it.unimi.dsi.fastutil.Hash.Strategy;
+import it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap;
+
+/**
+ * A mutable linked map with a hashing strategy and a merge function.
+ *
+ * @param <K> the type of keys
+ * @param <V> the type of mapped values
+ */
+public class MutableHashedLinkedMap<K, V> implements Iterable<Map.Entry<K, V>>
+{
+    private final Strategy<? super K> strategy;
+    private final Map<K, Entry> entries;
+    private final MergeFunction<K, V> merge;
+    private Entry head = null;
+    private Entry last = null;
+
+    /**
+     * Creates a mutable linked map with a default new-value-selecting merge function.
+     *
+     * @param strategy the hashing strategy
+     */
+    public MutableHashedLinkedMap(Strategy<? super K> strategy)
+    {
+        this(strategy, (k, v1, v2) -> v2);
+    }
+
+    /**
+     * Creates a mutable linked map with a custom merge function.
+     *
+     * @param strategy the hashing strategy
+     * @param merge the function used when merging an existing value and a new value
+     */
+    public MutableHashedLinkedMap(Strategy<? super K> strategy, MergeFunction<K, V> merge)
+    {
+        this.strategy = strategy;
+        this.entries = new Object2ObjectOpenCustomHashMap<>(strategy);
+        this.merge = merge;
+    }
+
+    /**
+     * Inserts the mapping with the specified key and value pair.
+     *
+     * <p>If there is a mapping already associated with this key, then the previous value and the specified (new) value
+     * are merged according to this collection's merge function, and the position of the entry is not modified. If there
+     * is no such mapping, then the key-value mapping is inserted at the end of this collection.</p>
+     *
+     * @param key key to be inserted
+     * @param value (new) value to be associated with the key
+     * @return the previous value associated with the specified key, or {@code null} if there was no mapping for the key
+     */
+    @Nullable
+    public V put(K key, V value)
+    {
+        var old = entries.get(key);
+        if (old != null)
+        {
+            V ret = old.value;
+            old.value = merge.apply(key, ret, value);
+            return ret;
+        }
+
+        var self = new Entry(key, value);
+
+        var l = last;
+        self.previous = l;
+        if (l == null)
+            head = self;
+        else
+            l.next = self;
+        last = self;
+
+        entries.put(key, self);
+        return null;
+    }
+
+    public boolean contains(K key) { return this.entries.containsKey(key); }
+    public boolean isEmpty() { return this.entries.isEmpty(); }
+
+    @Nullable
+    public V remove(K key)
+    {
+        var ret = this.entries.remove(key);
+        if (ret == null)
+            return null;
+
+        remove(ret);
+        return ret.getValue();
+    }
+
+    @Nullable
+    public V get(K key)
+    {
+        var entry = entries.get(key);
+        return entry == null ? null : entry.getValue();
+    }
+
+    @Override
+    public Iterator<Map.Entry<K, V>> iterator()
+    {
+        return new Iterator<>()
+        {
+            private Entry current = head;
+            @Override public boolean hasNext() { return current != null; }
+            @Override
+            public Map.Entry<K, V> next()
+            {
+                if (!hasNext())
+                    throw new NoSuchElementException();
+                var ret = current;
+                current = current.next;
+                return ret;
+            }
+        };
+    }
+
+    /**
+     * Inserts the mapping with the specified key and value pair at the beginning of this map.
+     *
+     * <p>If there is a mapping already associated with this key, then the previous value and the specified (new) value
+     * are first merged according to this map's merge function, then the entry is moved to the beginning of the map.</p>
+     *
+     * @param key key to be inserted at the beginning
+     * @param value (new) value to be associated with the key
+     * @return the previous value associated with the specified key, or {@code null} if there was no mapping for the key
+     *
+     * @see #putBefore(Object, Object, Object)
+     */
+    @Nullable
+    public V putFirst(K key, V value)
+    {
+        if (head != null)
+            return putBefore(head.getKey(), key, value);
+        return put(key, value);
+    }
+
+    /**
+     * Inserts the mapping with this key and value pair immediately after the entry with the specified positioning key.
+     *
+     * <p>If the specified positioning key is not present within this map, then this method behaves like
+     * {@link #put(Object, Object)}. If there is a mapping already associated with this key, then the previous value
+     * and the specified (new) value are first merged according to this map's merge function, then the entry is
+     * moved to directly after the entry with the specified positioning key.</p>
+     *
+     * @param after the key to position this new entry afterwards
+     * @param key key to be inserted at the beginning
+     * @param value (new) value to be associated with the key
+     * @return the previous value associated with the specified key, or {@code null} if there was no mapping for the key
+     *
+     * @see #putBefore(Object, Object, Object)
+     */
+    @Nullable
+    public V putAfter(K after, K key, V value)
+    {
+        var target = entries.get(after);
+        if (target == null)
+            return put(key, value);
+
+        V ret = null;
+        var entry = entries.get(key);
+        if (entry != null)
+        {
+            ret = entry.value;
+            entry.value = merge.apply(key, ret, value);
+            remove(entry);
+        }
+        else
+        {
+            entry = new Entry(key, value);
+            entries.put(key, entry);
+        }
+
+        entry.previous = target;
+        if (target.next == null)
+            last = target;
+        else
+            target.next.previous = entry;
+        entry.next = target.next;
+
+        target.next = entry;
+        return ret;
+    }
+
+    /**
+     * Inserts the mapping with this key and value pair immediately before the entry with the specified positioning key.
+     *
+     * <p>If the specified positioning key is not present within this map, then this method behaves like
+     * {@link #put(Object, Object)}. If there is a mapping already associated with this key, then the previous value
+     * and the specified (new) value are first merged according to this map's merge function, then the entry is
+     * moved to directly before the entry with the specified positioning key.</p>
+     *
+     * @param before the key to position this new entry afterwards
+     * @param key key to be inserted at the beginning
+     * @param value (new) value to be associated with the key
+     * @return the previous value associated with the specified key, or {@code null} if there was no mapping for the key
+     *
+     * @see #putAfter(Object, Object, Object)
+     */
+    @Nullable
+    public V putBefore(K before, K key, V value)
+    {
+        var target = entries.get(before);
+        if (target == null)
+            return put(key, value);
+
+        V ret = null;
+        var entry = entries.get(key);
+        if (entry != null)
+        {
+            ret = entry.value;
+            entry.value = merge.apply(key, ret, value);
+            remove(entry);
+        }
+        else
+        {
+            entry = new Entry(key, value);
+            entries.put(key, entry);
+        }
+
+        entry.previous = target.previous;
+        if (target.previous == null)
+            head = entry;
+        else
+            target.previous.next = entry;
+        entry.next = target;
+
+        target.previous = entry;
+
+        return ret;
+    }
+
+    private void remove(Entry e)
+    {
+        if (head == null)
+            head = e.next;
+        else if (e.previous != null) // Should never be null, but just in case.
+        {
+            e.previous.next = e.next;
+            e.previous = null;
+        }
+
+        if (last == e)
+            last = e.previous;
+        else if (e.next != null) // Should never be null, but just in case.
+        {
+            e.next.previous = e.previous;
+            e.next = null;
+        }
+    }
+
+    public static interface MergeFunction<Key, Value>
+    {
+        Value apply(Key key, Value left, Value right);
+    }
+
+    private class Entry implements Map.Entry<K, V>
+    {
+        private final K key;
+        private V value;
+
+        private Entry previous;
+        private Entry next;
+
+        private Entry(K key, V value)
+        {
+            this.key = key;
+            this.value = value;
+        }
+
+        @Override public K getKey() { return this.key; }
+        @Override public V getValue() { return this.value; }
+        @Override
+        public V setValue(V value)
+        {
+            var old = this.value;
+            this.value = value;
+            return old;
+        }
+
+        @Override
+        public String toString()
+        {
+            return "Entry[" + this.key + ", " + this.value + "]";
+        }
+
+        @Override
+        public boolean equals(Object o)
+        {
+            if (!(o instanceof Map.Entry))
+                return false;
+
+            Map.Entry<?,?> e = (Map.Entry<?, ?>)o;
+            return (key == null ? e.getKey() == null : key.equals(e.getKey())) &&
+                   (value == null ? e.getValue() == null : value.equals(e.getValue()));
+        }
+
+        @Override
+        public int hashCode()
+        {
+            return (key == null ? 0 : strategy.hashCode(key)) ^
+                   (value == null ? 0 : value.hashCode());
+        }
+    }
+}

src/main/java/net/minecraftforge/event/CreativeModeTabEvent.java

@@ -7,15 +7,18 @@ package net.minecraftforge.event;
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Set;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
+
 import net.minecraft.resources.ResourceLocation;
 import net.minecraft.world.flag.FeatureFlagSet;
 import net.minecraft.world.item.CreativeModeTab;
+import net.minecraft.world.item.CreativeModeTab.Output;
+import net.minecraft.world.item.CreativeModeTab.TabVisibility;
 import net.minecraft.world.item.CreativeModeTabs;
 import net.minecraft.world.item.ItemStack;
 import net.minecraft.world.level.ItemLike;
-import net.minecraftforge.common.CreativeModeTabRegistry;
+import net.minecraftforge.common.util.MutableHashedLinkedMap;
 import net.minecraftforge.eventbus.api.Cancelable;
 import net.minecraftforge.eventbus.api.Event;
 import net.minecraftforge.fml.LogicalSide;
@@ -106,58 +109,20 @@ public class CreativeModeTabEvent extends Event implements IModBusEvent
      * This event is fired on the {@linkplain FMLJavaModLoadingContext#getModEventBus() mod-specific event bus},
      * only on the {@linkplain LogicalSide#CLIENT logical client}.
      */
-    public static final class BuildContents extends CreativeModeTabEvent
+    public static final class BuildContents extends CreativeModeTabEvent implements Output
     {
         private final CreativeModeTab tab;
-        private final Set<DisplayItemsAdapter> customGenerators;
+        private final FeatureFlagSet flags;
+        private final boolean hasPermissions;
+        private final MutableHashedLinkedMap<ItemStack, TabVisibility> entries;
 
         @ApiStatus.Internal
-        public BuildContents(CreativeModeTab tab, Set<DisplayItemsAdapter> customGenerators)
+        public BuildContents(CreativeModeTab tab, FeatureFlagSet flags, boolean hasPermissions, MutableHashedLinkedMap<ItemStack, TabVisibility> entries)
         {
             this.tab = tab;
-            this.customGenerators = customGenerators;
-        }
-
-        /**
-         * Registers the given generator.
-         */
-        public void register(DisplayItemsAdapter generator)
-        {
-            this.customGenerators.add(generator);
-        }
-
-        /**
-         * Registers the given generator if the provided creative mode tab matches this event's currently populating tab.
-         */
-        public void register(CreativeModeTab tab, DisplayItemsAdapter generator)
-        {
-            if (this.tab == tab)
-                this.customGenerators.add(generator);
-        }
-
-        /**
-         * Registers the given generator if the provided tab name matches this event's currently populating tab's name.
-         */
-        public void register(ResourceLocation tabName, DisplayItemsAdapter generator)
-        {
-            if (this.tab == CreativeModeTabRegistry.getTab(tabName))
-                this.customGenerators.add(generator);
-        }
-
-        /**
-         * Adds the given item if the provided creative mode tab matches this event's currently populating tab.
-         */
-        public void registerSimple(CreativeModeTab tab, ItemLike item)
-        {
-            this.register(tab, (enabledFlags, output, permissions) -> output.accept(item));
-        }
-
-        /**
-         * Adds the given items if the provided creative mode tab matches this event's currently populating tab.
-         */
-        public void registerSimple(CreativeModeTab tab, ItemLike... items)
-        {
-            this.register(tab, (enabledFlags, output, permissions) -> output.acceptAll(items));
+            this.flags = flags;
+            this.hasPermissions = hasPermissions;
+            this.entries = entries;
         }
 
         /**
@@ -167,70 +132,36 @@ public class CreativeModeTabEvent extends Event implements IModBusEvent
         {
             return this.tab;
         }
-    }
 
-    @FunctionalInterface
-    public interface DisplayItemsAdapter
-    {
-        void accept(FeatureFlagSet enabledFlags, CreativeModeTabPopulator populator, boolean hasPermissions);
-    }
-
-    @FunctionalInterface
-    public interface CreativeModeTabPopulator
-    {
-        void accept(ItemStack stack, CreativeModeTab.TabVisibility visibility, ItemStack before, ItemStack after);
-
-        default void accept(ItemStack stack, ItemStack before, ItemStack after)
+        public FeatureFlagSet getFlags()
         {
-            accept(stack, CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS, before, after);
+            return this.flags;
         }
 
-        default void accept(ItemLike stack, CreativeModeTab.TabVisibility visibility, ItemStack before, ItemStack after)
+        public boolean hasPermissions()
         {
-            accept(new ItemStack(stack), visibility, before, after);
+            return this.hasPermissions;
         }
 
-        default void accept(ItemLike stack, ItemStack before, ItemStack after)
+        public MutableHashedLinkedMap<ItemStack, TabVisibility> getEntries()
         {
-            accept(stack, CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS, before, after);
+            return this.entries;
         }
 
-        default void accept(ItemStack stack, CreativeModeTab.TabVisibility visibility)
+        @Override
+        public void accept(ItemStack stack, TabVisibility visibility)
         {
-            accept(stack, visibility, ItemStack.EMPTY, ItemStack.EMPTY);
+            getEntries().put(stack, visibility);
         }
 
-        default void accept(ItemStack stack)
+        public void accept(Supplier<? extends ItemLike> item, CreativeModeTab.TabVisibility visibility)
         {
-            this.accept(stack, CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS);
+           this.accept(item.get(), visibility);
         }
 
-        default void accept(ItemLike itemLike, CreativeModeTab.TabVisibility visibility)
+        public void accept(Supplier<? extends ItemLike> item)
         {
-            this.accept(new ItemStack(itemLike), visibility);
-        }
-
-        default void accept(ItemLike itemLike)
-        {
-            this.accept(new ItemStack(itemLike), CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS);
-        }
-
-        default void acceptAll(Collection<ItemStack> stacks, CreativeModeTab.TabVisibility visibility)
-        {
-            stacks.forEach(stack -> this.accept(stack, visibility));
-        }
-
-        default void acceptAll(Collection<ItemStack> stacks)
-        {
-            this.acceptAll(stacks, CreativeModeTab.TabVisibility.PARENT_AND_SEARCH_TABS);
-        }
-
-        default void acceptAll(ItemLike... items)
-        {
-            for (ItemLike item : items)
-            {
-                this.accept(item);
-            }
+           this.accept(item.get());
         }
     }
 }