search

Kir-Antipov / sync-fabric

One mind. Many bodies.
MIT License
17 stars 13 forks source link

[Bug] NBT Tag Size Too Big Kicking Me From My Server #39

Open CantWeAllDisagree opened 1 year ago

CantWeAllDisagree commented 1 year ago

What happened?

When Right Clicking Shell Constructor, kicked from server for exceeding NBT Tag size

Mod Compatibility Issue?

Version

mc1.19.2-4.3

Minecraft Version

1.19.2

Fabric API Version

qfapi-4.0.0-beta.30_qsl-3.0.0-beta.29_fapi-0.76.0_mc-1.19.2

Installation Source

Modrinth

Logs

https://pastebin.com/zgntbTXu

Other Mods

No response

Additional Information

Not sure if its a mod compatibility issue or a issue with Sync as it can be reproduced with different mods, and I assume that its bloating the player save when you make a clone. Trying to load a world with a shell inside a constructor after adding these mods creates the same issue.

https://modrinth.com/mod/artifacts https://modrinth.com/mod/vanity

Had issues with these 2 mods, assume there may be more.

Kir-Antipov commented 1 year ago

Wow. More than 2 MiB for the NBT. The most memory-intensive part is shell's inventory, so are you sure that the problem is caused by a shell in a constructor, not in a storage?

Also, it would be nice if you could get the NBT in question, so we can inspect why it's so bloated.

CantWeAllDisagree commented 1 year ago

world.zip

Reproduced the error on a fresh world on the server. Not sure what to look for in the NBT so heres the whole world zip.

All I do to produce the error is place a Shell Constructor and right click it. Immediately get kicked for a NBT tag save issue and cant reconnect unless I delete my player save data or the world.

If I delete the player save data I can reconnect and see the constructor with the shell, but if i place a new constructor same issue.

Would love to get this figured out as I'm trying to make a mod pack and I would love for sync to work. If it ends up being a issue with other mods and sync I will make sure to move the Issue there!

2023-03-27_16 38 19

Kir-Antipov commented 1 year ago

Oh god...

Oh god

I think, you can guess where the issue comes from :D

CantWeAllDisagree commented 1 year ago

Oh lord, too many trinkets. Fuuuuuuuuun

CantWeAllDisagree commented 1 year ago

It was EMI Loot For whatever reason????

Kir-Antipov commented 1 year ago

The fun part is all of them are minecraft:air.

Welp, my bad. The issue is I misunderstood how the "Trinkets" lib works under the hood. I genuinely thought that its inner inventory should only contain trinkets which are actually equipped on the player, but it seems that the library allocates a slot for every registered trinket in every available trinket group (for some reason). So, in your case it's 41 trinkets per 13 groups, which is equal to 533 (!) slots.

While I really need to add .isEmpty() check to this method to eliminate the "NBT-bomb" on my side:

https://github.com/Kir-Antipov/sync-fabric/blob/e821350f37576909751053ec39fba3f15b852689/src/main/java/dev/kir/sync/compat/trinkets/TrinketShellStateComponent.java#L89-L105

I wouldn't say that the responsibility for this problem is completely on me.

Kir-Antipov commented 1 year ago

It was EMI Loot For whatever reason

This may be a problem on "EMI Loot"'s part, because every empty item had emi:[something_something] enchantment on it (why?...). Maybe this is what caused this strange behavior of the "Trinkets" library, but I'm not entirely sure.

CantWeAllDisagree commented 1 year ago

Yeah I removed EMI Loot for now as its not nearly as important as Sync lol. But damn that's a lot of air.