Flower Pouch emptied when put in ME Network #2624

Open
Xiaminou opened this Issue Nov 15, 2016 · 14 comments

Projects

None yet

5 participants

@Xiaminou
Xiaminou commented Nov 15, 2016 edited

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

  • Minecraft Version: 1.10.2
  • AE2 Version: alpha 6
  • Forge Version: 2125
@yueh
Member
yueh commented Nov 16, 2016

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.

@yueh yueh added the type-bug label Nov 16, 2016
@yueh yueh added this to the rv4 beta - 1.10 milestone Nov 16, 2016
@mindforger
Collaborator
mindforger commented Nov 16, 2016 edited

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
Member
yueh commented Nov 16, 2016

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

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
Member
yueh commented Nov 16, 2016

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

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
Member
yueh commented Nov 16, 2016

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
Collaborator

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

@Xiaminou

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
Collaborator

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

Maybe some sort of blacklist?

@yueh
Member
yueh commented Nov 16, 2016

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
Member
yueh commented Nov 17, 2016

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
Contributor

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
Member
yueh commented Dec 9, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment