Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NetworkCodecs #180

Closed
wants to merge 13 commits into from
Closed

NetworkCodecs #180

wants to merge 13 commits into from

Conversation

0xJoeMama
Copy link

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 0xJoeMama requested a review from a team as a code owner August 22, 2022 14:38
@0xJoeMama 0xJoeMama mentioned this pull request Aug 22, 2022
@0xJoeMama 0xJoeMama changed the title Network codecs NetworkCodecs Aug 22, 2022
Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't expose impl classes from an API class.

@0xJoeMama
Copy link
Author

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

@LambdAurora
Copy link
Contributor

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

@0xJoeMama
Copy link
Author

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.

@LambdAurora
Copy link
Contributor

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.

@0xJoeMama
Copy link
Author

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
Copy link
Author

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

@TheGlitch76 TheGlitch76 dismissed their stale review August 28, 2022 15:46

concerns addressed

Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for getting the split done; that's all i really had to say about this PR at the moment

@LambdAurora LambdAurora added library: core Related to the core library. t: new api This adds a new API. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. labels Aug 28, 2022
private final NetworkCodec<A> entryCodec;
private final IntFunction<? extends A[]> arrayFactory;

public ArrayNetworkCodec(NetworkCodec<A> entryCodec, IntFunction<? extends A[]> arrayFactory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of construction methods left public through all these classes.

Are users meant to be able to construct these codecs directly, or always go through NetworkCodec, NetworkCodecBuilder, etc?

@Cypher121
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: core Related to the core library. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants