AppliedEnergistics / Applied-Energistics-2

A Minecraft Mod about Matter, Energy and using them to conquer the world..
https://appliedenergistics.github.io/
Other
1.4k stars 624 forks source link

Flower Pouch emptied when put in ME Network #2624

Closed Xiaminou closed 6 years ago

Xiaminou commented 7 years ago

Description

When putting a flower pouch from Botania that contains mystical flowers into the ME Network and pulling it back into your inventory it will end up empty of flowers.

Environment

list.txt

yueh commented 7 years ago

This is mostly true for anything using capabilities to store data, because there is no way to serialise this part without a potential performance issue.

No idea how to fix it, except of ASM transforming forge directly.

mindforger commented 7 years ago

how about exposing an API to allow mod makers to implement their own serialisation for items with special content ... sure the performance threat is still there but you can blame it on the other mod devs then :)

nbt tags can be very bothersome to use for "efficient" serialization ... but i see a threat there where you put and inventory into another inventory that has an inventory inside XD ... you could possibly limit any exploitation to this by putting a price tag on such items ... for each x amount of serialized data it will take another byte of your storage drive, making stacked inventorys not something like "infinite zip containers" in your me drives

yueh commented 7 years ago

The API exists and is called Forge. They just decide to not expose it for itemstacks. Because Lex for whatever reasons deciedes it is a great idea to fully serialise an itemstack to NBT than deserialise parts of it to fetch the caps and serialise them again to NBT. And that is actually something Forge itself does already to some degree...

Xiaminou commented 7 years ago

That's what I was afraid of, but it worked in 1.7.10. Will it ever work again or are items with NBT doomed to never be stored in ME Networks?

yueh commented 7 years ago

Normal NBT is fine, so please do not spreading rumors about it.

It is just about caps. Which use their own data storage instead of the normal NBT tag. Which kinda makes sense because anyone can attach random caps to anything, thus breaking recipes and what not would they share the same data. But it will be a major issue once every idiot starts doing it and attach stuff to every itemstack, which will be a huge issue at some point because serialising that data is slow. And depending on how data is synchronised between the server and client it might also affect the network load quite a bit. There are already enough mods, which can bring down a server with just a few (normal) NBT heavy items. Either by completely utilise your network, especially ones hosted at home or kill the TPS due to NBT being serialised and taking a major part of the cpu time. Previously that was mostly possible with things like bags. Now even cobblestone can cause it.

Xiaminou commented 7 years ago

Why would I be spreading rumors? I don't even play with anyone else and I hardly ever talk to people who play minecraft.

yueh commented 7 years ago

Oh sorry, the avatar colour throw me off a bit and I typed it during the short github downtime without any real verification after copy&pasting it once it was back online.

But just in terms of correctness, other people will read the issues and might misunderstand it and spread that they heard it has problems with any item having NBT data and just the ones with capabilities.

mindforger commented 7 years ago

he partially means me :) but we already sorted that out

Xiaminou commented 7 years ago

I do believe people who read issues on Github are smart enough not to draw conclusions from a single post, especially not from such an inexperienced person as me.

bookerthegeek commented 7 years ago

Then you have had a much more pleasant experience with github users then most.

Maybe some sort of blacklist?

yueh commented 7 years ago

Would require the same approach. ASM transform or reflect into forge and check if an item has any capability as currently it is only possible to query each one explicitly. And that is the same effort we had to do to serialise the NBT for caps directly.

yueh commented 7 years ago

We also cannot forget that there is still a possible forge bug which will cause ItemStacks to lose their cap nbt on servers. So even if we are able to serialise it, forge might still cause it to be lost in some cases.

dshadowwolf commented 7 years ago

Perhaps call 'serializeNBT' on any incoming item that supports said call and have a 'deserializeNBT' on extract from storage?

There might be, however, a Forge bug where ItemStack caps are broken on dedicated. Give me a day or two to make sure I'm not breaking things completely and I'll see if I can spin a patch for this. It won't be simple or easy, but it should cover all cases where the incoming ItemStack has been implemented to actually care about its Cap data :)

yueh commented 7 years ago

This is exactly what I want to avoid. We only need the ForgeCaps key from the fully serialised stack. So we would have to serialise each itemstack, then deserialise it to extract this one key and serialize it again.

Which similar to the approach other storage mods use and which you can easily exploit to take down a server with like 4 blocks.

KaneHart commented 7 years ago

pfft I could show you how to do it in 3... Just shift clicking 9 things into 1 via crafting terminal can easily knock a server down with the packet thread or whatever. Shown the profiles on this before before. This issue now is insane and a joke. Can't have items vanishing like this either put something in to stop it from being stored or fix it really. I don't want to sound like the nasty person here but now I'm looking at the other mod now just because I don't want my users to make silly mistakes.

fscan commented 6 years ago

AE2 now uses forges ByteBufUtils.writeItemStack to synchronize ItemStack's which uses the NBTShareTag (defaults to the full NBT tag). If the Bonatia bag fails to synchronize with this it looks to me like a Botania bug.