[SOLVED][1.19.4] How to remove Items from Vanilla creative tabs?

[JimiIT92]

I added some paintings to my mod and I want them to be displayed only in my custom creative tab. However the vanilla game puts them also in the Functional Blocks tab, because when that tab is created all painting variants are added.
Is there a way to prevent this?
So far I tried intercepting the CreativeModeTabEvent.BuildContents event and remove those item stacks from both dispalyItems and SearchTabDisplayItems, but they still appears in the vanilla tab
 

tab.getDisplayItems().removeIf(BLTabs::isEblPainting);
tab.getSearchTabDisplayItems().removeIf(BLTabs::isEblPainting);

private static boolean isEblPainting(ItemStack stack) {
  if(stack.is(Items.PAINTING) && stack.getTagElement("EntityTag") != null) {
    Optional<Holder<PaintingVariant>> paintingVariant = ForgeRegistries.PAINTING_VARIANTS.getHolder(ResourceLocation.tryParse(stack.getTagElement("EntityTag").getString("variant")));
            return paintingVariant.map(paintingVariantHolder -> paintingVariantHolder.containsTag(BLTags.Paintings.EBL_PAINTINGS)).orElse(false);
  }
  return false;
}

I'm sure the vent gets called because in the same event listener I add the painting variants to my own creative tab.

What I've noticed also is that when I search the painting and hover the item, only the vanilla tab is reported, despite the item being correctly placed into the mod tab as well

 

What am I missing here? I'm really new to this new tabs system

Edited March 30, 2023 by JimiIT92
solved

[warjort]

I don't think modifying the tab.getDisplayItems() will do anything?

That is what the event is currently building. It hasn't been populated yet.

 

I would guess the thing you want to modify is the event.getEntries() map which is what will be used to populate the real data.

 

[JimiIT92]

Ok, so I did this
 

if(tab.equals(CreativeModeTabs.FUNCTIONAL_BLOCKS)) {
  event.getEntries().forEach(stack -> {
    ItemStack itemStack = stack.getKey();
    if(isEblPainting(itemStack)) {
      event.getEntries().remove(itemStack);
    }
  });
}

to remove entries from the map if they met the criteria.
What I'm getting now is that the creative tab gets cut off at some point, with the last painting variant being one of the excluded ones

 

If I try to remove all paintings from the tab (so changing the condition to itemStack.is(Items.PAINTING) ) I get a similar result where the tab gets cut off and a single painting still remains as the last Item. This doesn't happen if I remove another Item (for example the Lightning Rod). I really have no clue why is doing this

[warjort]

You cannot usually iterate over a collection and remove elements from it at the same time unless you use the Iterator.remove() method (assuming it supports that method).

Your code likely corrupts the iterator's internal cursor. I'm suprised you are not getting ConcurrentModificationExceptions?

https://docs.oracle.com/javase/8/docs/api/java/util/Iterator.html#remove--

If an iterator does not support concurrent modification (most don't), the usual workaround is to make a copy of the keys collection and iterate over that.

Edited March 28, 2023 by warjort

[JimiIT92]

The entries iterator does not support the remove operation (you get an UnsupportedOperationException).
What I tried is storing the keys (the ItemStacks) I want to remove in a separate list and then iterate over that list to remove the stack from the entries
 

var iterator = event.getEntries().iterator();
ArrayList<ItemStack> itemsToRemove = new ArrayList<>();
while (iterator.hasNext()) {
  var entry = iterator.next();
  if(isEblPainting(entry.getKey())) {
    itemsToRemove.add(entry.getKey());
  }
}
itemsToRemove.forEach(stack -> event.getEntries().remove(stack));

However that seems to break the iterator too for some reason

[warjort]

This code kind of works for me:

@EventBusSubscriber(modid = MODID, bus = Bus.MOD)
public class Test {

    @SubscribeEvent
    public static void removePaintings(BuildContents event) {
        if (event.getTab() == CreativeModeTabs.FUNCTIONAL_BLOCKS) {
            var entries = event.getEntries();
            var paintings = new ArrayList<ItemStack>();
            for (var entry : entries) {
                var stack = entry.getKey();
                if (stack.is(Items.PAINTING)) {
                    paintings.add(stack);
                }
            }
            paintings.forEach(entries::remove);

            // DEBUG CODE FROM HERE
            LOG.info("=======> " + paintings);
            for (var entry : entries) {
                var stack = entry.getKey();
                if (stack.is(Items.PAINTING)) {
                    LOG.info(stack + " " + stack.getOrCreateTag());
                }
            }
        }
    }
}

 

But what I am seeing is the "kebab" painting is still in the list.

Quote

=======> [1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting, 1 painting]

1 painting {EntityTag:{variant:"minecraft:kebab"}}

All the other paintings are removed. So the above code should work for you?

 

I suspect the issue is something to do with "kebab" being the default painting variant which is somehow confusing the special key comparison mojang use.

ItemStackLinkedList.TYPE_AND_TAG

I would need to dig into how PaintingVariants get stored in the NBT and implement the equals/hashCode to understand what is happening. But that doesn't seem relevant to your problem?

Edited March 28, 2023 by warjort

[JimiIT92]

If I inspect the entries after my removal I get these results
 

1 painting {}
1 painting {EntityTag:{variant:"minecraft:kebab"}}
1 painting {EntityTag:{variant:"minecraft:aztec"}}
1 painting {EntityTag:{variant:"minecraft:alban"}}
1 painting {EntityTag:{variant:"minecraft:aztec2"}}
1 painting {EntityTag:{variant:"minecraft:bomb"}}
1 painting {EntityTag:{variant:"minecraft:plant"}}
1 painting {EntityTag:{variant:"minecraft:wasteland"}}
1 painting {EntityTag:{variant:"blazersmod:ebl_cheer"}}

with the last one being one of my mod's variants. I'm trying to dig into the remove function and a red flag I'm noticing is in MutableHashedLinkMap on line 249. There is a null check on entry.previous with a comment "Should never be null". What I've noticed is that the first time it executes this code the check passes (entry.previous is not null), however the second time it does fail (entry.previous is null).

Now I've temporarly removed my paintings from the "placeable" tag, which causes them to be added to the creative inventory, and inside the event I'm also removing every painting from the tab, so I'm effectively running the code you posted above and indee in game I only have the "kebab" painting which also breaks the rest of the tab.

Moving on with the remove, the first execution is trying to remove the default painting (the one with a random variant).
When entering the remove function, entry.previous corresponds to the Glow Item Frame, while entry.next corresponds to the "kebab" painting.
So entry.previous.next become the "kebab" painting while entry.previous become null (why setting the first value if we are setting the parent to null anyway?).
After that, on line 257, there is another check o nentry.next, which in this case is the "kebab" painting. Here something similar happens:
the entry.next.previous value is set to entry.previous, which we just set it to null, and entry.next is set to null (again, we are setting the parent to null anyway)
That concludes the first iteration.

On second iteration the code is trying to remove the "kebab" painting variant.
When hitting the remove function this happens:
entry.previous is now null, so the first check fails, but entry.next is not null (it is the "aztec" painting variant), so entry.next.previous is set to the value of entry.previous (which is null), while entry.next is set to null directly
This goes on for all consecutive variants, and I assume it also accounts for the modded ones.

Not sure if that's helpful since I'm not totally sure what's going on here, but maybe the first null set is what's causing the issue (which causes susequent checks to fail)

[warjort]

Yep, this totally broken.

I changed my code to print out in more detail what is happening at each step (warning inefficient code)

@EventBusSubscriber(modid = MODID, bus = Bus.MOD)
public class Test {

    @SubscribeEvent
    public static void removePaintings(BuildContents event) {
        if (event.getTab() == CreativeModeTabs.FUNCTIONAL_BLOCKS) {
            var entries = event.getEntries();
            var paintings = paintings(entries);

            for (var painting : paintings) {
                LOG.info(show(painting) + " >> " + show(paintings(entries)));
                entries.remove(painting);
                LOG.info(show(painting) + " << " + show(paintings(entries)));
            }
        }
    }

    public static String show(List<ItemStack> stacks) {
        return stacks.stream().map(Test::show).collect(Collectors.joining(",", "(", ")"));
    }
    
    public static String show(ItemStack stack) {
        return stack + " " + stack.getTag();
    }
    
    public static List<ItemStack> paintings(MutableHashedLinkedMap<ItemStack, ?> entries) {
        var paintings = new ArrayList<ItemStack>();
        for (var entry : entries) {
            var stack = entry.getKey();
            if (stack.is(Items.PAINTING)) {
                paintings.add(stack);
            }
        }
        return paintings;
    }
}

 

What actually happens when you remove the "kebab" painting is it removes everything else?

Quote

Removing the "null" painting works

1 painting null >> (1 painting null,1 painting {EntityTag:{variant:"minecraft:kebab"}},1 painting {EntityTag:{variant:"minecraft:aztec"}},1 painting {EntityTag:{variant:"minecraft:alban"}},1 painting {EntityTag:{variant:"minecraft:aztec2"}},1 painting {EntityTag:{variant:"minecraft:bomb"}},1 painting {EntityTag:{variant:"minecraft:plant"}},1 painting {EntityTag:{variant:"minecraft:wasteland"}},1 painting {EntityTag:{variant:"minecraft:wanderer"}},1 painting {EntityTag:{variant:"minecraft:graham"}},1 painting {EntityTag:{variant:"minecraft:pool"}},1 painting {EntityTag:{variant:"minecraft:courbet"}},1 painting {EntityTag:{variant:"minecraft:sea"}},1 painting {EntityTag:{variant:"minecraft:sunset"}},1 painting {EntityTag:{variant:"minecraft:creebet"}},1 painting {EntityTag:{variant:"minecraft:match"}},1 painting {EntityTag:{variant:"minecraft:bust"}},1 painting {EntityTag:{variant:"minecraft:stage"}},1 painting {EntityTag:{variant:"minecraft:void"}},1 painting {EntityTag:{variant:"minecraft:skull_and_roses"}},1 painting {EntityTag:{variant:"minecraft:wither"}},1 painting {EntityTag:{variant:"minecraft:fighters"}},1 painting {EntityTag:{variant:"minecraft:skeleton"}},1 painting {EntityTag:{variant:"minecraft:donkey_kong"}},1 painting {EntityTag:{variant:"minecraft:pointer"}},1 painting {EntityTag:{variant:"minecraft:pigscene"}},1 painting {EntityTag:{variant:"minecraft:burning_skull"}})

1 painting null << (1 painting {EntityTag:{variant:"minecraft:kebab"}},1 painting {EntityTag:{variant:"minecraft:aztec"}},1 painting {EntityTag:{variant:"minecraft:alban"}},1 painting {EntityTag:{variant:"minecraft:aztec2"}},1 painting {EntityTag:{variant:"minecraft:bomb"}},1 painting {EntityTag:{variant:"minecraft:plant"}},1 painting {EntityTag:{variant:"minecraft:wasteland"}},1 painting {EntityTag:{variant:"minecraft:wanderer"}},1 painting {EntityTag:{variant:"minecraft:graham"}},1 painting {EntityTag:{variant:"minecraft:pool"}},1 painting {EntityTag:{variant:"minecraft:courbet"}},1 painting {EntityTag:{variant:"minecraft:sea"}},1 painting {EntityTag:{variant:"minecraft:sunset"}},1 painting {EntityTag:{variant:"minecraft:creebet"}},1 painting {EntityTag:{variant:"minecraft:match"}},1 painting {EntityTag:{variant:"minecraft:bust"}},1 painting {EntityTag:{variant:"minecraft:stage"}},1 painting {EntityTag:{variant:"minecraft:void"}},1 painting {EntityTag:{variant:"minecraft:skull_and_roses"}},1 painting {EntityTag:{variant:"minecraft:wither"}},1 painting {EntityTag:{variant:"minecraft:fighters"}},1 painting {EntityTag:{variant:"minecraft:skeleton"}},1 painting {EntityTag:{variant:"minecraft:donkey_kong"}},1 painting {EntityTag:{variant:"minecraft:pointer"}},1 painting {EntityTag:{variant:"minecraft:pigscene"}},1 painting {EntityTag:{variant:"minecraft:burning_skull"}})

Removing the "kebab" painting does the opposite of what is suppossed to do.

1 painting {EntityTag:{variant:"minecraft:kebab"}} >> (1 painting {EntityTag:{variant:"minecraft:kebab"}},1 painting {EntityTag:{variant:"minecraft:aztec"}},1 painting {EntityTag:{variant:"minecraft:alban"}},1 painting {EntityTag:{variant:"minecraft:aztec2"}},1 painting {EntityTag:{variant:"minecraft:bomb"}},1 painting {EntityTag:{variant:"minecraft:plant"}},1 painting {EntityTag:{variant:"minecraft:wasteland"}},1 painting {EntityTag:{variant:"minecraft:wanderer"}},1 painting {EntityTag:{variant:"minecraft:graham"}},1 painting {EntityTag:{variant:"minecraft:pool"}},1 painting {EntityTag:{variant:"minecraft:courbet"}},1 painting {EntityTag:{variant:"minecraft:sea"}},1 painting {EntityTag:{variant:"minecraft:sunset"}},1 painting {EntityTag:{variant:"minecraft:creebet"}},1 painting {EntityTag:{variant:"minecraft:match"}},1 painting {EntityTag:{variant:"minecraft:bust"}},1 painting {EntityTag:{variant:"minecraft:stage"}},1 painting {EntityTag:{variant:"minecraft:void"}},1 painting {EntityTag:{variant:"minecraft:skull_and_roses"}},1 painting {EntityTag:{variant:"minecraft:wither"}},1 painting {EntityTag:{variant:"minecraft:fighters"}},1 painting {EntityTag:{variant:"minecraft:skeleton"}},1 painting {EntityTag:{variant:"minecraft:donkey_kong"}},1 painting {EntityTag:{variant:"minecraft:pointer"}},1 painting {EntityTag:{variant:"minecraft:pigscene"}},1 painting {EntityTag:{variant:"minecraft:burning_skull"}})
1 painting {EntityTag:{variant:"minecraft:kebab"}} << (1 painting {EntityTag:{variant:"minecraft:kebab"}})

 

The rest is irrelevant, the data is already broken.

1 painting {EntityTag:{variant:"minecraft:aztec"}} >> (1 painting {EntityTag:{variant:"minecraft:kebab"}})
1 painting {EntityTag:{variant:"minecraft:aztec"}} << (1 painting {EntityTag:{variant:"minecraft:kebab"}})

Something is broken with the remove() of that LinkedList implementation.

You can report it here: https://github.com/MinecraftForge/MinecraftForge/issues

[JimiIT92]

2 hours ago, warjort said:

You can report it here: https://github.com/MinecraftForge/MinecraftForge/issues

Just reported here

[JimiIT92]

Quick update on this. I tried to setup Forge locally to made the change myself, but ran into some issues described here (which I hope to fix, so any help would be much appreciated). So I did something different: I cloned the MutableHashedLinkedMap inside my project and then "clone" the vanilla map entries to my object. Inside my object I removed the null assignments and that seems to fix the issue. If I remove all paintings and then iterate over the leftover entries the breakpoint inside the check for a painting is never hit, which is what I expect. If I apply a filter to exclude only the modded variants, the breakpoint is hit only for the vanilla ones. So it seems that is just that null assignments on both previous and next that breaks the iterator.

This is how I debugged using my own local class
 

if(tab.equals(CreativeModeTabs.FUNCTIONAL_BLOCKS)) {

  org.blazers.MutableHashedLinkedMap<ItemStack, CreativeModeTab.TabVisibility> test = new org.blazers.MutableHashedLinkedMap<ItemStack, CreativeModeTab.TabVisibility>(ItemStackLinkedSet.TYPE_AND_TAG);
  event.getEntries().forEach(entry -> test.put(entry.getKey(), entry.getValue()));

  var iterator = test.iterator();
  ArrayList<ItemStack> itemsToRemove = new ArrayList<>();
  while (iterator.hasNext()) {
    ItemStack stack = iterator.next().getKey();
    if(stack.is(Items.PAINTING)) {
      itemsToRemove.add(stack);
    }
  }

  itemsToRemove.forEach(test::remove);

  var iterator2 = test.iterator();
  while (iterator2.hasNext()) {
    ItemStack stack = iterator2.next().getKey();
    if(stack.is(Items.PAINTING)) {
      LOG.info(stack + " " + stack.getOrCreateTag());
    }
  }
}

I'm submitting a PR now that I know that removing those null assignments fixes this, hopefully that's really what it needs to solve it

[JimiIT92]

Marking this as solved, the issue has been fixed (tested with the current latest build 45.0.39, but I think it has been fixed since 45.0.30)