quilt-standard-libraries icon indicating copy to clipboard operation
quilt-standard-libraries copied to clipboard

NetworkCodecs

Open 0xJoeMama opened this issue 3 years ago • 7 comments

This PR adds:

  • NetworkCodecs to the networking module. They are the equivalent to Codecs but are network safe. They do not send NBT but rather they use the builtin PacketByteBuf methods. They have a similar API to Codecs allowing one to use them extensively.

Split from #179 because it had 2 features, one of which needed further discussion.

0xJoeMama avatar Aug 22 '22 14:08 0xJoeMama

Yeah I noticed that. I'll get on it.

0xJoeMama avatar Aug 24 '22 07:08 0xJoeMama

Random thought, does this allow to transform a normal Codec into a network codec?

LambdAurora avatar Aug 28 '22 13:08 LambdAurora

Depends on what 'transform' refers to. Currently you can create a NetworkCodec from a normal Codec. But that just writes NBT.

There might be ways to properly create a NetworkCodec though. However, I feel as if it's gonna be a huge pain to do so.

0xJoeMama avatar Aug 28 '22 13:08 0xJoeMama

There might be ways to properly create a NetworkCodec though. However, I feel as if it's gonna be a huge pain to do so.

Wouldn't it be possible to create a PacketByteBuf-backed Ops class? like JsonOps or NbtOps? This would bring the ability to reuse existing codecs.

LambdAurora avatar Aug 28 '22 13:08 LambdAurora

This would bring the ability to reuse existing codecs.

That's something I have been thinking about. It just doesn't work. An Ops class requires something similar to the JSON tree structure. Something where you have elements, arrays and compounds. It also needs the ability to merge stuff, which is really inefficient with ByteBufs.

0xJoeMama avatar Aug 28 '22 13:08 0xJoeMama

I originally started with that as my idea. However, from what I have gathered, it's just not possible.

0xJoeMama avatar Aug 28 '22 13:08 0xJoeMama

That's something I have been thinking about. It just doesn't work. An Ops class requires something similar to the JSON tree structure. Something where you have elements, arrays and compounds. It also needs the ability to merge stuff, which is really inefficient with ByteBufs.

As discussed on Discord some time ago, I'd like to point to CBOR as prior art on binary representations of JSON-like structures, maps, lists, nulls and all. Full spec found in RFC 8949.

I see two options for implementing this. First, the easy route: piggyback off of JsonOps, grab the resulting JsonElement, write it into CBOR by following the spec. No Ops implementation needed. However, JSON has worse typing than CBOR, not being able to distinguish between floating-point and integer numbers, and not allowing non-string keys in maps, meaning the format would lose efficiency and expressive power: Map<Int, Int> is now encoded wastefully as Map<String, Double>, and users have to do the Int<->String conversion themselves since JsonOps doesn't bother itself with such things.

Second option is a full-fledged DynamicOps implementation, and here I'd like to give some pointers on implementing:

  • Section 4.2.1 of the RFC has some notes on how to achieve the least space used possible, including checking the min number of bytes for an integer, etc.

  • Instead of trying to write the whole thing directly into a packetbytebuf, it will likely be easier to write it as a JsonElement-like object structure and only once that's done serialize it. This would make merging significantly more straightforward as well.

Encoding maps and lists is a pretty crucial operation, so it should be implemented either way, and this provides a well-described and known format to do so. For my personal use-cases, I have a packet which sends a Map<VillagerProfession, Pair<Int, Int>> to clients, and it'd be a shame if that kind of structure would have to fall back to writing bytes directly instead of using codec composition. And funneling network through the existing structure would save a few headaches for QKL's upcoming serialization-codec bridge as well.

Cypher121 avatar Sep 26 '22 22:09 Cypher121