ItemStack remains visible after being removed from logic

[FaMa91]

What is happening?

Initially, the GUI looks like this — you can see a green book in the third row, and the first row is empty:

When I click on the green book, it moves to the first row and is removed both from the GUI and from the player’s inventory:

Clicking the green book in the first row should give it back to the player and move it to the third row again, returning the GUI to its original state.

The logic behind this is slightly more complex, the third row is dynamically built from the player’s inventory, which is why the green book appears and disappears from that row.

What did you expect to happen?

When I click the green book in the first row, the underlying logic executes correctly, logs confirm that the Set used to populate the first row is properly cleared.

However, the GUI is then redrawn with the green book still visible in the first row. Clicking it again keeps giving new green books to the player’s inventory, even though the internal data structure is empty:

Version

1.6.5-SNAPSHOT

Config

setup:
  - "  12345  "
  - "         "
  - " iiiiiii "
  - " iiiiiii "
  - "         "

Server/system Version

Paper 1.21.1

Log

This is the output of the .size() on the set before and after having applied the logic.
[21:39:02 INFO]: [TEST-core] SIZE:1
[21:39:02 INFO]: [TEST-core] SIZE:0

What other programs/plugins are you running?

I tried with no other plugins

Additional context

This is how I'm creating the GUI's elements:

GuiElementGroup group = new GuiElementGroup('i');
HashMap<Integer, ? extends ItemStack> inv = viewer.getInventory().all(Material.KNOWLEDGE_BOOK);
for(Integer slot : inv.keySet()) {
    ItemStack item = inv.get(slot).clone();
    if(!item.getItemMeta().hasCustomModelData() || !ItemStorage.getRunes().containsKey(item.getItemMeta().getCustomModelData()))
        continue;
    WearableItem rune = ItemStorage.getRunes().get(item.getItemMeta().getCustomModelData());
    if(rune.cannotBeUsed(target))
        continue;
    StaticGuiElement element = new StaticGuiElement(
        'i',
        item,
        1,
        click -> {
            selectRuneLogic(item, false);
            return true;
        }
    );
    group.addElement(element);
}
addElement(group);

int i = 1;
Log.debug("SIZE2:{0}", RuneStorage.getEquippedRunes(target.getUniqueId()).size());
for(ItemStack item : RuneStorage.getEquippedRunes(target.getUniqueId())) {
    int finalI = i;
    DynamicGuiElement element = new DynamicGuiElement(
        Character.forDigit(i, 10),
        v -> new StaticGuiElement(
            Character.forDigit(finalI, 10),
            item,
            1,
            click -> {
                selectRuneLogic(item, true);
                return true;
            }
        )
    );
    addElement(element);
    i++;
}
show(viewer);

[Phoenix616]

Two things:

• You are using a dynamic element for the first row but a non-dynamic one for the other one. Is that intentional? If you want to update the content by using redraw you should use a dynamic one for all elements you want to change. (You can have a dynamic element return a whole group too)
• Without seeing the code that actually modifies your data and redraws the gui this is hard to impossible to really debug this any more than that for me.

[FaMa91]

The dynamicElement is the one not working properly.
I tried calling the draw function in the click lambda, before the show(viewer) and instead of the show(viewer) none of this sovled the issue.
Right now I don't call the draw function.
The show function you see is the one from inventoryGui.

The code that modifies my data is working properly, I tested it but anyway this is it:

import org.bukkit.inventory.ItemStack;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;
import java.util.UUID;

public class RuneStorage {
    private static final HashMap<UUID, Set<ItemStack>> equippedRunes = new HashMap<>();

    public static Set<ItemStack> getEquippedRunes(UUID uuid) {
        return equippedRunes.computeIfAbsent(uuid, k -> new HashSet<>());
    }

    public static void removeEquippedRune(UUID uuid, ItemStack rune) {
        Log.debug("SIZE:{0}", getEquippedRunes(uuid).size());
        getEquippedRunes(uuid).remove(rune);
        Log.debug("SIZE:{0}", getEquippedRunes(uuid).size());
    }

    public static void equipRune(UUID uuid, ItemStack rune) {
        getEquippedRunes(uuid).add(rune);
    }
}

If there is anything else I can tell you to help, let me know

[Phoenix616]

If there is anything else I can tell you to help, let me know

Well the selectRuneLogic would be good.

[FaMa91]

	private void selectRuneLogic(ItemStack rune, boolean active) {
		if(active) {
			RuneStorage.removeEquippedRune(target.getUniqueId(), rune);
			InventoryUtil.addItemToInventoryOrDrop(viewer, rune);
		} else {
			if(RuneStorage.getEquippedRunes(target.getUniqueId()).size() >= max) {
				viewer.sendPlainMessage(Lang.getMessage("effect.runeEffect.full").replace("%max%", String.valueOf(max)));
			} else if(!canEquipRune(target, rune)) {
				viewer.sendPlainMessage(Lang.getMessage("effect.runeEffect.doubleRune").replace("%rune%", BasicItem.getItemName(rune)));
			} else {
				if(InventoryUtil.removeItemFromInventory(viewer, rune)) {
					if (rune.getAmount() > 0) {
						RuneStorage.equipRune(target.getUniqueId(), rune);
					}
				}
			}
		}
		runeGui();
	}

[FaMa91]

So am I doing something wrong or is there a bug for you to solve? Because if it's the latter I need to implement a workaround in some way

[Phoenix616]

Sorry for the late reply. I don't see how this could be a bug in the library itself as you are recreating the GUI every time you call selectRuneLogic, not redrawing an existing GUI. (So you don't even need to use a DynamicGuiElement if you are rebuilding the full InventoryGui object on every click)

Your issue most likely simply stems from the fact that you are not removing the rune ItemStack from your RuneStorage as you are purely relying on the hashcode of the ItemStack in the HashSet to match on removal which will very likely to not be the case for two different ItemStack objects due to clones potentially taking place so I would suggest checking that while debugging first.

[FaMa91]

As I said before:

This is the output of the .size() on the set before and after having applied the logic.
[21:39:02 INFO]: [TEST-core] SIZE:1
[21:39:02 INFO]: [TEST-core] SIZE:0

The hashSet is empty when I redraw the GUI, there must be something left in cache.
I tried different versions of it, with and without dynamicElement

[FaMa91]

I added additional logs and substituted the DynamicElement with a StaticElement, same issue:

Log.debug("SIZE2:{0}", RuneStorage.getEquippedRunes(target.getUniqueId()).size());
if(RuneStorage.getEquippedRunes(target.getUniqueId()).isEmpty()) {
	Log.debug("empty RuneStorage.getEquippedRunes");
} else {
	Log.debug("NOT empty RuneStorage.getEquippedRunes");
	for (ItemStack item : RuneStorage.getEquippedRunes(target.getUniqueId())) {
		int finalI = i;
		StaticGuiElement element = new StaticGuiElement(
				Character.forDigit(i, 10),
				item,
				1,
				click -> {
						selectRuneLogic(item, true);
						//click.getGui().show(viewer);
						return true;
				}
		);
		addElement(element);
		i++;
	}
}
show(viewer);

Those are the logs:
[19:58:04 INFO]: [TEST-core] SIZE:1
[19:58:04 INFO]: [TEST-core] SIZE:0
[19:58:04 INFO]: [TEST-core] SIZE2:0
[19:58:04 INFO]: [TEST-core] empty RuneStorage.getEquippedRunes

This confirms the for is not reached when the issue happens