search

Caltinor / Project-MMO-2.0

A continuation of Harmony's Project MMO project
37 stars 21 forks source link

Client side crash when hovering over an item >99 stack size #551

Open Natanaelel opened 1 week ago

Natanaelel commented 1 week ago

Describe the bug Client crashes when trying to save stacks larger than ItemStack.CODEC allows https://github.com/Caltinor/Project-MMO-2.0/blob/main/src/main/java/harmonised/pmmo/util/TagUtils.java#L72 Paste your crash log here crash-2024-06-21_00.35.36-client.txt debug.log https://github.com/Natanaelel/BankStorage/issues/21

Caltinor commented 1 week ago

The line you reference is a vanilla method for serializing ItemStacks to NBT. How do you save itemstacks in your mod with a value greater than 99?

Natanaelel commented 1 week ago

https://github.com/Natanaelel/BankStorage/blob/1.21-neoforge/src/main/java/net/natte/bankstorage/state/BankSerializer.java#L22

Natanaelel commented 1 week ago

I wonder why do you have to serialize items clientside?

Caltinor commented 1 week ago

https://github.com/Natanaelel/BankStorage/blob/1.21-neoforge/src/main/java/net/natte/bankstorage/state/BankSerializer.java#L22

This may work for your custom serialization inside your mod, but vanilla isn't using this codec to serialize these itemstacks.

I wonder why do you have to serialize items clientside?

Project MMO let's users define the player skills that are required to interact/use items based on their NBT (for example potions are one item with different NBT values based on the potion). The tooltips show this information and therefore the client has to parse the NBT tag according to the mod configuration to display the correct requirement for the item.

Natanaelel commented 1 week ago

I've looked at your code and I see no reason for serializing instead of just passing the ItemStack directly. Am I missing something?

Caltinor commented 1 week ago

yes. I need the NBT value. There is no way around reading itemstack properties dynamically without having a JSON-like representation of the data. This feature allows for mods like Tinkers/Tetra/SilentGear to (which all have one item that changes based on NBT) have custom XP and requirements based on its composite properties.

the alternative would be to have explicit compat with every possible mod and their specific items so that I could parse the config string explicitly to data components, which is not practical.

Natanaelel commented 1 week ago

Okay. If you need item count then you are welcome to copy my codec. Otherwise ItemStack has SINGLE_ITEM_CODEC which could work? I don't think I can solve it on my side.

Natanaelel commented 1 week ago

would it be fine if I pr'ed a fix?

Caltinor commented 1 week ago

would it be fine if I pr'ed a fix?

As mentioned in the issue on your project, your choice to violate the stack limit minecraft sets makes your mod incompatible with others, not the other way around. You need to find a better way to store larger-than-vanilla stack sizes. It's PMMO today, but it will be something else tomorrow.

BLUF: don't waste your time on a PR. The issue isn't with pmmo.

Natanaelel commented 1 week ago

The only thing that >99 stack limits violate is the vanilla codec CODEC. I think it is my responsibility as the storage mod to serialize items. Similar mods (dank, sophisticated) have the same approach of simply storing the count in the stack. I would say that your usecase of serializing items clientside is rather unique and therefore requires special caution. I'm happy to PR my suggested fix. If you don't think it is a good idea I will not spend any more time on this and simply consider pmmo incompatible.

Caltinor commented 1 week ago

The only thing that >99 stack limits violate is the vanilla codec CODEC. I think it is my responsibility as the storage mod to serialize items. Similar mods (dank, sophisticated) have the same approach of simply storing the count in the stack. I would say that your usecase of serializing items clientside is rather unique and therefore requires special caution. I'm happy to PR my suggested fix. If you don't think it is a good idea I will not spend any more time on this and simply consider pmmo incompatible.

In pre 1.20.6 it didn't matter if you serialized more than the stack size because the NBT limit was the integer limit. In 1.20.6+ data component have stricter limitations. PepperFly hasn't even released a 1.20.6 or 1.21 version for sophisticated backpacks yet. and Itemstack hasn't released a version for dank after 1.20.1. They will have to develop their own solutions for storing items as well. I would reach out to them to see what they plan to do. If their plan is to serialize with a custom codec as you do, then I will reconsider changing pmmo.