TerraFirmaCraft

TerraFirmaCraft

2M Downloads

Vanilla server incompatibility due to FriendlyByteBufMixin

juanmuscaria opened this issue ยท 6 comments

commented

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)

commented

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,

commented

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.

commented

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.

commented

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)

commented

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.