Vanilla server incompatibility due to FriendlyByteBufMixin
juanmuscaria opened this issue ยท 6 comments
Because FriendlyByteBufMixin adds additional buffer reads/writes it breaks compatibility of joining vanilla servers (or servers lacking TFC itself).
A legit use of being able to join vanilla servers is hub servers before sending the player to the forge server.
Putting the capability data inside the stack nbt seems to work from my tests (see fix.txt). If it is an acceptable solution, I can PR that change, or think of another solution if needed.
(Mod version built directly from commit 733e809)
I don't have any objections to this in principle or the implementation, but I am mostly just concerned that it does in fact, work properly, both w.r.t the creative menu, dedicated/integrated server environments. Ensuring that the tag key is sent namespaced, which should ensure no conflicts with other data sent,
I'll change it to include prefixed capability names (modid:capability) and test on a live server with my dev modpack to see if any issue arises from that fix.
It works, but looking through how I implemented again, I noticed that I'm mutating the item stack nbt while serializing to network and not removing the added capability data. And even if I add to remove the extra data, it does not change the fact the serialization method now changes the item stack in some way. While I've not had any issue with that, it sounds like an issue cooking to cause problems in the future.
Copying the item stack or nbt just for serialization is wasteful.
One way I'm thinking of solving this issue is redirecting the writeNbt method within writeItemStack and injecting capability data in there directly, which should be fairly clean and keep compatibility with vanilla servers.
We've had very difficult to debug issues before with capability reading and stack serialization (i.e. see the notes about synchronization on ItemStackCapabilitySync
, and issues like #2465, #2212, etc.)
Copying the tag before writeNbt
, followed by modification, should be safe, not involve unsafe capability queries, and not mutate the item stack being sent in any way. (And is less expensive than copying the stack, which will involve a capability initialization from AttachCapabilitiesEvent
being fired)
With writeNbt redirection (Dirty mixin with the fix here FixTFCCapSync.java) seems like to work flawlessly in single player. I'll test it on a live server and see if I find any issue before making a PR.