When slicing the itemStack by right-clicking the counter doesn't update.
It also doesn't count the emeralds on the cursor.
Because the sever only sends ClientboundContainerSetSlotPackets when the prediction of the client is false, a solution has to listen to both ServerboundContainerClickPacket(e.g. using mixins in MultiPlayerGameMode#handleInventoryMouseClick) and ClientboundContainerSetSlotPacket(e.g. using ContainerSetSlotEvent).

Does this really happen on latest version? I am confident I fixed this a while back...

it does. I tested it on the dev version. The event used only fires when the ItemStack gets removed or added and not when edited.

In EmeraldModel.onSetSlot() there is at least a piece of buggy code. IntellJ says about:

} else if (event.getContainer() == McUtils.containerMenu()) {

that there is no subclass that can ever match this test.

I don't know if this causes the reported bug, but it should be fixed, either by making a proper test (if needed), or by removing this code (if this was not really needed).

I will test this once I get home but my ide doesn't complain and they also do have a common super class:container

In EmeraldModel.onSetSlot() there is at least a piece of buggy code. IntellJ says about:

} else if (event.getContainer() == McUtils.containerMenu()) {

that there is no subclass that can ever match this test.

I don't know if this causes the reported bug, but it should be fixed, either by making a proper test (if needed), or by removing this code (if this was not really needed).

This is actually not true. The event returns AbstractContainerMenu (abstract class) and the MC code returns Container which is an interface. This means that there might be classes that inherit/implement both, but Qodana couldn't figure that out.

oh ye, i thought he was refering to event.getContainer() == McUtils.inventory();. A container is a block or entity eg. chest boat and the container menu is an "inventory" so there is no common superclass (chest block vs chest menu).

In EmeraldModel.onSetSlot() there is at least a piece of buggy code. IntellJ says about:

} else if (event.getContainer() == McUtils.containerMenu()) {

that there is no subclass that can ever match this test.
I don't know if this causes the reported bug, but it should be fixed, either by making a proper test (if needed), or by removing this code (if this was not really needed).

This is actually not true. The event returns AbstractContainerMenu (abstract class) and the MC code returns Container which is an interface. This means that there might be classes that inherit/implement both, but Qodana couldn't figure that out.

Tested this, and it is not always correct, don't rely on it.

As for my other changes, I pushed to https://github.com/Wynntils/Artemis/tree/experimental_emerald_fix, but I still couldn't get it to work..

I think a ContainerMenu and a Container are two separate concepts in the vanilla code base. A container is more abstract description of a storage, but the ContainerMenu is about how the GUI is presented on the screen for interacting with the container. So I think Qodana is right and the test is incorrect.

(That being said, it might not cause the bug described here; maybe all it does is that we have a piece of dead code. But my assumption was that maybe it was supposed to test for something else, and if it did that correctly, things would work.)

I am close to removing this whole optimization. We used to sum up emeralds on set item/set slot so we don't have to parse and sum up items every frame, like legacy did. But since Artemis has item parsing, we would just have to sum up the count on render time, which is FAR more faster, maybe to the point that this whole mess is not needed...

What do you think @magicus?

EmeraldModel should be able to return an up-to-date value of how many emeralds we have. In worst case, we have to return that value by summing over all items when it is requested; I think that having a the information stored explicitly in the model is preferable, not only due to performance but e.g. as a debugging help. But maybe summing is not that bad if you test if each item is the correct WynnItem first. Is that what you are suggesting? I'd think that updating the value each time we update a slot would be fine, but maybe that's too tricky to get correct.

I don't really understand what you mean by "render time" vs "every frame" though...

EmeraldModel should be able to return an up-to-date value of how many emeralds we have. In worst case, we have to return that value by summing over all items when it is requested; I think that having a the information stored explicitly in the model is preferable, not only due to performance but e.g. as a debugging help. But maybe summing is not that bad if you test if each item is the correct WynnItem first. Is that what you are suggesting? I'd think that updating the value each time we update a slot would be fine, but maybe that's too tricky to get correct.

I don't really understand what you mean by "render time" vs "every frame" though...

I would basically sum up on every request instead of on set slot, since it is too tricky to get right.