#Networking Protocol

1 messages · Page 1 of 1 (latest)

errant summit
mental ingot
#

A nice :d

#

You around?

#

I thought you said around 3PM today?

errant summit
#

yep plans changed

opal ferry
#

Nice 🙂 Let me know of any questions. Im quite happy to help test it as well.

mental ingot
#

Okey

#

I will jump in voice chat then

#

@errant summit VC?

errant summit
#

yea just getting set up

mental ingot
#

Okey

indigo hazel
#

We can solve the “is this a compatible server check”, by using a different networking protocol versioning scheme if forge is an incompatible server. Current suggestion is to simply negate the vanilla protocol version. A vanilla client would as such reject the connection, while a forge enabled client would be aware of this
Assuming you are talking about the server list info, that shouldn't be necessary, the additional ping data we attach to the ClientboundStatusResponsePacket should be sufficient for that. Adding that is fine for a vanilla client because the data gets sent as JSON and we just modify the codec to add an optional entry. In fact, checking for the presence of this optional data is already how the server list compat check tests for vanilla vs. Neo connection

mental ingot
mental ingot
#

So we know what kind of connection it is / can be

#

But I am currently debating ripping clientintent out

#

And doing that kind of check differently

#

However ServerStatus could likely stay the same

#

As it is a JSON structure that is being send

#

I am trying to determine

#

Where the whole FML logic of: Is this a compatible connection

#

Should live

#

And in what place in the connection

indigo hazel
# mental ingot But I am currently debating ripping clientintent out

We have to rip out the crap in client intent because it either automatically makes us incompatible with a vanilla server in a way that produces nonsensical results for the user or requires crap like the host name hack. We also have to keep in mind that a proxy/hub server could use the config phase to move a player between a vanilla and a modded server, so we can't use the early login stuff to detect the server type and compatibility.

mental ingot
#

Actually

#

A proxy server can't do that

#

Wheter it uses the configuration phase or not

#

At least not right now

indigo hazel
#

Why wouldn't it?

mental ingot
#

Right now: Because all connection properties and connection channel configuration happens way earlier

#

It happens during handshake

#

At the moment I am investigating what the concequences of it are when we move all of that back into the configuration phase

#

As far as I can tell

#

It should be possible

#

Cause nothing really of interest uses those properties earlier

indigo hazel
#

Yeah, I'd say there's nothing stopping use from moving all our stuff to the config phase

mental ingot
#

Technically it is used during STATUS as well

#

But only implicitly

#

And it only works because of the JSON hack

indigo hazel
#

The status packet is, as far as I can tell, only used in the STATUS mode and that's the only packet where the JSON hack applies.

mental ingot
#

Yeah

#

What i am saying is that it only works because of the JSON hancks

indigo hazel
#

Absolutely

mental ingot
#

What I find particularly stupid is the fact that mojang does not encode the string length before it writes a UTF string

#

but always hardcodes the limit XD

indigo hazel
#

Actually, they do write the length, it's hidden in Utf8String.write(). Took me a minute to notice as well though

mental ingot
#

Right now I am trying to decern wether there is a better solution

mental ingot
#

That is stupid as all hell

indigo hazel
#

No idea whatsoever 😄

mental ingot
#

What I am trying to figure out is, is if it is possible to figureout what kind of client the server is talking too and what kind of server the client is talking to, from a combination of the handshake and server status packets

buoyant pagoda
#

wait, are we questioning why Mojang has a limit to strings instead of being potentially boundless thonk

mental ingot
#

No

buoyant pagoda
#

(or practically a very chonky string)

mental ingot
#

I am wondering why it is 32000-+ characters

#

100000 is perfectly fine

#

And then a modded environment would likely never run into any issues what so ever

#

But that is just me

buoyant pagoda
#

both are arbitrary numbers
and the former is probably because it's near to a power of two

#

and that it's probably well sufficient for a vanilla environment, which is what they mostly program in mind for

errant summit
#

its 7FFF hex

#

or 111111111111111 in binary

buoyant pagoda
#

so largest positive value for a signed short

errant summit
#

it's max short

mental ingot
#

Yeah

#

It is max.short

#

Why it is a short

#

I don't know

#

Because shorts are on the VM level just ints......

buoyant pagoda
#

but they aren't shorts across the wire thinkies

mental ingot
#

They don't go across the wire

buoyant pagoda
#

there's probably some sort of compatibility concern at play

mental ingot
#

That is the point

indigo hazel
#

The number that does go on the wire is a varint

buoyant pagoda
indigo hazel
#

I gotta leave for now. I'll be taking another look over the ideas either later tonight or tomorrow depending on how late I'm home again

mental ingot
#

Question: Do we want to keep Event based networking around?

fervent token
#

how do they currently work?

mental ingot
#

Via NetworkInstance

fervent token
#

does anyone other than maty know how to query waifu? If so we could try to see what mods make use of that so that we can see if there is some use case that isn't covered by the non-event based networking

fair sandal
#

I hate these events @mental ingot

mental ingot
#

Okey

#

Cause I just ripped them out

fair sandal
#

Cool

fervent token
#

I don't see a big reason for them, I just figure we may as well look if there is some use case that isn't satisifed by other means? Given we have the technology @velvet whale

mental ingot
#

You get a single event on the modbus to register your packets and listeners

#

And that is it

fair sandal
#

Btw I think what xfact suggests with keeping the fml client/server detection for the config phase makes sense

#

Good

mental ingot
#

I already designed that in mind

#

And I also ripped the compression hack from server status

fervent token
mental ingot
#

I found a much much much better way

mental ingot
fervent token
#

ah interesting

mental ingot
#

Or for the event a single string is used

#

We basically rip everything out

#

And just use the mojang implementation

#

Cause that is literally what it does itself

#

That system just holds a map: resloc <-> packet/listener

#

And just looks up the resloc of the packet (as its id) in that map

#

And then invokes the listener

fervent token
#

so they kept mods in mind while rewriting their networking. How kind of them

mental ingot
#

Well it is not dynamic

#

But it is trivially easy for me to patch to make it dynamic

velvet whale
buoyant pagoda
#

remind me, why did waifu switch from metabase to grafana

velvet whale
#

because metabase doesn't have general oauth in the CE, just google and ldap

mental ingot
#

@opal ferry In your Fabric PR and its Diagramm, you do a bunch of "minecraft:register" and "ping/pong" packets, combined with "c:version" and "c:register". Me @errant summit are trying to understand what their function is? And why Forge would need something like that?

#

I understand that it is used to detect whether or not it is a modded client at all that we are using

#

And to potentially disconnect them if need be

opal ferry
mental ingot
#

Our new design does not support custom channels

#

And solely relies on the Vanilla CustomPacketPayload / CustomPacketQuery system

opal ferry
#

You cannot start a configuration task such as registry sync unless you know the client will be able to handle it. It will stall the login otherwise.

buoyant pagoda
#

channels are part of the custom payload packet system, Orion

mental ingot
#

Aaahh

#

Okey You are basically saying

#

Make sure the client and server have the same handlers list

#

To ensure that shit does not stall

#

Basically I need to ask for a mod list

#

And disconnect the client if it mismatches

opal ferry
#

Doesn't need to be the same, the other side needs to know if the receiver is able to handle the packet, it can either choose to send it, do nothing, or disconnect.

mental ingot
#

A handler implementation could do jack shit

opal ferry
#

Im surprised you didnt already have something for this? Using the existing minecraft:register packets?

mental ingot
#

Nope

velvet whale
#

c:register is the list of client-mandatory channels the server has basically

velvet whale
mental ingot
#

No we don't

#

Or did I miss this in Forge

opal ferry
#

I dont see how you couldnt not have this?

mental ingot
#

Where is it normally?!?

#

Like in vanilla

#

?

#

Found it

velvet whale
opal ferry
#

This was the API have for configuration tasks. ServerPlayerConfigurationTask is a vanilla class.

Screenshot_2023-11-05_at_16.42.39.png
velvet whale
#

minecraft:register is the core of the whole idea of disconnecting clients that have incompatible versions

opal ferry
mental ingot
opal ferry
#

The critical bit is the ServerConfigurationNetworking.canSend

mental ingot
opal ferry
#

Thats part of our API, and is driven by the minecraft:register packets

mental ingot
#

Yeah

#

But functionally that is not really relevant

#

I mean

#

As long as a task is ran, and eventually triggers the "finishCurrentTask" method in the listener

#

It does not matter

#

Right?

opal ferry
#

But if you start the task and the client cannot complete it you stall the login

mental ingot
#

I could send an opertunistic packet from the server to client

#

Immediatly continue

#

And call it a day

#

But why is that something Forge needs to care for. I mean in practice a mod could do 100 things that prevents the queue from being drained stalling the login

opal ferry
#

Vanilla does it

#

The whole point is you can wait for the client to finish that configuration task before moving onto the next one.

mental ingot
#

Yeah

#

Or you can't

#

It is literally up to the tasks own implementation

buoyant pagoda
mental ingot
#

Yeah

#

But there is not really anything we can do to prevent that

#

I understand the concept

#

I am just saying that it is redundant

opal ferry
#

But what if the mod wants to still support a vanilla client, or a client without the mod?

mental ingot
#

See that is what makes more sense

#

But that exchange of information

#

Does not need to happen on minecraft:register

#

It can literally happen with any packet

#

As long as we collect that information ahead of time, before the task queue is build

opal ferry
#

Why would you not use minecraft:register for it?

#

Its what it was designed for

mental ingot
#

Because architecturally that is a massive headache!

opal ferry
#

How come?

mental ingot
#

Right now, the architecture for the network is a single event

#

and that event does all

#

Literally all

#

Well actually

#

I might be able to shoe horn the channel in there somehow

opal ferry
#

Maybe you need to change that architecture, thats up to you how to want to do that. It should support minecrft:register. We managed it without any problems.

mental ingot
#

Actually I would argue the other way around

#

The API should simply be: this.connection.send(somepayload)

#

That is all that should be needed

opal ferry
#

But its not, you need to know if the other side can receive

mental ingot
#

Sure

#

But I know that even without that channel

#

Because I can do that with a custom payload

#

And if I don't get a payload back

#

Before I get the pong

#

I have the same inforamtion

#

Without ever touching that channel

opal ferry
#

The whole idea of minecraft:register is to send that information in a platform agonistic format.

#

Its been that way since it was added, 10 years ago

mental ingot
#

That is nice, but the age of the thing is not really relevant to me right now, I want a functional reason why it needs to be that packet........

A forge client can not and will likely never connect to a fabric server or vice versa........

opal ferry
#

The modloader/plugin server sends one of those packets for all of the channels registered by mods. You wouldnt expect everyone to have their own packets for this.

#

What about a forge client connecting to a spigot/paper server? There is no reason why a forge client cannot connect to a fabric server.

mental ingot
#

The point is that our new architecture has no channels other then the ones mojang has it self

#

Okey hold on

#

I am starting to see my miscommunication

buoyant pagoda
#

i'd have to agree with modmuss here -- I see no good reason why a Forge client cannot connect to a Fabric server (assuming both sides have no mods installed that add things like blockstates)

mental ingot
opal ferry
#

That makes no sense to me? A channel being mod_id:some_packet or minecraft:register right send via the custom payload? And the phase can be login/configuration/play

mental ingot
opal ferry
#

Why not? I dont see how that can even work?

buoyant pagoda
mental ingot
#

They are all custom paylaods

opal ferry
#

Yes, and each custom payload gets its own channel. I think there is some confusion somewhere

mental ingot
#

Hold on

#

Payloads are not Packets

buoyant pagoda
#

I thought we clarified already earlier that channel in this reference means the channel used in the custom payload packet

mental ingot
#

You can not set the Packet channel equal to the Payload Id

mental ingot
#

I am trying to wrap my head around the definition of a channel

opal ferry
#

I think we have 2 definition's of a channel 😄

mental ingot
#

Because when I read the old EventNetworkChannel

buoyant pagoda
#

which I am trying to define and clarify here

mental ingot
#

It clearly means the packet

#

Not the damn payload!

buoyant pagoda
#

so excuse me if I get offended at your attempt to shut me up while trying to clarify it for your sake

mental ingot
buoyant pagoda
#

i'm bowing out of this conversation now, good luckl

mental ingot
#

And that is simply one person to many

mental ingot
#

Okey

errant summit
buoyant pagoda
#

i will still bow out of this conversation, as it seems my effort to help is unwanted

#

again, good luck

steady sonnet
#

I only barely remember the networking stuff, but isn't the difference here that fabric uses a separate packet ID for each mod, or something along those lines, and forge bundles them together and differentiates the packet internally?

mental ingot
mental ingot
#

And what was implemented before

#

Which is why I am several confused

#

Because the concept of a channel and the concept of a payload id

#

Are significantly different

errant summit
#

what we have now (after the rewrite) is that modders directly use CustomPacketPayload and not SimpleChannel or EventNetworkChannel

mental ingot
#

If i understand modmuss

opal ferry
mental ingot
#

So yeah

#

The concept of a channel on forge and fabric are different

#

But not anymore 😄

#

Cause we are now in the same line as you

#

Because now the custom payload id is the channel id

opal ferry
#

Ok, cool. Thats what makes the most sense imo.

mental ingot
#

It does

#

Which is why we went with it

#

Now the question becomes

#

You said the "minecraft:register" payload id was/is the packet that is send to the client to request the handlers, and then also the id of the packet send back?

opal ferry
#

Server sends its handlers in minecraft:register, and then client responds with its.

mental ingot
#

Okey

#

I mean technically only thing we would need is the information from the client

#

So that when the configuration task list is build mods are aware what is available on the other side

opal ferry
#

We also use this to detect a vanilla client but sending a ping to the client right after minecraft:register, and seeing if we a minecraft:register or pong back first.

mental ingot
#

Yeah that ping/pong I understand

#

But technically it suffices to send a "request" packet with what the client knows

#

I mean yeah we can do that in the same packet

#

But that feels like sending unneeded information over the wire

#

But what ever

errant summit
mental ingot
#

I am still trying to design a system for "versioning" of individual packets

fervent token
#

schurli is that hackmd being actively updated as you guys further finalize things? (if so we probably want to pin it to this thread so that people can easily see the high level overview of the system if they look at this thread)

mental ingot
#

Yes

mental ingot
#

Also

#

We are pretty close to this working

fair sandal
#

If a mod registers at least one of these, then the server is considered not compatible with vanilla
this might be problematic for some vanilla+ use cases

fervent token
#

and mods with optional server or client components

mental ingot
#

For the people wondering, what are the currently planned changes:

  • Removal of EventNetworkChannel
  • Removal of SimpleChannel
  • Addition of Payload type registration
  • Request of known payload handlers at the client side
  • Ability to construct custom configuration tasks and process them
    • Modders will get the list of known client handlers exposed, as well as a way to send them and immediatly continue without waiting for a response (allows optional configuration tasks to be added always)
mental ingot
#

We are now querrying the remote handlers, as modmuss suggested

fair sandal
#

great

mental ingot
#

And wether or not the connection is vanilla capable will be determined based on a more advanced configuration system

#

Allthough I am still struggling to define "versioned networking"

#

Primarily when it comes to changes within a given packet

fair sandal
#

IMO each mod should be able to give a protocol version

#

and we disconnect if the protocol versions don't match

fervent token
#

which is basically what happened in 1.20.1 and earlier

mental ingot
#

You can already do that, by simply using a different proper name for that particular payload.....

errant summit
fair sandal
#

does that mean that payloads are required to match? not sure how that works

errant summit
#

having to change the name of your packet if it changes seems suboptimal

mental ingot
#

Yes obviously....

fervent token
mental ingot
#

At best I can probably offer from kind of versioning system were you supply a number per packet

#

And those have to line up

#

Actually I think I can offer something better

#

I have a design in mind

mental ingot
#

That however means I am going to need to do some modifications to our internal code

#

But the new system won't be as flexible as it is right now

mental ingot
#

Well actually

#

@opal ferry Question: Why is channel version negotiation after the configuration phase?

#

In my mind it should be before the configuration phase

#

Right after the minecraft:register and ping/pong pogo

buoyant pagoda
# mental ingot Are significantly different

not really that different -- in Forge, the *Channels (SimpleChannel and EventNetworkChannel) correspond to a channel ID in the custom payload packet
and then SimpleChannel have multiple packets through the use of the IndexedMessageCodec (which prepends a discriminator before each message payload, to identify what packet it is)

#

which is what I wanted to say earlier, before I got angry

#

according to the above and iirc past conversations I've had, other modloader frameworks (and technically vanilla) consider each channel ID on the custom payload packet as a separate 'packet' (in the sense of a SimpleChannel packet)

opal ferry
#

I do agree that it may make more sense (for future addions?) to go sooner, I can possibly change that on our end.

mental ingot
#

Well the point i am asking is that the whole "minecraft:register" dance is done to guarentee to the best of our abilities that it does not hang yes?

#

But what if two channels exist with the same id

#

But which are incompatible with each other

#

Say V1 and V2 of that particular channel

opal ferry
mental ingot
#

Yes please

#

Cause I would like to implement that before the configuration phase

opal ferry
#

Yeah, if 2 mods register a handler for the same channel, 1 will win.

mental ingot
#

Not two mods

#

One mod

#

Just different versions

opal ferry
#

What do you mean diffrent versions?

#

registerGlobalReceiver(Identifier channelName, ConfigurationChannelHandler channelHandler) is the API we have?

mental ingot
#

Well you do c:versions negotiation

#

Is that only for play channels then?

opal ferry
#

No, both configuration and play

mental ingot
#

Like the c:versions stuff?

#

Okey, but why are you doing c:versions for configuration after you have used those channels already to do the configuration?

opal ferry
#

We don't need them for configuration, they are used to prepare for the play phase during configuration.

errant summit
#

but how do you send data inside a configuration task? (which is needed for them to complete)

mental ingot
#

Like

#

YOu do the whole minecraft:register dance to guarantee that all tasks complete

#

But you do not check that the channel in use are actually compatible with each other?

opal ferry
#

The version in c:versions is basically only for the c: packets. I do think moving it before regsync and what not makes sense.

errant summit
#

so if a mod has changed a packet that is used during configuration what will happen if the server has one version and the client another?

fair sandal
#

undefined behavior on fabric

#

that's why I suggest introducing the concept of a "mod network protocol version" or whatever you want to call it, that modders can bump every time they make an incompatible change to their networkign protocol

opal ferry
#

Are modders going to be bothered to do that 😄

fair sandal
#

debatable 😄

tiny herald
#

SimpleChannel has its own protocol version, and no i havent touched mine since it was implemented 😛

image.png
#

having to bump the versions manually definitely isnt impossible to do, its just another step that needs to be thought about

opal ferry
#

Id default something like that to requiring a matching mod version. And then for the people that can be bothered have something to allow them to do what they want.

tiny herald
warm ravine
#

I have the simple channel protocol version gradle-filled-in to be exactly the mod version, because it solved many issues of server-client-with-different-version bugs that kept getting reported.

#

having the mod versions require to match would be identical (preferrable) behavior

tiny herald
errant summit
tiny herald
#

i know, the point of the message was that modders are not often bothered to change their protocol versions manually

errant summit
#

I'm myself on version 5 for my mod

mental ingot
#

I have an api in mind

#

It will work very nicely

mental ingot
#

Okey question, likely open here for for like 18 hours:
Working on the versioning scheme for the new protocol system, if you had to chose between implementing several records, all with the same interface that represent your packet, which is then passed to a single handler, or several handlers but one packet type for your versioning strategy, how would you go about it?

#

Or better said which would you prefer

#

Vote on this message with 1️⃣ to indicate a single handler with multiple message types, or 2️⃣ for the opposite, a single message type but multiple handlers. Note this is just an informal questioning.

delicate dawn
#
  1. I don't particularly care about protocol versioning because I expect people to use the same version of my mods on both sides.
spark saffron
#

Why would we drop simpleimpl?

delicate dawn
#

(but if it's about how one would contribute network-related things to neo, then I would pick 1️⃣ , I think)

mental ingot
spark saffron
#

Are you going to send packet names with each packet or negotiate an ID during the initial connection?

mental ingot
buoyant pagoda
#

do you mean getting rid of IndexedMessageCodec, and its use of the discriminator/index prepended to the message payload

#

(I mean 'message' in the sense of a SimpleChannel packet, to differentiate from 'packet' in the sense of the custom payload packet)

mental ingot
#

Yes

#

It is already gone

buoyant pagoda
#

is SimpleChannel gone, or refactored heavily?

mental ingot
#

Gone

#

But its spirit exists in the new design

#

That is what a new registration on a play packet looks like

#

ALlthough it is its internal representation

#

Basically a resloc for its id (is needed for the vanilla map), the class to type the relevant registation, and do casts related to the payload type, a handler for when the payload arrives, and a reader to read it from the network

#

I am currently designing a wrapper around this

#

That allows you to fluidly define

#

Your versioning strategy

#

While also keeping a simple registation method

#

For people that want something very similar to the simple channel

#

That has literally 0 versioning what so ever XD

#

But is usefull int he case where you do: Hey I need a need a simple packet, don't care about the rest

#

Just send it

mental ingot
#

Somebody asked for javadoc?

#

There readable in read mode

#

Already fixed some mistakes 😄

#

Yes you can obviously send packets from the client to the server during config phase

buoyant pagoda
#

i shall stab at thine javadocs once the PR's ready for review kek

mental ingot
#

I know

#

But I think it is important that we document this system very well

delicate dawn
#

noOOOO, I'll have to think of names for each packet?

mental ingot
delicate dawn
#

it's fine

#

I mean for me, maybe someone else has 200 packets an they will be like "fuck me"

timid fern
#

Packet1, Packet2, ..., Packet199, Packet200 shipit

granite mortar
errant summit
delicate dawn
#

yes I mean if they have to convert their names to resourcelocations

#

eh if it was me I'd just write a little script that will turn TitleCase into snake_case and be done with it

timid fern
#

thinkies Does Vanilla already have that method? For the NBT changes datafixer

mental ingot
#

there is an id() method on the payload

#

Which has to return the value

delicate dawn
#

I mean an external script

#

so I can paste the generated name into the code

errant summit
#

orion did you push your changes?

mental ingot
#

Not yet

#

Yesterday was a wash

errant summit
#

I noticed

errant summit
#

bump

mental ingot
#

I have not yet reached the state where I can gen patches

mental ingot
#

Okey

#

Work quited down

#

I just implemented the Negotiator

#

So that the channel can be negotiated

#

now needs to write unit tests for this :D

fair sandal
#

I had to 😄

mental ingot
#

It is fine

#

It is just a class that checks for optional channels and all the other components

#

Immediatly found a bug

mental ingot
#

Hmmf

#

Strugling to create a way to deserialize packets per connection

#

The handlers (and with that the packets that can be processed) are declared statically

#

@errant summit You areound?

mental ingot
#

Okey, further progress, every connection, now stores its Network setup, aka all available custom payloads on the connection, as an attribute. This required some modifications to the connection protocol system to support deserialisation with a context (aka what channel the deserialization is happening on)

#

The client and server now negotiate the what version of a channel it is suppose to use

#

The following rules are now used to determine if two channels are compatible:

image.png
#

Once this is done, I can reimplement the missing payloads that forge previously had.

fair sandal
#

Do we expect mods to support multiple versions of a given channel?

mental ingot
#

They can if they want to

fair sandal
#

Is it worth the complexity? Is what I am asking

mental ingot
#

It is what several people asked for

fair sandal
#

Hmmm, where? I thought people said it'd be hard to manage

mental ingot
#

@errant summit Requested it

#

And right now it is not multiple version support

#

It is support one version on each end

#

But being able to support an entire range if need be

#

So say the client has v4 and the server v5

#

You can tell it that the server supports reading v4 and v5

#

However that logic in my current implementation is borked

buoyant pagoda
#

also, what's the return type of that method? the docs aren't too clear on that

mental ingot
#

NegotiationResult

buoyant pagoda
#

and the NegotiationResult contains a nullable Component field?

#

makes me think Either<NegotiationResult, Component>

#

or possibly even DataResult<NegotiationResult>...?

mental ingot
#

We are not doing DFU types

#

We are not doing any data processing here

#

And that blocks any option in the future to extend the returned information

buoyant pagoda
#

next thonk would then be a sealed class NegotiationResult with Success and Failure subclasses

mental ingot
#

It is a record

buoyant pagoda
#

the idea i'm getting at here is to make sure that the return is statically known to be a success or a failure, without intermingling both types of results into a single object (and relying on a isSuccess() or such)

mental ingot
#

Stop trying to make this way more complicated then it actually needs to be

#

99% of this gets slattered with APIStatus.Internal

#

And then it does not matter what the return type is or is not

buoyant pagoda
#

if the NegotiationResult contains both types for succeses and failures, the return type doc for the method should just say returns "the result of the negotiation"
and the docs for the NegotiationResult record should be descriptive itself

mental ingot
#

Can we do this when I make this public?

#

And not in the middle

#

I posted the jdoc

#

To get feedback on the rules

#

Not on the design of the API

#

Which I left specifically out of it

#

So that this would not happen

buoyant pagoda
#

That fifth bullet in the docs could be simplified by being "the following conditions must all be true, otherwise negotiation fails:"

#

eh, fine

mental ingot
#

But yeah that section can indeed be reworked

#

But I realised 2 minutes after posting the screenshot

#

That my logic to base it of the preferred version is flawed

buoyant pagoda
#

for the record (heh), I suggested doing the split of the result using sealed clases and success/failure subclasses partly for nullability safety
so instead of relying on a field being non-null to know its related fields are non-null (which may be contrary to their nullability annotations), we explicitly rely on subclasses which enforce non-null values for all their fields
would be useful if the failure state has additional information, such as what channels/components failed negotiation and such

mental ingot
mental ingot
#

That gets given the channel name of the failure

buoyant pagoda
#

no programmatic access to the components/channels that caused the failure?

mental ingot
#

No, by design!

buoyant pagoda
#

even by Forge's own prettified server disconnection screen?

#

so it'd just be one lump of info in a single translatable component?

mental ingot
#

I am not aware of a screen for incompatible channels

#

Especially not since no code path on the client and server have been found by me and schurli that indicate such a thing

#

The server just triggers a disconnect message

#

But I might have overlooked it

#

That said

#

The new system allows us to technically implement this purely on the client side as well

buoyant pagoda
#

you can even see how the client and server both disconnect with the same (literal) component message about the mismatched mod channel list

#

I see you've acknowledged the disconnect the server does, but I fail to see how the other code surrounding that disconnection was overlooked

#

unless you're talking about some other disconnection message which I don't know about, which is possible thinkies

mental ingot
#

Oh yeah that piece is gone

#

It just gets a disconnected

buoyant pagoda
mental ingot
#

Well I could collect them all

#

And then send the packet....

buoyant pagoda
#

so, a regression then

#

an intended one, but a regression nonetheless

mental ingot
#

It is not intended perse

#

It is more like that the Handshake logic is gone

buoyant pagoda
#

but still a regression
and one which I'm inclined not to like, since players may be accustomed to the new screen, and may find it jarring to be replaced with a simple message

mental ingot
#

But network channels seems a stretch to mme

#

But yeah I can probably reimplement this tomorrow

buoyant pagoda
#

mod (in)compatibility is partly determined by network channel (in)compatibility

#

(other determining factors being e.g., registries data)

mental ingot
#

Yeah

#

But how do you represent that all in one setup

buoyant pagoda
#

at the very least, we shouldn't regress and hide away the channel mismatch info that we previously showed

#

particularly given iirc your previous statement on the possibility of using the mod version as the network channel version (in lieu of an explicitly set network channel version)

mental ingot
#

The network channel version is an int

#

I changed my mind on that

buoyant pagoda
#

ah

mental ingot
#

It was a lot easier to implement

buoyant pagoda
#

i expect the reasoning for moving from string versions to int versions in the PR thinkies

hasty yew
#

Given that channel version bumps inherently imply a change in the logic of parsing or encoding channel data, a single int makes perfect sense to me

#

It's like using just the first version of semver

buoyant pagoda
#

it's gonna suck for me, as someone who leverages the string version for stuff in Concord kek

hasty yew
#

Whether or not it's backwards compatible, it implies a change in the logic of what that channel is sending

buoyant pagoda
#

but I think I have a good enough workaround in mind, so continue

hasty yew
#

There's also vanilla precedent for this sort of stuff, though not networking - the resource/data pack versions

mental ingot
#

Did I miss the "disconnect" option on the damn client

#

Looks like there is no packet that is send to the server to gracefully terminate a connection

#

Wtf

#

Am I blind

#

Yeah

#

The protocol literally has no, disconnect gracefully option

#

The fuck

mental ingot
#

Okey

#

With that bullshit out of the way

#

I now implement all protocol changes needed

#

What is remaining is now all the packets and the configuration tasks

#

As well as some fixes to the negotiator to support the "modded connection" be broke screen

mental ingot
#

Okey packet filtering as modmuss described it is now implemented

#

And I documented the entire network registry

mental ingot
#

@buoyant pagoda So that custom screen will for sure get a regression

#

The best I can do is reasonable print out what is missing

#

The new system is much stricter with the data on the client side

#

And it is completely enforced from the server side

#

The client now gets a "hey: this is why you failed to connect per broken channel"-message

#

But that is it

#

Information about what mods exists etc

#

Are all not available to the client

velvet whale
#

does it not know what channels there are on the server? because that's a massive regression I'd consider a deal breaker

mental ingot
#

No

#

But the server tells it where it fails

#

The server basically now asks: Hey what channels do you have (id, preferred version, acceptable range, optional)

#

It then finds the common ground between its set of channels and the clients returned values

#

If successfull

#

It will send the a message back in the sense of: These are the channels we are going to use, with X as version (if applicable, a channel is not obliged to have a version)

#

If failure:

#

It will send a message back in the sense of: These are the incompatible channels, and for each channel the reason why, which could be anything from a mismatched version line, to a missing channel.

#

Allthough as it states right now: If there are missing channels which are none optional then the server won't do version finding

#

And only return the missing channels as failure reasons, which could potentially be changed.

mental ingot
#

Okey implemented the screen in a similar fashion now

mental ingot
#

Okey packet splitter has been implemented

#

For people wondering, this is how the packet registration API looks at the moment

#

What do you guys think?

buoyant pagoda
#

what does common mean?

mental ingot
#

Useable in both configuration and play phases

#

So a common packet here

#

Because packet splitting can happen in both phases

buoyant pagoda
#

that's gonna need clarification, because of the whole use of "common" with regards to sides, y'know

mental ingot
#

But there is a phase specific method as well

#

If you know a better term, I am all ears

#

I have no preferenence for common

buoyant pagoda
#

hmmmm, will have to think on that

#

how do you specify the network direction for the packet?

mental ingot
#

No

#

It is automatically supported on both directions

#

Because vanilla makes no pratical difference when it comes to the payload serializatino

buoyant pagoda
#

so mods cannot register packets that may only be received at one side?

#

because that's a regression

mental ingot
#

No

#

Why?

#

There is no functionality removed

#

Well the api surface is different

#

But there is no blocker to send the payload or not

buoyant pagoda
#

FML allowed a packet to have a specific network direction, to guard against the possibility that a (malicious) client sends packets of the clientbound variety to the server, causing it to crash

mental ingot
#

That is not needed anymore

#

The system gracefully deals with stupid packets

buoyant pagoda
#

i'd like an explanation, please

mental ingot
#

It just invokes the handler....

buoyant pagoda
#

that doesn't sound like it resolves the issue I just described

mental ingot
#

You need to remember that these are not packets!

#

They are payloads

#

Within the concept of vanilla's packet system

buoyant pagoda
#

you also need to remember that even in the past system, 'packets' were also just payloads carried over the vanilla custom payload packet

mental ingot
#

So?

#

Then the argument is invalid in the first place

buoyant pagoda
#

so I fail to see what situation has changed to invalidate checking payloads' reception side

mental ingot
#

In what way?

#

There is no concept of a side on the payload

#

It is just a pipe for data

#

With readers and senders on both ends

#

In what way can that fail?

#

Sure if you actually have the concept of unidirectional packets

#

Then yeah a receiving end would potentially not know how to read a payload coming the opposite side

#

But that really does not exist in this new design

#

So.....?

buoyant pagoda
#

...what?

mental ingot
#

You always have a reader on both ends

#

You say you need a check on the receiving end to prevent it from reading an unknown packet.
Yes, yes you do.
And that is handled, by vanilla, if it is an unknown payload, it will literally discard it. If it is too big, it will discard it.
So I don't quite understand what is missing in your opinion?

buoyant pagoda
#

imagine this scenario:
take a mod that registers a 'packet' (I am using the packet terminology here, but remember that this is and has always been a custom payload in the custom payload packet) which is handled by code that invokes client-only functions
in normal play, the mod expects that 'packet' to be sent only from the server to the client -- it would previously enforce this notion through a Optional.of(NetworkDirection.SERVER_TO_CLIENT) guard (which Forge enforces)

without that guard, a client may construct such a packet, send it to the server, which deserializes it and calls the handler
and the handler's reference to client-only functions will cause a crash

#

(and the Forge enforcement of the guard is "I will terminate this connection if I receive a 'packet' that's not meant for my side")

#

in your current API, as far as I can tell, you've removed that NetworkDirection (nevermind the Optional) parameter, which means mods have to manually implement that feature themselves, else they risk their packets being used for that purpose

mental ingot
#

It is indeed not in the registration, but in the handler context available

#

I am thinking

#

In practice however I wonder if that check suffices......

buoyant pagoda
#

so you'd have mods duplicate the same set of code for every packet they have, rather than having this option be built-in to (and enforced by) Forge itself?

mental ingot
#

I mean especially in SimpleChannel the callback is a functional interface

#

Referencing the method alone would trigger the classload right?

#

I mean during registration

buoyant pagoda
#

the side validation in Forge is done directly after it finds the associated NetworkInstance for the payload ID

mental ingot
#

I know

#

But that is not what I am referring to

#

SimpleChannel currently accepts a lambda to the handler of the packet

#

Correct?

#

Using a functional reference alone would trigger the classload and as such the above scenario

#

And immediatly crash the server on boot

buoyant pagoda
#

yes, mods still bounce their client-only functions to a bouncer as per recommended modding practices

mental ingot
#

Okey

buoyant pagoda
#

but irregardless, that means you're passing the buck of handling improperly sent/received packets from Forge itself to every single handler

mental ingot
#

So if they already need to do the whole if (client) { doClientShit() } in each handler anyway

#

There is no real loss then

#

I understand your concern

#

But that system alone would increase the complexity massively

#

And I am considering trying to balance that vs its needs

#

Especially, given that each modder already needs to do the above if-check anyway

buoyant pagoda
#

they don't do a if (client) -- most mods (as I believe) just invoke doClientStuff() directly in the handler body, on the basis that their packet will only be invoked on the proper side due to Forge's validation

mental ingot
buoyant pagoda
mental ingot
#

Sure it does

steady sonnet
#

re: method name - perhaps rename common to anyPhase since it refers to phases rather than sides?

indigo hazel
mental ingot
#

Ahh right

#

Okey

#

Then I will need to think about this

indigo hazel
buoyant pagoda
#

and note that Forge's enforcement of the side is to terminate the connection -- because if a client is sending the wrong packets, it should be let go as its trying to crash the server (intentionally or not)
you would suggest mods either silently discard packets, or manually implement disconnect code

fair sandal
#

common is the name mojang uses, iirc

mental ingot
fair sandal
#

Fine by me, people can also read javadoc 😛

mental ingot
#

I thought that a static call is not sufficient

#

It has to be a WRAPPED static call

#

So if you did something like this:

#

Handler::onReceived

#

That it would still classload the static method

#

Because it is not wrapped

steady sonnet
#

no, it is super lazy

mental ingot
#

Okey

#

So it is extra extra lazy

buoyant pagoda
#

static void handle(NetworkEvent.Context ctx) { ClientDoer.clientStuff(); } with a Packet::handle as the handler
that's what I meant by "just invoke doClientStuff() directly in the handler body"

mental ingot
#

Well that means I need to investigate this shit some more

indigo hazel
mental ingot
buoyant pagoda
#

funnily enough, we had a discussion on classloading in #maintenance-talk, which you did pop in for

steady sonnet
#

sometimes I've wondered if the whole sidedness issue should just be solved by shipping client classes on the server lol

mental ingot
#

But sci somewhat rightly says that it should not be so verbose / copy-paste for this specific case

mental ingot
#

Check the binary patches for once 😄

#

You will notice some weird shit being created when you install a server

buoyant pagoda
#

plus, the whole "wrong-sided packets should cause connection termination, because naughty client"

hasty yew
buoyant pagoda
#

(if a mod does that intentionally somehow, that's a very big bug on the mod's part)

mental ingot
mental ingot
steady sonnet
#

oh so the whole RuntimeDistCleaner thing is just a massive hack to emulate client classes not being present? 😛

mental ingot
#

I mean does that then mean that a single sided connection like this, requires different processing for version compatibility

buoyant pagoda
mental ingot
#

Or does it only matter for whena packet arrives

steady sonnet
#

I think if someone changes the sidedness of a packet from one version to the next, that is their problem

indigo hazel
buoyant pagoda
hasty yew
indigo hazel
mental ingot
#

I mean i could always check if the payload has the same sidedness set on the client and the server

#

.....

#

Okey, give me like 20 minutes

buoyant pagoda
#

ideally, 'packets' (custom payloads, but essentially packets to mods) should be done in the same form as vanilla -- a packet is sent by one side and received by the other, with the reverse being invalid
and two separate packets exist for things that need sending from either side

mental ingot
#

Need to dig up the API and do the check

mental ingot
#

I just discard invalid packets

#

Which is the same vanilla does

#

It does not do the disconnect

buoyant pagoda
#

but that is different from what Forge did, which was disconnect for an inappropriate (known) packet

mental ingot
#

Forge only did that for MODDED packets

indigo hazel
mental ingot
#

if you send an invalid payload

#

You get a DiscardedPayload instead

#

Which is considered unknown

#

And just logs an entry in the log that it received an invalid payload

#

Without further processing

buoyant pagoda
#

as far as vanilla is concerned, a custom payload packet with an unknown payload ID is no biggie (maybe the server is trying to see if the client knows that ID and replies back, or the same with the client to server)
an unknown packet, on the other hand, is a connection termination

mental ingot
#

Yeah

#

But since we do not have the concept of "packets" anymore

#

Only payloads

#

I followed what vanilla does

#

And am not terminating if somebody sends shit to any end

#

But just discard it

buoyant pagoda
#

in the context of mods, payloads are packets -- it's more that we build a networking tunnel consisting of packets between the sides through the custom payload packet mechanic

mental ingot
#

Well not anymore!

#

I tend to agree that in the past, that the NetworkInstance concept behaved more like a Packet

#

I will see what I can do

#

But no promise on the termination

#

I will however add a check for the correct expected flow

buoyant pagoda
#

they were and still are, though
there is no practical difference to a mod between the concept of a 'payload' and a 'packet'

mental ingot
#

You are kind of right

#

And kind of wrong

#

But that is the problem

#

Mojang has made this area unclear

#

With their own use of payloads as packets

#

And vice versa

buoyant pagoda
#

I fail to see how I am wrong in this regard

mental ingot
#

Well there is a clear difference in how mojang treats packets

#

Vs how it treats payloads

#

The new system allows the modder only to define Payloads. Not packets

#

Even if that was what you consider it to be in the past

#

But to be 100% perfectly clear: They are payloads. And mojang simply does not terminate on mismatched arrival

hasty yew
#

I think the question here is this: are these payloads, as modders would likely use them, doing the sorts of stuff vanilla does with packets or what vanilla does with payloads?

mental ingot
#

See that is the core question: and the answer is both

#

Because mojang has started using the payloads themselves on the newer version, for what would have been in the past a custom packet

#

That is what I ment with mojang has muddied the water

indigo hazel
#

I would say there is a clear difference: vanilla does not use the custom payloads for mission-critical packets, mods however do

hasty yew
mental ingot
hasty yew
mental ingot
#

Mojang only transfers debug information using the payloads

#

If I can make the disconnect work

#

I will do so

#

But that might not be possible

indigo hazel
#

Assuming you know the intended side-of-receipt, it should be trivial since the packet listener (i.e. ClientPacketListener) gives you access to the Connection on which you can just call disconnect() with a reason

mental ingot
#

the listener never gets invoked if the packet is technically invalid

#

Because it has been discarded remember

#

So I would still need to parse the packet

#

Even if it is invalid

hasty yew
#

Poking at prior art here, fabric also has something like this implemented, right? Do they just stick with the vanilla behaviour on unknown payloads and discard it, or do they bother to include that logic to disconnect?

indigo hazel
mental ingot
mental ingot
#

Technically vanilla says: if payload invalid then discard

#

Not even parse it

#

Just completely discard it

indigo hazel
#

Well, yeah, but the point here is to specifically handle known packets which are received on the wrong side and in that case you should end up in a state where the payload has gone through the listener and is in your custom infrastructure such that checking this while having access to the Connection is possible

mental ingot
#

Okey I think I figured it out

errant summit
#

after reading the conversation:

  1. the concept of sided packets is there since some packets are only registered to one of the sides of the packet set
  2. a safeguard that a packet can only be sent one way would be good because for a unidirectional packet you don't want to have to check if it is actually received on that side
mental ingot
#

I already added it

mental ingot
#

Okey, completely implemented the flow processing logic.

mental ingot
#

Okey reimplemented the packet distributor

mental ingot
#

And all remaining core features are implemented

#

Missing now are the packets

#

So I hope to have those by tomorrow evening

indigo hazel
#

For the packets I have one single wish: no more mega classes per phase holding all packet types for that phase as inner classes

#

(assuming you are talking about Neo's own packets like the open container stuff)

mental ingot
#

There won't be 😄

#

They are all records

#

In individual files

indigo hazel
#

👌

mental ingot
#

But really the amount of packets is probably 6

#

2 for negotiation

#

1 for split

#

1 for registry

#

1 for spawn entity

#

1 for open container

#

And that is it

#

Oh and potentially one for the extended server status

#

I think I found a way to make the server status not use the stupid json hack

#

With the compression

#

So that it should result in a much better experience and implementation

#

But I still need to test and research it

buoyant pagoda
errant summit
#

orion can you push the working state and open a draft PR for it (so we can track progress)

mental ingot
#

Cause right now I still can't

errant summit
#

I thought you have the system itself working and only need to do the usages?

mental ingot
#

And you can only generate patches

#

If everything compiles

errant summit
#

so it is the neo source and not the mc source that is currently not compiling

mental ingot
#

Yes

#

But it still requires a compileable environment

errant summit
#

(just comment the usages of the payloads out to gen patches)

mental ingot
#

That is what is missing 😄

#

Okey

#

Hold on to your hats

#

Actually that won't happen

#

I have over 100 erros

#

@errant summit Ever seen this:

image.png
#

Nearly all of those 100+ errors are this

#

And I don't understand what it wants from me

#

Fixed

#

Something I did 😄

velvet whale
#

(for better or for worse)

mental ingot
#

It failed for me

#

But I was also tweaking NG7 to deal with some shit

#

Realistically it does not matter at the moment

#

I am down to less then 30 errors

#

Implementing them is trivial

velvet whale
#

it works

mental ingot
#

Ah okey 😄

#

Nice

buoyant pagoda
#

threw me for a loop

#

until I reread it and saw the error in the word

mental ingot
#

@errant summit I just pushed the feature branch

buoyant pagoda
#

oh lawd, it's a feature branch

mental ingot
#

I am still working on the payloads and the feature branch

mental ingot
fair sandal
#

can you open a PR so that we can start leaving review comments?

marsh loom
buoyant pagoda
#

sadly, I have naught a euro to my name

mental ingot
fair sandal
#

you can also open a draft ¯_(ツ)_/¯

mental ingot
#

That is simply not how I work....

mental ingot
#

@buoyant pagoda This now supports directional filtering

#

Using the flow(PacketFlow) and bidirectional calls

#

It is possible to tell the registrar that any following new packet registrations are only allowed to flow in one particular direction

#

Next to the fact that it is additionally possible to now specify a handler for a given direction if you do not want to use a common handler

#

That gives the modder now 2 different routes to deal with packets arriving at the wrong side

#

If the modder uses the flowing(PacketFlow) and bidirectional() methods as shown above, then the packet won't be parsed at all, but the client will be disconnected if the packet arrives at the wrong side.

If he or she instead forgoes that option and uses sided handlers, while only registering a handler for a given side, that means that the packet parser will be invoked on both sides, but that the handling is discarded and no method call will be made on the invalid side

#

This should provide enough flexibiltiy to allow for nearly everybody whishes to be fullfilled

fair sandal
#

@mental ingot the TC build is intended right?

mental ingot
#

Yeah

#

I want this eventually to be in hands for people to test

fair sandal
#

alright alright... then I don't have to kill it 😄

mental ingot
#

It will fail untill I fix all the compile errors

#

But untill then it can just chuck away at it

fair sandal
#

👍

mental ingot
#

Okey added the check for the enforced variant of packet flow management now also into the negotiator

#

This should as such now also show up nicely in the mismatch screen

#

Okey I am of to bed

#

Tomorrow I need to figure out how to get the negotiated channel version into the write handling of payloads

buoyant pagoda
# mental ingot

some thoughts at present:

  • that call to flowing/bidirectional doesn't look... natural to me? as in, how they affect subsequent calls to the registrar looks like a possible point of misconfiguration

  • what happens when a payload configured for e.g. CLIENTBOUND flow only has a server handler? do we gracefully handle that?

  • do we really need to have that whole specification on what direction those payloads flow? could we not just infer it from the handlers -- if a handler is not present for a payload on reception, then assume it was sent improperly and terminate the connection

    in that way, those who want the behavior of 'if received on the wrong side, discard and continue' should just need to make a no-op handler, which is better at communicating the intention of the dev (that they wish to discard if it's received on that side)

#

the flowing/bidirectional seems like unnecessary additions to the API, when we can infer the directionality of a payload based on the existence of handlers for the receiving side

#

plus, the API wouldn't need to confirm that e.g. a clientbound payload has a handler for the client-side, and not a handler for the server-side (since it will never receive it on the server)

#

i.e., remove flowing/bidirectional, and change behavior such that receiving a known payload (i.e., the ID is known to us) but with a missing handler causes a disconnection

buoyant pagoda
#

how long do you think before the networking refactor is ready for review? I'm considering going with XFact's suggestion in https://github.com/neoforged/NeoForge/issues/205 of the band-aid fix, in order to fix vanilla<->NF compatibility at present while this refactor is still ongoing

mental ingot
buoyant pagoda
#

custom entity with additional payload?

#

it sounds...familiar

#

but I can't quite put my finger on the name of the class

#

anyway, if the main substance of the rework is finished and all that's left is hooking up NF-added APIs to use it, consider making a draft PR so we can look at the details of the new APIs and impl. while you finish up the last few bits

mental ingot
# buoyant pagoda some thoughts at present: - that call to `flowing`/`bidirectional` doesn't look...

So the chained call is not needed a modder can do this as well:

    @SubscribeEvent
    public static void register(final RegisterPacketHandlerEvent event) {
        final IPayloadRegistrarWithAcceptableRange registrar = event.registrar()
                                                                       .withVersion(buildNetworkVersion());
        
        registrar
                .flowing(PacketFlow.CLIENTBOUND)
                .configuration(
                        FrozenRegistrySyncStartPayload.ID,
                        FrozenRegistrySyncStartPayload.class,
                        FrozenRegistrySyncStartPayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                )
                .configuration(
                        FrozenRegistryPayload.ID,
                        FrozenRegistryPayload.class,
                        FrozenRegistryPayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                );
        
        registrar
                .bidirectional()
                .configuration(
                        FrozenRegistrySyncCompletePayload.ID,
                        FrozenRegistrySyncCompletePayload.class,
                        FrozenRegistrySyncCompletePayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                                            .server(ServerForgeRegistryHandler.getInstance()::handle)
                );
    }

or:

    @SubscribeEvent
    public static void register(final RegisterPacketHandlerEvent event) {
        event.registrar().withVersion(buildNetworkVersion())
                .flowing(PacketFlow.CLIENTBOUND)
                .configuration(
                        FrozenRegistrySyncStartPayload.ID,
                        FrozenRegistrySyncStartPayload.class,
                        FrozenRegistrySyncStartPayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                );
        event.registrar().withVersion(buildNetworkVersion())
                .flowing(PacketFlow.CLIENTBOUND)
                .configuration(
                        FrozenRegistryPayload.ID,
                        FrozenRegistryPayload.class,
                        FrozenRegistryPayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                );
        event.registrar().withVersion(buildNetworkVersion())
                .bidirectional()
                .configuration(
                        FrozenRegistrySyncCompletePayload.ID,
                        FrozenRegistrySyncCompletePayload.class,
                        FrozenRegistrySyncCompletePayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                                            .server(ServerForgeRegistryHandler.getInstance()::handle)
                );
    }

So it is completely up to the modder to chose the format he or she wants to use. They can store the registrar in between, call it in one long chain, or store it. In practice it is just a builder for the internal registation record.

So with respect to a CLIENTBOUND flow payload with only a server handler: If it arrives at the client it will be discarded and nothing happens, if it arrives at the server it will not be parsed from the friendly bytebuf and instead disconnect the server, never invoking the handler. It might in deed be better to create some feedback during the setup loop to ensure that modders don't shoot themselves in the food.

(1/2), next message will follow.

#

The API is specifically designed, to not have to assume anything. You don't need to use the:

.flowing(PacketFlow.CLIENTBOUND)
                .configuration(
                        FrozenRegistryPayload.ID,
                        FrozenRegistryPayload.class,
                        FrozenRegistryPayload::new,
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                );

Syntax, something like this is also possible:

.flowing(PacketFlow.CLIENTBOUND)
                .configuration(
                        FrozenRegistryPayload.ID,
                        FrozenRegistryPayload.class,
                        FrozenRegistryPayload::new,
                        ClientForgeRegistryHandler.getInstance()::handle
                );

The aim with this registrar design is to not lock the user into a specific design or use pattern. In practice it can supports all.

So I did not want to assume anything from the handles which are registered.

The handlers -> based api, is in practice just a call to the second API above, with a handler that has this code defined:

    @Override
    public void handle(ConfigurationPayloadContext context, T payload) {
        if (context.flow().isClientbound()) {
            if (clientSide != null) {
                clientSide.handle(context, payload);
            }
        } else if (context.flow().isServerbound()) {
            if (serverSide != null) {
                serverSide.handle(context, payload);
            }
        }
    }

for a configuration packet.

errant summit
#

eh I don't like the current builder design, it somehow feels clunky

mental ingot
#

But I am not exposing the raw underlying registration object

#

Because that is internal API

#

And if we ever change something related to the networking registration code

#

I want to have that ability even in a breaking changes window

mental ingot
#

But I think there is a group of people which will do versioning

#

Some will do enforced flow

#

Some will do optional processing

#

Some will do exact phase packets

#

Others will be, meh screw it, i will just register a common packet

mental ingot
#

And at least you know can get into the world

#

I have a stupid feeling i missed something

#

And might need to refactor shit.....

buoyant pagoda
#

i still feel like this is overreaching...? in its attempts to have a 'flexible' API

when instead we can restrict it a bit (require the handler to be specified to client and/ server, for lack of ambiguity)
and derive a benefit from it (naturally concluding that if a handler is not present, the payload must not be intended for that side),
and have no loss of flexibility on the user side (if a payload should be received but discarded, then register a noop handler -- or even provide a specific method for that, if we want to avoid deserialization in that case)

#

i don't see a practical benefit of a 'common' handler (as I assume the one with no handlers -> means), because (a) I do not forsee many mods reusing the same packet for both network flows, and (b) even in that case, mods can just declare the same handler for both sides -- making their intention more explicit (that the payload is intended for processing on both sides)

mental ingot
#

Realistically I will just be doing:
event.registrar()
.play(
FrozenRegistrySyncStartPayload.ID,
FrozenRegistrySyncStartPayload.class,
FrozenRegistrySyncStartPayload::new,
ClientForgeRegistryHandler.getInstance()::handle
);

#

And go home

delicate dawn
#

I have never liked the idea of an unsided packet, in almost all cases it should be two packets because each direction serves a different purpose

mental ingot
#

Why?

spark saffron
#

If you have to pass a class you shouldn't need to pass an ID, and/or vice versa

mental ingot
#

Forge uses them now, but has a spplit handler

mental ingot
#

And the resourcelocation is needed for lookup

spark saffron
#

Then use the class for lookup

#

or don't cast it

mental ingot
mental ingot
hasty yew
#

Why do you need a cast? Can't it be generic so the consumer (silently) casts without having to see anything

mental ingot
#

Something needs to make the cast to your specifically typed handler

#

And that needs to happen in the internals

#

Before I invoke your generic handler

delicate dawn
#

nah you just ignore the generic and use raw types internally

mental ingot
spark saffron
delicate dawn
#

the type is erased so the receiver will cast again regardless

mental ingot
#

But so dirty.......

buoyant pagoda
mental ingot
mental ingot
spark saffron
#

Other question, why is packet registration event-driven at all?

#

seems unnecessary

mental ingot
mental ingot
#

You are not supposed to register packets outside of your namespace to prevent collisions

buoyant pagoda
#

I still question its purpose
since I find it difficult to believe that there is some common realistic use-case that involves a mod which exchanges the same structured data (in form and in use) between the client and server

mental ingot
#

In the particular case here it is an empty packet

#

It is an aquital packet

#

That indicates that the server send all registries to the client

#

And the client acknowledges back that it received them

spark saffron
#

magic static context is a horrible anti-pattern

mental ingot
#

It is not static

#

Well it is

spark saffron
#

lol

mental ingot
#

Nvm

#

Give me another design that prevents Mod A from registering shit in Mod Bs namespace

delicate dawn
# mental ingot But so dirty.......

yes but since presumably the owner provides both the deserializing factory and the handler (if it works like somplechannel works now), their <T> will by necessity be consistent, so you can internally ignore the generics altogether in the registration record without loss of consistency. dirty, yes, but a few suppresswarnings shut the compiler up abd then you look away. ;p

mental ingot
spark saffron
mental ingot
#

Its dirty but it works....f..

spark saffron
#

Just let people create a helper object with their modid and have it automatically prepend it to created objects

buoyant pagoda
spark saffron
# 
var reg = new MessageRegistrar(MODID);
reg.whatever(name, [packet info]);
mental ingot
# spark saffron We don't need to care about this. Babyproofing like that is what gets us to "po...

Babyproofing is fucking needed here!

I had no versioning and no sidedness checks in the firstplace in this API. And people complained that it is possible to crash the server by sending unknown or not well structured packets to the server by a malicious or broken client!

If you can register shit to somebody else, you can get very undefined and unexpected behaviuor.

So either we baby proof shit with a proper API, or we don't do it at ALL. But not this in between shit

spark saffron
#

We shouldn't do it at all

#

it doesn't prevent anyone from doing it

#

it just makes a bunch of noise

mental ingot
#

I was of that opinion

hasty yew
spark saffron
#

But also you should not be able to crash the server by sending malformed packets, that indicates a lack of security on the serverside

#

the bad packet should be discarded and the connection terminated

#

(as is the current case, iirc)

hasty yew
#

If it's just a warning it's not much use and just causes annoyance

mental ingot
hasty yew
#

If it's an error it makes legitimate use cases painful

spark saffron
#

Well we can fix that :p

mental ingot
#

And I agree that a simpler API, without versioning, without Sidedness is much simpler

#

It would allow the same systems

#

But only implicitly

#

But then again I point to scis and mine discussion in this thread a couple of days ago......

spark saffron
#

Though wait a second, can you just crash a vanilla server with a bad packet?

mental ingot
spark saffron
#

That should be considered a serious flaw in their design

mental ingot
#

Send it a custom payload packet

#

With a known id

#

And something that needs to parse a map

spark saffron
#

is there a mojira ticket on that? That's an insane oversight

mental ingot
#

And make the map pretty large

buoyant pagoda
#

I had no versioning and no sidedness checks in the firstplace in this API. And people complained that it is possible to crash the server by sending unknown or not well structured packets to the server by a malicious or broken client!
on the point of the sidedness check: that is a regression in your draft of the API, with respect to the current API, which was properly raised in order to prevent that regression and have a redo of that commit which added the packet network direction feature in the first place

mental ingot
#

It won't crash

#

But you can get the network threads to starve for sure

mental ingot
#

Vanilla does not do this

spark saffron
#

I think w.r.t. the current api (simpleimpl) most people don't register a direction bc its optional

#

and tutorials glance over it

mental ingot
#

And there is literally no reason to do it in the first place......

buoyant pagoda
#

it is literally impossible to send a clientbound packet to the server

mental ingot
spark saffron
#

I am of the opinion there should be no double-sided packets, modders tend to be bad enough with designing packets as-is

#

You should be forced to set the net direction on packet registration

errant summit
buoyant pagoda
spark saffron
#

Other question, why not just keep simpleimpl

mental ingot
spark saffron
#

You could make it work though

#

its an api

mental ingot
#

I could, but then I also need to make event work

#

And that is a whole thing

#

And you need a different simpleimpl for when you want config packets

#

And a different event for that phase as well

#

It gets you in a whole mess

#

The point is to have one entry point

#

Where you can see

#

I have one packet

#

And I need to set it during config or play

#

That was literally all the original api had

#

Very simple

#

Yes it had no such thing as side enforcement

#

Or versioning

#

And I am still of the opinion that neither is needed

#

I understand your hesitation with respects to the event

#

Might I ask for a usecase to register in a different mods namespace for packets?

#

I could trivially give you a method to create one for something other then your namespace

spark saffron
#

There isn't one (that I could reasonably think of), I just don't think its worth the time or engineering effort to write a bunch of lockdown code like that

#

The modid-implied context thing available to the mod bus events is poor design and should be abolished in general

mental ingot
#

I just fire it in a different bus, with a different bus entrypoint..... It is not more or less difficult

#

But I mean given that it is trivial to add

#

If somebody comes up with a reason to have it

#

I can easily add it

hasty yew
mental ingot
#

And fallback to the current mod if callers don't care

mental ingot
errant summit
#

also registering payloads to other mod ids should be possible (see WorldEditCUI)

mental ingot
#

Question: Is the mod bud parallel?

#

No right

spark saffron
#

The short of my point is that anything using ModLoadingContext.get().getActiveNamespace() is poorly written and should be removed

hasty yew
mental ingot
#

It is a serial dispatch

#

It should be trivial then

#

I can just add a method to the event

hasty yew
mental ingot
#

That allows you to get a registrar for an arbitrary namespace

#

And if you don't supply a namespace

#

It will just fallback to the active one

hasty yew
#

Context dependent namespace is screm

spark saffron
#

just do not fall back

#

force providing your modid

mental ingot
#

No?

#

We have infrastructure for this

spark saffron
#

it is GARBAGE infra

#

it is horrible design and should never have existed