Mekanism

Mekanism

111M Downloads

Tile Entity networking (initial syncing) with BetterPortals

Johni0702 opened this issue ยท 2 comments

commented

The state of Mekanism's tile entities appears to not be synced with the initial chunk packet (1) but instead requires the client to actively poll (2) for an update.
Why is that?

Is there some non-obvious reason to not change it (which I'm missing) or is there just a lack of reason to change it because it works fine as is?

In the latter case, BP is here to help: It only allows client-to-server packets in the main world and drops any packets sent from a secondary world. As a result, the Teleporter doesn't work when it's in two different worlds: Johni0702/BetterPortals#382 (sponge is only mentioned because there's another bug in BP which results in similar behavior, completely unrelated though)
Mekanism is the first mod with a (somewhat) valid case for sending client-to-server packets on a secondary world, however I still think this should be fixed on Mekanism's side because:

  • in 99% of cases a packet which is sent in a secondary world wasn't actually supposed to be sent there in first place (e.g. ChiselsAndBits/Chisels-and-Bits#473 (comment) or it's a packet which is moving the (fake) player in the secondary world and you don't really want that to happen on the server either)
  • adding client-to-server tunneling to BP requires a significant amount of work because, untill now, it was operating under the assumption that doing so is not necessary, whereas
  • there's no strong reason to keep Mekanism's current initial sync logic (afaict) and it should be fairly simple to switch it over to using the vanilla/forge methods
  • the comments in (1) suggest that you wanted (or at least considered) to do this anyway
  • removing the polling would reduce network traffic and load time (even if just by a tiny amount)

Implementation

AFAICT implementing this change should be as simple as removing the previous initial sync requests (one in onLoad, one in onValidate) and then instead adding (or rather modifying (1)):

    override fun getUpdateTag(): NBTTagCompound {
        val bytes = Unpooled.buffer().let { buf ->
            PacketHandler.encode(getNetworkedData(TileNetworkList()).toTypedArray(), buf)
            ByteArray(buf.readableBytes()).apply { buf.readBytes(this) }
        }
        return super.getUpdateTag().apply {
            setByteArray("NetworkedData", bytes)
        }
    }
    override fun getUpdatePacket(): SPacketUpdateTileEntity? = SPacketUpdateTileEntity(pos, 0, updateTag)

    override fun handleUpdateTag(tag: NBTTagCompound) {
        super.handleUpdateTag(tag)
        val bytes = tag.getByteArray("NetworkedData")
        handlePacketData(Unpooled.wrappedBuffer(bytes))
    }
    override fun onDataPacket(net: NetworkManager, pkt: SPacketUpdateTileEntity) = handleUpdateTag(pkt.nbtCompound)

Pardon the Kotlin, I wrote and tested this in BP's Teleporter overwrite before deciding that it should probably go upstream. This does solve the original issue but I'm not sure if it works fine under all circumstances since I'm not very familiar with Mekanism's networking system.
Normal updates (Mekanism's TileEntityMessage) should be completely unaffected by above change. One could switch them to use SPacketUpdateTileEntity as well but right now I see no good reason to so.

commented

Most of why it is the way it is is indeed legacy code we haven't updated (because it is a rather large change)

commented

Finally got around to finishing removing client -> server data requests. In 9.9.16 cleaned up the majority of it, 9.9.18 will fix the remaining cases with transmitters.