Networking Protocol

[iron ivy]

I suggest going over the chat histroy where Modmuss first commented on the new spec

[errant summit]

you mean the one we analyzed that only does channel and protocol version negotiation at the start of the play phase?

[fair sandal]

I think it's easier to not have it in the spec at all

No, fabric has a different protocol that handles splitting just for regsync, and also uses some optimized byte format to avoid repeating namespaces

[iron ivy]

That was discussed above

[errant summit]

so you are specifically talking about their implementation of reg sync (please clarify next time)

[fair sandal]

Fabric also doesn't do packet splitting, so it has to handle the splitting at the regsync level

[errant summit]

i think we should discuss the reg sync protocol with modmuss and find something that is robust and fits all our needs

[velvet whale]

that's a bad example, the thunderbolt & usb4 situation is horrible regarding spec compliance and 'optional' stuff

[errant summit]

well then I'll use HDMI for my examples next time (it also has a lot of optional stuff)

[fair sandal]

I don't think that fabric will ever have payload flow or versioning either, so I'd place all of that in the neoforge namespace

We can easily check if the other side supports the c: negociation channel and fall back to neoforge: otherwise

That's how we evolved the fabric regsync protocol some time ago while maintaining backwards compatibility

[errant summit]

that was discussed with modmuss, fabric will send everything as optional, unspecified flow and versionless

[errant summit]

why introduce complexity in the implementation just to remove something optional from the spec?

[fair sandal]

It's not an agreed upon spec

You're basically forcing a spec onto the others

[iron ivy]

The whole point of this discussion is to come to a consensus and agree upon a spec.. So yes, it is not agreed upon, yet

[mental ingot]

My response to the discussion held yesterday can be found here: https://hackmd.io/@4pEru98VR-2vMVSQF0Jilg/HJfc_7FYp

[fair sandal]

I would say the 4th change is unneeded if we already do the packet channel versioning. I will also request that anything that fabric does not agree to support be moved to the neoforge ID. Configs in particular must be moved to the neoforge id. For the rest, do whatever.

[iron ivy]

The 4th change is needed, the server only knows if a mod is installed, if it has a channel. Which that change is specifically to allow the server to know the entire mods list

think, Paper banning clients from joining with xray mods installed, etc

[fair sandal]

Final request: NeoForge mods should have a way to say they will only accept a NeoForge server but not any other loader. This is to ensure that the user gets a proper error message.

[fair sandal]

This was already discussed some time ago and deemed unsuitable. It's weak protection at best. Ask @opal ferry for the details.

[iron ivy]

There are more use cases than just stopping xray mods, it was a single example

same concept, its data currently accessible by servers, used for many purposes, it should be retained

[obsidian nova]

For any hacks, it's trivial to remove it from the sent list, so basing security on it is useless

[iron ivy]

Correct, not for security. The server can do all sorts with the list, blocking xray/hacks is purely an example of the direction to think. Blocking mods is the most common case though, not specifically hacks, minimaps, broken content mods, unfair client-side mods, etc.

[mental ingot]

An example of why this might be usefull, is to allow the server to say: Hey you should not connect with "Immersive Engineering 4.3" it contains a bug that crashes the server

But "Immersive Engineering 4.4" is fine

[obsidian nova]

Thats a questionable example, because a malicious client could fake the version easily

[fair sandal]

It seems very redundant with channel versions

[mental ingot]

Again it is not really a security idea

It is more for content management

Server owners have indicated that they would want to maybe use it for security

[obsidian nova]

I don't personally want my mod list sent to the server

[mental ingot]

Why not?

It already is today

[obsidian nova]

I consider the mod list I use to be my own personal information that the server doesn't know about

[mental ingot]

LexForge, NeoForge and some Fabric mods already do this

And have been doing this for forever

[obsidian nova]

And I don't play public Neo servers. I trust the server owners I play with

[dim wagon]

lol

[mental ingot]

Okey, but that does not matter

[obsidian nova]

I play public servers with my Fabric setup

[mental ingot]

Which also partially sends this list?

So what is the issue?

[obsidian nova]

It doesn't

[fair sandal]

Fabric only sends the channel list I think

[obsidian nova]

It sends the packet IDs

Yeah

Not the mod list

[mental ingot]

I said, that some mods do it

Fabric sends the channel list yes

[obsidian nova]

I don't think it's the server's right to know which version of Sodium I'm on, for example

[fair sandal]

It seems very redundant with channel protocol versions, is what I am saying. I'd pick one or the other, not both

[obsidian nova]

Or if I use Falling Leaves or Not Enough Animations

[mental ingot]

It achieves two different things

But I have no issue with making it optional

Forge will send it

It did in the past

And we hjave requests to bring it back

Because it is in use

Some of the uses indeed can be replaced with the channel list and its versions

Others can't

[velvet whale]

what uses can't? version channels are what have historically been used for compatibility checks

dummy channels for versions are what has quite literally been recommended for version checks since simplechannel became a thing

[mental ingot]

No actually, the compat check has been performed on the mod list, and on the network list. The mod list was used in the past via the DISPLAYLIST extension, which is now not used. While the channel list was used by the channels own version checker

[velvet whale]

the ping compat check is different

[hybrid prism]

3. We will update the state id sync component of the registry id sync to follow a similar pattern, there are reasonable compression gains that can be made following the same kind of style.
   Note that there are various mods that affect blockstate properties, e.g. I've seen a Fabric mod that adds extra distance to vanilla scaffolding via a new property, but has the further states placed at the end of the blockstate list so vanilla ids aren't shifted around, while syncing them to clients as one of the vanilla scaffolding states, allowing e.g. vanilla clients or modded Fabric clients without the mod to join a server that has that mod on it.

[velvet whale]

and that doesn't exist anymore and was shut up in the past by a lot of mods anyways

(since people didn't mark their mods correctly as side-specific)

[mental ingot]

I will think about it

You have a point

[mild spruce]

Thanks Orion for the long post, cleared a lot up and makes sense.

[mental ingot]

Good. That was the point, We have invested a lot of time to figure this out. And it seemed people were missing information and I hoped with these long messages / posts it cleared up how we got to were we are.

[whole hare]

you should pin it maybe so that it doesnt get lost because chat

[mental ingot]

Shite I deleted the first message

For fucks sake

[delicate dawn]

@mental ingot sorry for ping. is it supposed to be public?

[mental ingot]

Yes...

Hold on

[timid fern]

shouldn't logger have them?

[mental ingot]

Update the message

With a working linke

I hope

Can somebody check?

[iron ivy]

403

[timid fern]

still getting 403

[mental ingot]

Try again?

[iron ivy]

yep

[mental ingot]

So it works?

[iron ivy]

yes

[errant summit]

orion I think 1 - 3 of your points are good 4 is not needed imo but we should make config sync into a neoforge: packet

[mental ingot]

A comment from somebody who requested it: "Another use case could be mods which alter vanilla behaviour, which are required to exist on both sides, but don't add any registry entries, or network channels"

[fair sandal]

Is the block state syncing going to be separated from the rest of regsync?

[fair sandal]

The easy fix is to just add a dummy channel 😛

[mental ingot]

Yeah but should not have too. This is not an entirely uncommon scenario and I think it is worth catering for. Even if it is like an optional part

[steady sonnet]

Blockstate syncing is weird because they are not in a regular registry like literally everything else

[mental ingot]

Given that this technically already effects more then just the blockstates, makes it reasonable to put it in the same packet as optional data

[errant summit]

ok yea that makes sense

[mental ingot]

But a seperate packet is possible tech

[fair sandal]

What if they have multiple plugins with versions that match?

[mental ingot]

We have to draw the line somewhere. Multiple plugins with the same or different version is so niche that that is for me beyond the scope of this iteration. But if it becomes a problem then we need to address that yeah in the future

[fair sandal]

I don't like the idea of having two parallel systems with a lot of overlap

[mental ingot]

They serve different purposes

The versioning on the channels is functional

While the mod information is for me at least in the way I see it informational in that it has no direct effect on the network protocol

I was thinking of making that optional anyway

So that neoforge can send it

If it wants to

[errant summit]

what two systems are those?

[mental ingot]

The versioning

One system for versioning channels

One for mod information

[steady sonnet]

That's ugly but also logical

[errant summit]

I would not do versioning on mod level only existence

[mental ingot]

Yeah but the uglyness is what tech does not like

[mental ingot]

Maybe....

The idea was that server owners could block a range of mods and version that cause issues

But I have not decided yet

[steady sonnet]

I feel like it should be enough to provide versioning at channel level

If we're being honest the main use case of mod versioning is going to be to block cheat mods

And they can just modify the client to not send the version anyway

[errant summit]

yea no I'd just have the client send the modlist and that's it

[steady sonnet]

why is the modlist necessary?

personally I prefer the architecture of making an optional unused channel if you're changing behavior in a way that matters for client/server interaction

[errant summit]

yea that is probably the cleanest solution

[steady sonnet]

it makes it clear that the set of network channels & registries are what dictate behavior

[obsidian nova]

Yeah, that's what I personally do on Neo

On Fabric I do some funny manual handshaking to make the same thing happen

[mental ingot]

Several reasons mods that alter behaviour but do not have networking (mixins etc).

Server management of connected mods (yes hackers will evade, but it is more ment to be able to say, don't connect with IE 4.4, connect with 4.5 etc)

Displaying that information on the mods list screen. Now that the system does not require mod versions to line up, having this displayed somewhere in the client is a good idea

Those were the initial three reasons why we should send it

[obsidian nova]

If those mods want to broadcast themselves, that's fine

I don't want my mod list sent

That's my own business

[errant summit]

yea no, no version checking on mod level if necessary only send the mod list but I don't think that to be good

[obsidian nova]

And not that of the servers I join

[steady sonnet]

I mean I can see some potential 'weird' issues server support people will get if players connect with an older version of a mod that happens to be network-compatible

[mental ingot]

Yes

[steady sonnet]

but I don't know if that's a good enough justification to send the whole mod list

maybe allow mods to opt out of being sent

[obsidian nova]

I think mods should opt in, not out, if that kind of feature is included

[iron ivy]

Whats the solution for mods which want to enforce that the client and server side of their mod have the same version? Is the channel version a string or an int?

[steady sonnet]

make your channel version your mod version

[mental ingot]

String

[iron ivy]

thats fine then

[fair sandal]

Quilt had the idea of modpack version as well

To reject the handshake if the client has a different modpack version

[mental ingot]

Maybe i mean that is the same trend is it not

[fair sandal]

Same trend yes. Could easily be a dummy c:modpack channel as well

[mental ingot]

I personally think it is worth while to have such a system

But not as a hardcode requirement

I think that goes to far

And with system I mean the modlist/modpack version sync

I think it is nice for forge to have

To be able to show the versions of the mods in the server console, as well as in the clients mods list

But it is really not a requirement for me for that to be part of the protocol

[fair sandal]

Idk, there's also some more complex social questions around showing the mod list (e.g. client has a mod that you "dislike" for political or such reasons)

[mental ingot]

I would argue that that is up to the system it self

It is already trivial to do such enforcement even without

By requiring a special "mod" to be instlaled

Which sends this over

But I get your point

On the other hand, we have this exact information being exchanged today

For the same reasons

And people really have not batted an eye

Hell they cheared it on

When the new disconnect screen with information based on that list was added

[fair sandal]

Wasn't that the display test? Or another completely different system

[mental ingot]

Mix of both

[errant summit]

the question is the modlist information that is gdpr protected?

[fair sandal]

(asking because these systems might have been opt-in)

[mental ingot]

But it used both the mod list packet and the display test system together to produce information in a large screen as to why you could not connect

[fair sandal]

Sending the mod version for any mod that registers a payload makes sense imo

[mental ingot]

Like the server could send you a upgraded disconnect packet

With a "we do not like your mod" message

Or "this mod X is incompatible"

[steady sonnet]

This is a reasonable hybrid solution

[mental ingot]

and people really really liked it

[errant summit]

you mean a disconnect packet with a message

[fair sandal]

It's a bit different. As a client/server mod developer, I want my users to be disconnected on version mismatch with the clearest possible error message

[mental ingot]

No it is actually like a big special packet

Which renders like a whole table

And has the ability to show links to issue pages etc

[fair sandal]

But mods that don't do networked stuff should probably not be included

[steady sonnet]

why does this read like a formal user story

[fair sandal]

Lol that's an accident:P

[fair sandal]

Once you have an updated protocol we can send it to modmuss for validation, there's probably been enough discussion on the latest proposal

I don't know what @opal ferry would think of generic packet splitting in general. It would solve a bunch of problems quite nicely (recipe sync, simpler regsync), and I don't really see the drawbacks (especially if it's standardized and the other server platforms recognize it)

[mental ingot]

It is also very easy to do

Even with mixins

You just need to target the pipeline construction callback

And add a single "beforelast" netty component

And you are good to go

[opal ferry]

Under no circumstances should we be touching any minecraft: packets. We can possibly solve the registry sync issue by sending another packet instead using our own format. Reg sync isn't complex at all. Reg sync and configs should be loader specific as well imo, I don't imagine many people will run content modded servers across two loaders.

[mental ingot]

But the "minecraft" packet are the problem. The recipe sync packet etc can really easily escape the limits

Especailly on large packs

Were there are now due to mojangs splitting of the colorization recipes a massive crapton of recipes that need to be send

[opal ferry]

So then the mod loader should have its own recipe packet that can be split

[fair sandal]

We'd only touch the minecraft packets if they would exceed the max size that a client can handle

[opal ferry]

Something has gone wrong if you start randomly splitting these packets

[mental ingot]

But why?
Why make this so hard, and complicated

[fair sandal]

So the client would be rejecting the packet by default

[opal ferry]

Changing anything in the Minecraft namespace is a massive no-no. We agree not to change the register packets but now we are happy to change these. There are (external) tools that inspect these packets. Packet splitting should be opt in, for most packets something has gone terribly wrong if it's massive

I'll be around more tomorrow, it's hard to type on mobile

Our networking api isn't in a position to split vanilla packets, especially in 1.20.5

[fair sandal]

I wonder... is there a fundamental difference between splitting the packet and just increasing the limit on the client side?

[opal ferry]

A "c:recipes" or whatever packet might a better solution

[fair sandal]

That is not observable by external tools either

[mental ingot]

It is not the limit

The recipe packet can be 8MB

Because it is a full packet

Not a payload

Remember we are talking packets here not payloads

[fair sandal]

Couldn't we lift the 8 MB limit on the client hmmm?

[mental ingot]

No that would break the vanilla client

And is an actuall full hardstop limit on the protocol

It is encoded in the max sizes of the buffers etc

[opal ferry]

At least the external tool wouldn't try to decode it

[fair sandal]

Ah cause packets over the limit disconnect whereas unknown split packets just get ignored?

[mental ingot]

Correct

But it is even easier

We know if the remote endpoint supports splitting or not

If it does not

Then we simply don't inject the pipeline step into the netty pipeline

And it will run into the normal error message that mojang shows: Packet X was too big. It was X but only Y is allowed

The problem is that X Y and Z are numbers

Nobody knows what this is

Or what causes it on a first glance

It is a very poor user experience

And yes modmuss I agree that normally touching minecraft shit is a big no no

But here I make an exception in that ideal

[opal ferry]

It's not something we can easily do anyway. We don't touch anything to do with the vanilla packets. Only the custom payload. To be fair I don't think my opinion on this is going to change.

I didn't realise the recipe issue is as big as it sounds here, that can be solved without changing the vanilla packet format.

[mental ingot]

We are not touching the format

We are simply adding a netty pipeline step

That intercepts the payload

Before it is encoded

To check for its size

If it is too big

We simply chop it up

And send it as a payload instead of a packet

On the receiving end we listen for those payloads, glue them together and then hand them to mojangs own pipeline, without it being any of the wiser

Because this is all TCP, the overhead is roughly 2 bytes per chopped section

[obsidian nova]

I don't like that though

[mental ingot]

It is minimal

[opal ferry]

This shouldn't happen magically for all packets.

[obsidian nova]

Because it changes the protocol

Yeah, I agree with modmuss

[mental ingot]

Not really, the serer and client never see the difference

[obsidian nova]

You cannot just think about the base game

There are things like proxies

[mental ingot]

That all still works

[fervent token]

Automatic splitting is useful. At the very least if it changes to not being automatic there should be a way for modded payloads to opt in

[mental ingot]

Because we send payloads

[obsidian nova]

They need to be able to see critical game packets

And not have them just not exist because they happen to be a custom payload

[mental ingot]

Only if they get too big, which is when they are not able to see them anyway, because the server never sends them out

So it is not a loss for proxies

Hell it is a gain if this becomes part of the common protocol

Because they then know

That something is in there

That they need to inspect

And because the split packet has a header

Of 1 byte and a var int

Where the var int is the vanilla packet id

They can trivially inspect if the slice is of interest to them

And process it accordingly

If the packet does not exceed the 8MB

Then it is never chopped or wrapped

And the normal vanilla packet is send

Without interference

And they get to see it

Like normal

Even better

Say they are interested in the worst offender here: The recipe packet

If it gets split, they can easily adapt it

decode the slices themselves

And alter the contents

Maybe now it is small enough

[hybrid prism]

What other vanilla packets are able to exceed 8MB in such circumstances?

[mental ingot]

And then they can send the normal packet back out after inspection

[mental ingot]

It is recipes, and tags mostly

Let me check what we had in the past

Recipes, Commands, Tags and Attributes

Are the ones that were the most problematic

[steady sonnet]

iirc datapack registries?

[mental ingot]

Oh and advancements

[opal ferry]

In 99% of cases if the packet is too big you are doing something wrong, splitting them all and silently sending loads of data is a recipe for wasted bandwidth. Instead disconnecting will force the mod dev to fix the issue, if they then find they need to split the packet as there is no way they can send less data then that option should be available for them. You were also very worried about security, I bet a lot of things will fall over if you start sending gigabytes of data to to the other side.

[mental ingot]

It is not 99%

It is simply that on larger modpacks this was an issue

Modpacks with like 20 mods don't care for this limit

But modpacks with 200

[steady sonnet]

to be fair part of this is the recipe network encoding being horrendously inefficient

[mental ingot]

They do

[obsidian nova]

I'm curious what @tawdry heart's (the VP+VL dev) thoughts are on the packet splitting

[opal ferry]

I was thinking about modded packets, and the 99% of vanilla packets. The recipe and advancement packets fall into that 1% and can be handled by the mod loader.

What's stopping the client from sending a 10GB packet, and causing the server to OOM when trying to decode it?

[mental ingot]

Nothing

It is a valid point

I am fine with making it optional

[tawdry heart]

I think this is a pretty good argument against automatic splitting

There shouldn't be many usecases where that much data has to be sent (Imagine multiple mods doing that. The network load would be really high for slow / unstable connections)

[mental ingot]

Welcome to modding. You seem to be new here. People do stupid shit.

Have you seen minecolonies and what it transfers.

Granted, with the recent increase from 1MB to 8MB it is even less of a case

but I would at least argue that this should be part of the common protocol, al be it optional

So that proxies can rely on what is in that channel

[tawdry heart]

I know that people do stupid shit, but that shouldn't be supported by FAPI. Modders should instead work on fixing it

[mental ingot]

And how it is encoded

[mental ingot]

Welcome to forge, there are good reasons for sending larger data. A lot of mods do this. So forge supports it

And we want to support it in such a way that proxies and fabric knows what the fuck is going on when it gets such a payload

[steady sonnet]

I don't think RK is saying splitting shouldn't be an available option

[mental ingot]

You either support it or you don't...... There is no real middle way

Either you listen to the c:split_packet channel with the required encoding of that packet

Or you don't

I don't really think there is another option

[steady sonnet]

You can listen to the channel without autosplitting every packet type, no?

[mental ingot]

Oh no sure

I am not saying that fabric needs to send the packets split

Maybe I should have made that clear

[tawdry heart]

If large packets are required there isn't much to avoid splitting

[hybrid prism]

Some config or other setting that server owners could enable to allow/deny splitting might be interesting if possible

[tawdry heart]

Good solution might be to make a wrapper protocol for split packets ontop of custom payloads

[mental ingot]

That is what we are discussing

It is literally what forge does

[fervent token]

Can we get a warning level log printed when splitting occurs including packet type and payload name?

[mental ingot]

Sure

[mental ingot]

https://github.com/neoforged/NeoForge/blob/1.20.x/src/main/java/net/neoforged/neoforge/network/filters/GenericPacketSplitter.java

It is a netty pipeline component

That takes in the packet

Checks its binary size

And if need be

Wraps it in a set of payloads

[fervent token]

Then there will still be some chance modders can fix their packets if relevant without impacting the users super negatively which imo is the ideal solution

[mental ingot]

I am also fine with making c:split_packet optional

I mean in reality I only want it in the spec

So that proxies know what this thing is

And what it does

But that means we kind of need some common ground

[velvet whale]

yeah that's a huge issue since it's the same packet for all regs lol

[mental ingot]

Ahhh right

They are all in one

Completely forgot about that shit

[red aspen]

I sync a lot of data from datapacks to the client. I used to use the splitter, but now I just send packets for each object the user configures.

[pliant stream]

ok but like 'item related packet containing a shulker box full of massive nbt items' also isn't any one mod's fault

and not anything any one mod can do to solve either

like certainly we can encourage mods like iron shulker boxes that add shulker boxes that have 108 slots instead of 27 and therefore make it somewhat more likely to try to take steps to prevent it from happening, but to some extent whether the limit will be reached depends on user behavior

[valid idol]

Something I haven't seen mentioned (probably because it's more API than protocol) is fallback registration as an ad-hoc/dinnerbone listening channel if the other side doesn't support the common protocol

For the mod version thing, I think for the most part versioning through channels is good enough
A separate mod could be used for modpacks / etc to ensure the client has the expected setup while negotiating

[fair sandal]

Note: fabric didn't actually implement the correct protocol by mistake

https://github.com/FabricMC/fabric/pull/3544

[mental ingot]

Oef

That is a new one to me

[buoyant pagoda]

[mental ingot]

But yeah kind of case and point

[opal ferry]

Thats why it would have been nice to have something else to test it against 😄

[fair sandal]

Yes 😉

[mental ingot]

We will figure this out 😛

I am updating the hackmd document again

It took me a good second to understand what the registry packet in fabric looked like

[mental ingot]

Here we go: Updated the protocol: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view

There are now c, o and neoforge payloads

C is the required part of the protocol

O is the optional part of the protocol

NeoForge are neoforge specific payloads

And are basically there as an example

[fair sandal]

wait you used the o namespace? 😛

[mental ingot]

Yes

I was trying to figure out how at a glance somebody could see that an optional part of the protocol is in use

I figure the same reasoning for "c" could be used for this case and I created the "o" namespace

[buoyant pagoda]

what reasoning

[indigo hazel]

Mainly that it's an invalid mod ID, so there's no chance of a collision with mods

[mental ingot]

Exactly

[fair sandal]

mod_list_server is quite problematic I would say

would fabric support this with no versioning nor flow, or would it just not support it at all?

[mental ingot]

?

[mental ingot]

What are you referencing?

The negotiation packets are version less

But they have a flow direction

Hence them being different payload ids

[buoyant pagoda]

i would think those first two payloads, c:supported_channels_negotiate and c:supported_channels_suggested, can be consolidated into one payload

[valid idol]

are the arrows in https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view#Definition going the wrong way? like is it meant to flow down?

[mental ingot]

No different direction!

[valid idol]

server: what protocols you have
client: ok i have x,y,z
server: ok, use z

[mental ingot]

Oh I see

Yes it is the wrong way around

Hold on

No actually

It is the right way around

[buoyant pagoda]

checks hackmd
ah, I see the protocol was fleshed out more than the last time I looked at it

I was basing off my memory of an earlier version that didn't specify the packet data

[valid idol]

i dont think the client responds with that right?
that top one is the first server inited one, and should flow into others i thought

[buoyant pagoda]

btw, I would highly recommend the packets be specified down to the byte format, just to remove any doubt on how they are formatted

[mental ingot]

They are?

In the sense that it uses Objects and contents that can be passed to the FBB

[buoyant pagoda]

hmmmm

[mental ingot]

And as such the byteorder is specified fully

[valid idol]

PacketFlow i havent seen the specification for

[mental ingot]

It is an Enum

But you are right

Hold on

[valid idol]

is it also worth adding a "NONE" packet flow, so you can have the versioned negotiation with a guarantee of no packets?
like for dummy versions of modpacks / other versions

[mental ingot]

That is BIRDIRECTIONAL

But yeah NONE is usefull for enforcing versioning

[buoyant pagoda]

going to look at the protocol a bit more in depth later or tomorrow

[valid idol]

also i think there should be something stopping you (defining it as a protocol violation maybe?) preventing you from having the same <id,version> pair multiple times in the ChannelSpecification list, since that is what is used to select the supported channel

[mental ingot]

Agreed

[buoyant pagoda]

to be clear, a "channel" in this proposed protocol is equivalent to a payload carried by the *boundCustomPayloadPacket?

[mental ingot]

Correct

[buoyant pagoda]

https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view#Concept (the definition of the concept of "channel") should notate each part of that Channel diagram as to what they mean

also, I don't think the connect and disconnect methods belong on that diagram -- that's moreso an impl. detail

[buoyant pagoda]

for example:

• what is an "ad-hoc" channel?
• what does it mean for a channel to be "required"?
• what does it mean when a channel is "connected"?
• how does "connected" relate to an "ad-hoc" channel?

other questions at the moment:

• do we want to define our own "channel" term? why not use the terminology of "payload" (as part of the *boundCustomPayloadPacket)?
• how are these custom "channels" (I'll stop using quotes here) represented in minecraft:register and minecraft:unregister?
  • how are these protocol-defined payloads (c:protocol_version_negotiate) represented in minecraft:register?
• what happens when the other side does not support the protocol? such as:
  • does not understand it at all (all protocol-defined payloads are not understood)
  • cannot negotiate a protocol version (no common protocol version)
• what protocol version is this proposal starting at? (name suggests 2, but it's not clear which)
  • what/who will define future versions of this protocol?
• why are the registry sync payloads under c when it's noted they are optional? accrd. to the above discussions, wouldn't they be o?
• are the registry sync and mod list sync part of the base protocol?
  • can they be 'split off' from the base protocol, and negotiated dynamically using the base protocol (through channel negotiation)

i shall leave these questions to stew in your minds as I try to un-nerd snipe myself

is this what @fervent token feels like when she goes about poking holes in maty's PRs /jk

[valid idol]

in https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view#Flow
what is actually sent in the minecraft register for supporting Legacy Systems?
i'm assuming it cant include all the channels that you want to use, as something might get the wrong idea from that.

this is what i came up with, thinking about the fallbacks:

c:net server, dinnerbone client

s2c minecraft:register C:*, a_mod_not_supporting:cnet
s2c ping
c2s minecraft:register some_other:mod (doesn't include c:* as it doesn't support it)
c2s pong

(server sees client doesn't support cNet)
s2c minecraft:register any_listening:channels that_can:fallback (they aren't send in initial register, as they are negotiated using new protocol)

other way round works mostly the same, with 1 difference:
since the client knows earlier that the server doesn't support c:net (since its not in initial register)
it can skip the stripped register where it doesn't include most channels

if they both do support c:net, then once negotiation is complete, but before they start listening, a new minecraft:register should be sent
containing all the channels they have been agreed upon
while its not required for ever end (the protocol already knows what was negotiated), its useful for any proxies that might try to enforce packets passing through based on whats registered

[errant summit]

what I discussed with orion (and he probably forgot) is that in the legacy packets we should send all payload ids we know, ignoring optionality and version including all c: (and as it seems now o: too) packets

[valid idol]

right ok.
that seems like it could be an issue for a system that otherwise supports c:net, but has a most listening onChannelRegister for their id to immediatly start their own packets / negotiation (before the channel setup is finished)

also, that only handles config phase packets right? onces its in play phase, there are a difference set of channels

[errant summit]

right @mental ingot we forgot the legacy stuff at the start of the play phase

[mental ingot]

Not needed

The mc: protocol is statefull

Once send the channel remains active on the connection

Regardless of phase switch

Or at least it is undefined

And open to interpretation

[errant summit]

ok then it's fine

[mental ingot]

I have not found an implementation that stores this information on the listener, all do it on the Connection object

Which persists even between phases

[valid idol]

so why have 2 seperate channel specifications for config and play?

[buoyant pagoda]

yea, it is undefined as to what the minecraft:register (and companion) payloads mean after the configuration phase was introduced, since we don't have the Word of Dinnerbone to set us all straight

but as Orion notes, it's practical to assume that channels/payloads registered through that persist through the phrases

[mental ingot]

Mojang does. Dinnerbones protocol does not

@errant summit Should we send the register stuff again, at the end of the config phase

Now with the play channels?

[errant summit]

as an actual config task yea

that means we don't need to send 2 arrays of specs in the channel negotiation

or wait you mean the legacy stuff... hmm

[fair sandal]

I have a lot of questions on the design still 😛

this is hell

we need sub-threads or something

[valid idol]

already 7417 msgs long too

[buoyant pagoda]

who wants to startup a NF forums /jk

GH Discussions could be an acceptable substitute

[fair sandal]

well on a PR you can open many threads simultaneously

[buoyant pagoda]

either under NF, or whoever is the answer to "what/who will define future versions of this protocol?"

[fair sandal]

does hackmd support that

I can add comments

[buoyant pagoda]

actually, I believe HackMD does have comment support

lemme see

https://hackmd.io/c/tutorials/%2Fs%2Fhow-to-use-comments

should we keep it at signed-in users, or allow everyone (even those without an account)?

[errant summit]

I think there should be a github org where all loaders (mod or plugin) have representatives and a repository in that with all common definitions documented

[errant summit]

signed in

[buoyant pagoda]

hmmmm, this idea has been suggested before I know

[valid idol]

(was for tags before i think)

[buoyant pagoda]

@fair sandal I think poked at reserving a GH org for that idea

[buoyant pagoda]

ah yes, t'was for #1134480199937957969

@polar tapir's work

[obsidian nova]

Or move this to a real channel :P

[buoyant pagoda]

(also, it took me 5 solid seconds to remember your name, Tele, and I have no idea why)

[errant summit]

can you set the document to members only write

[fair sandal]

I'm just leaving comments on the hackmd right now

I set write to neoforged org

[buoyant pagoda]

just to poke back at that common org idea:
we now have two candidates for things to be put under that common org: the common tags convention, and this common networking protocol

[fair sandal]

I already claimed the CommonMC org

[polar tapir]

I'm keeping my PRs up until a common org idea actually exists, pans out, and is utilized by both loaders

[buoyant pagoda]

yeah, that's understandable

imma poke some of the team internally on this, perhaps we can start a round of feeling out to potential stakeholders on this

[errant summit]

can you invite orion and modmuss as admins

[fair sandal]

I'll do it once there is something there 😛

[errant summit]

then create a "Specifications" repo in the org

[buoyant pagoda]

now now, don't be too hasty

[errant summit]

@fair sandal @valid idol I have replied to some of your comments

[mental ingot]

Okey back from walking the doc

Addressing comments and change requests as fast as I can

[buoyant pagoda]

walking the doc, you say?

now, I can choose to interpret this as a typo
or either in terms of 'documentation', or 'doctor'

i think I shall choose the latter two, because

[iron ivy]

probably dog

[errant summit]

he clearly walked the document

[red aspen]

gotta give it some fresh air every so often to clear out the outdated verbiage

[valid idol]

@errant summit

the protocol version does not define the content of the payloads but the mechanisms and sending order of stuff
does that mean if a new field was added to the channel specification object, the version would not change? even though it would be incompatable? or do you mean the content of registered channels, like my_mod:custom

i've realised what Technic4n previously said about versions being separate to channels can make more sense, esp if each channel is 1 packet so you need an overall protocol version:
you dont want a server that selects the highest version of each channel individually, because if a packet is removed, and backcompat is availible, then:

say:
legacy protocol
modern protocol, only on new servers

if on a new server, it should only select the modern one, and completely disable the legacy protocol, but since they use different channels (say because backcompat / moving to a shared format between multiple mods / smth), and they are versioned independently, they cant be linked in that way

i guess there is 1 way of fixed it, which is if you support the modern protocol, you register a new version of the legacy channels too
which noops / packetflow.NONE, to remove the channel from use.

[errant summit]

i mean it is the version for the protocol (the order stuff is sent in and the mechanisms) and the negotiation payloads (as they can't negotiate themselves)

all other c: payloads are treated the same as mod added ones

[valid idol]

yeah i agree

[obsidian nova]

So uh

Tags are synced over config now

[velvet whale]

so.. how's it going

[dim wagon]

not bad how about you

[errant summit]

We got a lot of feedback and I'd say it is time for another round of applying changes and double checking our own work (and answering more questions)

[dim wagon]

And waiting for 1.20.5 for stream codecs?

[errant summit]

we did not decide on that

[mental ingot]

I have time indeed later today

[errant summit]

👍 ping me when you know when

[velvet whale]

if we wait until 1.20.5 then we should hotfix the mc:reg stuff in 1.20.4 and release a stable version

[fair sandal]

I think we should fix everything now in 1.20.4

Seems easily implementable once the spec is agreed upon

[mental ingot]

Yes

The actual implementation is really easy

[fervent token]

how breaking is it for modder facing impl?

[steady sonnet]

not breaking aside from requested renames

last time I asked

[mental ingot]

Not at all

Unless we accept some of the requested networking api changes, like the isConnected api

[fervent token]

by that do you just mean rename or is there functionality changes that have been requested

[mental ingot]

The functionality changes are optional

The name change is requested

@errant summit I have one more work meeting

And then we can get started

[mental ingot]

And I am on hold for a while at work

Emergency meeting

Great

[mental ingot]

Okey, updated the spec again: https://hackmd.io/SYCaVQyMQZaaBAp6oGqfSg?view

The spec contains flows now for legacy mode, as well as vanilla mode

It also contains extra checks for compatibility with the protocol

@errant summit and @fair sandal Can you guys read through this again, and see if I made any correctness mistakes. I understand that you guys have some objections to the functionality of the protocol in some areas, and I made all of those optional now. So I am now looking for feedback and ideas, with respects to whether I missed something on a technical level.

[errant summit]

looks good, however I don't really like the move of optional parts to another namespace, if it is optional or not it is still part of the common spec and thus should be in the c namespace

also I think a brand exchange should also be added (so the each side knows what the other is running), so a simple 2 strings one for brand (neoforge/fabric/paper) and one for the version

[mental ingot]

That already exists

Vanilla sends the brand payload

Which at least forge patches

[iron ivy]

Does vanilla send it prior to modded negotiation?

[errant summit]

yes at the end of the login phase

[iron ivy]

then thats fine, duping into the spec is not required

[errant summit]

well if other loaders don't do it it is useless, it also only includes a single string (neoforge for us and vanilla for vanilla mc)

[fair sandal]

regsync is not going to work as a common spec without packet splitting as done by fabric

[iron ivy]

Question, why do we need to do packet splitting? Why don't we just remove the size limitation?

[fair sandal]

vanilla clients, notably

[iron ivy]

is there a limit on the decoder side?

i thought the limit was only emposed by the frame header appender

[fair sandal]

if a vanilla client receives a split packet it gets ignored

[iron ivy]

thats not what im saying

[fair sandal]

if a vanilla client receives a packet outside the limit it disconnects

yes the limit is on the receiving end

[iron ivy]

I see, they empose 21 bit length

carry on

[indigo hazel]

The 21 bit limit is the compressed limit, the 8 MB uncompressed limit is also checked IIRC

[errant summit]

the size limit is checked for custom payloads specifically in readUnknownPayload in the *CustomPayloadPacket class and errors if it is bigger than 2^15 - 1 bytes for serverbound and 2^20 bytes for clientbound

[mental ingot]

That is only if the payload is unknown

The protocol clearly states that all channels registered and their payloads inside of them should be treated as known

And as such do not fall under that limitation

[errant summit]

which it is if that side doesnot support the unsplit payload

[mental ingot]

That is not defined

[errant summit]

if undefined assume vanilla

[mental ingot]

The payload is never send then

So that codepath is also never triggered

[errant summit]

which breaks reg sync

[errant summit]

orion we are referencing this ^

[mental ingot]

Reg sync with a vanilla counterpart is broken anyway!

Reg sync works fine, the limit is 8mb for any connection that supports the protocol.

If you have a single registry where that packet takes up more then 8mb then modmuss argument stand: either add a mod that implements the splitting, or shrink the reg.

[errant summit]

how many blockstates would you need to reach that?

[mental ingot]

A large amount

Remember this is a compressed protocol

We only send blockstates if they are not trivial

And we shrink and compress them as far as we can

There is simply a functional limit to what we can achieve

[errant summit]

lets say a blockstate takes 8 byte that means we can have 1 million blockstates before we have a problem

[mental ingot]

It is simply a functional limit of the agreement

Fabric does not want the common level splitter

And we do not want the packer specific one

So we agreed to make it optional

Simple as that

[iron ivy]

It seems reasonable that splitting is optional. If Fabrics solution is to have it implemented by a mod, so be it. The protocol just needs to clearly define the split spec, and have proper disconnect messages defined for packet being to thicc

[mental ingot]

That is the idea covers

We define how we split

And fabric can choose or not to do it

[iron ivy]

I don't see why there is such a big song and dance around this

[errant summit]

ok then I think we can create the repo on the common org and put the document there via PR for final comments

[mental ingot]

The realistic point is: There are more then a handfull of packets where this is important

Whether they are in the protocol or not, does not matter

And NeoForge decided to once and for all take care of it in the situations where we could

Fabric is more conservative here

And only patches the locations where it specifically knows there are issues

Both are fine

[fair sandal]

my only problem is that we are trying to define a "common" protocol without any agreement from the other parties

[mental ingot]

We are making a proposal

I am currently looking for a prelim signof from our side

If we are okey with this

I would like to send it to modmuss and others

[fair sandal]

one thing I dislike but it's not really related to the protocol

is how sequential the config tasks are

[mental ingot]

I think mojang just did not want to do a toposort

So they whent linear instead

[fair sandal]

yeah but with 10s of mods registering config tasks the amount of round trips might get ridiculous, especially with the ack packets

[mental ingot]

I understand that

But that is what we have right now

And if it really becomes an issue

We could potentially create like an async approach somehow

In the future

It is not really related to the protocol though as you said

[fair sandal]

yeah

[mental ingot]

Use some of the CF Toposort dependency shit I am making for FML

And we would basically be able to run this in parallel

[errant summit]

ok the contacts for the other loaders are:

• fabric: modmuss and player
• paper: riley

[mental ingot]

I think if we are in agreement that it is okey as an alpha version of the protocol

Then we can run this through #wisdoms-private

[errant summit]

share link for the doc: https://hackmd.io/@neoforged/rkQ8WCW_T

[mental ingot]

Done RFC is out

[fair sandal]

looks fine

[obsidian nova]

Well iirc I've seen ~200k being hit (https://modrinth.com/mod/consistencyplus)

[errant summit]

with a single mod 👀

[fair sandal]

kind of a dumb mod imo

[obsidian nova]

C+ basically stress tests game limits lol. It adds thousands of blocks

[fair sandal]

just use framedblocks

[obsidian nova]

This predates that

[indigo hazel]

Nope, it doesn't

[obsidian nova]

According to CurseForge, it does

By like a year

But also this has more features

It's not just shapes

[indigo hazel]

Merely by one month 😄
And I have a 1.15 version

[uncut eagle]

I assume these are public where are they posted, or are they internal for the team only?

[fair sandal]

this is a bit over the top

we can just send c:protocol_version_negociate, minecraft:register, Ping in that order

depending on what we receive before Pong we know if the client supports 1) this common protocol (if it sent c:protocol_version_suggested), 2) only Dinnerbone (if it sent minecraft:register only), 3) vanilla (if it only replied with Pong)

[mental ingot]

It might be possible. But this is a more stable, and easier to implement scenario

But if need be we can make that work

[fair sandal]

it's literally the reason why minecraft:register is sent before Ping, so it's a trivial extension of that 😉

[mental ingot]

Hmm okey

Lets just see what others say first

And then we can iterate over this okey?

[fair sandal]

that's why I'm leaving the feedback here

[mental ingot]

Could you do it in the document

This gets lost

[fair sandal]

I added a note in the doc

[errant summit]

the discussion is in an internal channel but the doc is still the same: https://hackmd.io/@neoforged/rkQ8WCW_T

[obsidian nova]

(@uncut eagle)

[uncut eagle]

Ah okay, I was looking around on the neoforge repo for a pull request or something to follow along the progress. I'll keep an eye on this channel then thanks :)

[obsidian nova]

This should probably be pinned

[indigo hazel]

It is pinned, just under a slightly different URL

[obsidian nova]

Why is the URL different?

[indigo hazel]

Ask HackMD why there are two ways to link a page ¯_(ツ)_/¯

[errant summit]

one is the read-only and one is the editor in view mode

[obsidian nova]

They're both in the view mode for me

[fair sandal]

That's because only the neoforged team has write access

[mental ingot]

Note to self: need to deal with this: https://discordapp.com/channels/313125603924639766/1154167065519861831/1201607124711649290

[granite forge]

I'm gonna ask here, cuz i feel it would get lost in modder-support, is there any possibility to fix following:
you send message from server to client in PlayerLoggedInEvent.PlayerLoggedInEvent in singleplayer
you receive the message on client before client set Minecraft.player
so this is null https://github.com/neoforged/NeoForge/blob/77c8899d73751044cc19e4fe7cc521d50634f5b9/src/main/java/net/neoforged/neoforge/network/registration/NetworkRegistry.java#L418
but according to docs it should not be https://github.com/neoforged/NeoForge/blob/77c8899d73751044cc19e4fe7cc521d50634f5b9/src/main/java/net/neoforged/neoforge/network/handling/PlayPayloadContext.java#L23
i have not tried but i assume same scenario may happen during logout event
edit: guess delayed sending is the way, still it's weird if you don't get the player in play phase

[mild spruce]

That is a new issue with 1.20.4 i noticed it also, I think both forge and neoforge have that issue. Fabric does not.

[dim wagon]

it's been a thing for.. forever in forge

long before neoforge

think there's something similar with another event that you expect the player to be available in but it's null still... can't remember which though

[mild spruce]

I never had an issue with it until 1.20.4.
It was easy for me to work around it without delaying the packet. But, I don't need to have the player fully ready when that event fires.

[spark saffron]

We would have to move the login event firing to after login is completed and the client has been notified

[mental ingot]

Yes the simple answer is: do not use player logged in for networking

[mental ingot]

The problem with this event is that it is fired during initialization

So the player is simply not completely setup yet and as such the client does not have it yet

Now with respect to alternatives

There are two options:
If you do not need the player, use a configuration task to send for example custom data pack components

If you need the player to filter stuff, then you could try the data pack sync event

[dim wagon]

so basically, use hacky-workaround?

maybe a non-hacky workaround is a better idea

obviously not related to this specific thread

but I wholly disagree with the notion that the solution to syncing some data on player login is to 'use the data pack sync event'

unless we wanna rename that event to something more fitting

[delicate dawn]

maybe a ClientEnterPlayPhase event or such?

something specifically made for sending those packets

[dim wagon]

the thing is

PlayerLoggedInEvent sounds like it should be used for that

it should be named like playernegotiationevent or PlayerAboutToLoginButHasntYetSoDontSyncStuffHereEvent

[delicate dawn]

well there's a configuration phase now, between login and play

so maybe we need to make the distinction more obvious

I haven't looked at when the event fires in relation to the network state, nor in relation to the game state

so maybe that I suggested makes no sense

[mental ingot]

None of this works

Because the player initialization is a spread out thing

We fire different events for different reasons at different times

[mental ingot]

No both locations mentioned are specifically designed for what I said they are

There might be an argument for the player initialization completed event

[delicate dawn]

but I see three stages that the modder may care about:

1. the connection is being established, is there any reason it should be rejected?
2. the configuration is ongoing, is there anything additional to send?
3. the player is now fully ingame and initial state sync can be transferred

I assume this use case being discussed is 3.?

[mental ingot]

Yeah well except that 3 is wrong

Your initial state sync should happen during config phase

Unless you explicitly need the player entity itself

3 nearly never exists.

[delicate dawn]

does mc now send bulk chunk packets in configuration?

[mental ingot]

No, but they get triggered differently

If you have information related to chunks send it with the chunk

Don't abuse the login for that

[delicate dawn]

was thinking of like, SavedData state that is world-level but separate from chunks

[mental ingot]

Use the join world events for that

[delicate dawn]

I believe that's what I already do, just wanted to clarify the "game stage"

so in summary,
for stage 2 data we have either configuration tasks or datapack sync event,
and closest we have for stage 3 is join level event (which will fire more than once during a connection)
what is the login event good for, then? what use case do we have for it if modders can't rely on things being actually ready there?

[mental ingot]

It is for what it says on the can: to get notified of a login

It indicates the earliest possible time the player entity exists and could be used to verify if he or she should be allowed to join

[iron ivy]

I think the problem here is historically that's not what it's been, its been 'Player entered play phase and is in world'

Perhaps we need to change the name of the event

[spark saffron]

The DPSync event is appropriate for datasync

the login event allows for cancelling the login iirc so it can't be moved

[errant summit]

we should arguably make the DPSync event a config phase itself

[mental ingot]

We can't

For DPSync the player entity is most of the time needed

See the recipe filtering that mojang does

Hence it being in the place where it is

[errant summit]

hmmm

[dim wagon]

I hate when this crap gets said

the whole reason this conversation is happening is because it's obviously not distict/clear enough

[mental ingot]

Yet it is the truth

[dim wagon]

you can't argue that it's clear enough

cus we wouldn't even be here talking about it if it was

to ignore the truth of reality is just ignorance at best

[mental ingot]

It might not be clear

But they are designed with their name in mind

Not with a hacky idea that you should do other things in them

[dim wagon]

they can be designed for a steak dinner for all it matters

the point is, it's not clear

nor is it intuitive

[mental ingot]

What is not clear about:

PlayerLoggedInEvent

[dim wagon]

the player is logged in

therefore they exist

oops, apparently not

[mental ingot]

In what way does it imply: Hey I can send shit over the network?

[dim wagon]

in what way does it not

[mental ingot]

No.......

[mental ingot]

As you well know, logging in and playing the game are two different things

[dim wagon]

oh come off it

that's the first thing basically anyone will think

it's depressing you even have to be convinced of that

[mental ingot]

No it is the first thing you think

Do not equate yourself with the rest of the world

[dim wagon]

uh dude

I didn't start this conversation

[whole hare]

can yall not call it "PlayerLoggingInEvent" instead of "PlayerLoggedInEvent". cause they both mean different things

[dim wagon]

in case you forgot

other people also have the issue

not sure if you're aware

[mental ingot]

No cause it is technically fired after the login completes, aka right after the entity is instantiated, but not in the world yet

[iron ivy]

Orion, it depends what context your looking from. From a network POV, logged in != in world. From a general sense of logging in, i.e message in chat, means in world

We should change the name to Connected/Disconnected

[mental ingot]

Yeah I completly understand that

But we all talking about networking here

The question is: Why can I not send a payload to the client in that event and have the entity there as well

Well the answer is because the player is not in world

[dim wagon]

yes, so it's misleadingly named

[mental ingot]

And as such does not have an entity that the client can work with

[iron ivy]

I don't think we are, we understand what the event does now and what the answer is, The argument now is tht the event is missnamed, and the name should be changed

[mental ingot]

The event should be moved yes

[static kelp]

The realest question: How is something like this not explicitely mentioned in event docs, and how can docs be improved to make these kinds of things obvious?

[mental ingot]

It should not have a player at all

And be fired when the actuall login happens in the login sequence

And no replacement should be made

Because there are literally 20 other ways to do what is currently being done to that poor event

[iron ivy]

Imo, it also needs a new name. Connect/Disconnect is probably better

I'm not saying move the event

[mental ingot]

Sadly that does not work either, due to reconfiguration and reloads being shitty here

[iron ivy]

Just rename it and re-clarify its docs

Does it get fired again when reconfig happens?

[mental ingot]

Yes

Because the entity gets reconstructed

[iron ivy]

Then it should be PlayerConfigurationPhaseStartEvent or something

[mental ingot]

When you reconfigure a player

[mental ingot]

Nah the problem with this event is that it is in play phase

It is basically the first moment the server has the entity at all

If at all, it should probably be ServerLoadedPlayerEvent

[iron ivy]

Sure

something

[mental ingot]

To indicate that this is the moment that the server loaded the player from disk

[whole hare]

"Load" can also be confusing no ? as in loaded into the world

[iron ivy]

ServerPlayerDataEvent?

[mental ingot]

No

Load because that is literally what it does

It takes the gameprofile

[velvet whale]

there's another event for that lol

[iron ivy]

jesusfuckingchrist

[mental ingot]

Jesus christ

[iron ivy]

Does it need to exist at all then?

[mental ingot]

Okey so this event is going to get nuked in my hotfix PR

Yes

[iron ivy]

what purpose does it have?

[mental ingot]

It just needs to be in a different place

It needs to be during the login

But without the entity

[velvet whale]

that other event is fired in such a way that you can change the player nbt

[mental ingot]

ServerPlayerLoadCompletedEvent?

Then

Mojang should have just refactored the recipe shit

And moved that also to the config phase

[errant summit]

yea replace the player instance with the GameProfile

[dim wagon]

then it raises the question of - what event should we be using to sync down player data on login? lol

cus I don't even know anymore

I'm talking about stuff like capability data - things that only exist ocne the player exists

[velvet whale]

do you want to sync it on reload too

[dim wagon]

I mean the use case usually tends to be that someone's stored some relevant data about ap layer on the server

[errant summit]

config tasks or if you need the player to be in the world DPSync

[dim wagon]

and want to sync it down on login

like if you have an RPG mod for example and it has skills/levels, you want to sync down the player's levels on login

[velvet whale]

if it's data that can be reloaded and you want to sync it on reload too use the dp sync event. granted I'd always use that event since it's the one that guarantees most stuff

[dim wagon]

I see
Then would it be worthwhile also renaming that to something like PlayerSyncEvent

so it's obvious that it's for.. player syncing

[velvet whale]

that event can fire with a null player

[dim wagon]

ffs

[velvet whale]

in that case it's a /reload command

and you're given the player list

[dim wagon]

right so basically if someone comes into a modding server asking how to send their player down on login, I have to give them some seemingly unrelated event and then give them a conditional on it?

that seems.. problematic for something so common

[velvet whale]

if it's syncing for a specific player then you have a player, otherwise the server executed a reload and you have a player list and you're meant to iterate over that

[dim wagon]

so yeah feels problematic to me

that's not ideal =/

[errant summit]

if you don't need the world context a configuration task should be fine

[velvet whale]

https://github.com/neoforged/NeoForge/blob/77c8899d73751044cc19e4fe7cc521d50634f5b9/src/main/java/net/neoforged/neoforge/common/NeoForgeEventHandler.java#L115 you can do this

[dim wagon]

yeah just not a fan of its 'you have to already know, to know' problem

[mental ingot]

If it is static data related to a specific player:

Configuration task

As long as you do not want to reload it from disk

So something like their skill tree, levels etc

That is done in a config task

[valid idol]

id just do that in play, since it can update during play right?
there is 0 need to do it that early (and would break etc with server switching on a network, that re-entered config mode to reset client state)

[pliant stream]

doesn't join world fire every time the player changes dimensions?

[mental ingot]

It does

[pliant stream]

I thought the problem was finding a time to send a packet that is in play but that isn't "fully loaded in" [i.e. the player can't see the world and move around before your packet arrives]

i.e. along with the initial chunk data

[mental ingot]

That is called configuration phase payloads

The idea that there is no world and in play does not exist

[pliant stream]

does Minecraft.player exist for configuration then?

[mental ingot]

No that is the point

Minecraft sends everything that needs to be there without the world in config

But then also no player entity (you can only access the game profile, so UUID etc are accessible)

[fervent token]

I mean it just requires special casing on the mod devs to know which packets they send from the login event

given if you main thread it instead of network thread the handling you can access Minecraft.getInstance().player yourself instead of using the value from before it got set in context

granted we could also maybe just make the context have it as lazy so that when handling it then can find the value maybe?

[red aspen]

I've had no issue with that event, but i'm just using it to trigger packets to be sent so i guess since i've not needed the in-world player just the reference to send the packet to it's worked out?

[mild spruce]

Are the changes discussed being worked on or waiting for 1.20.5 to update the protocal again?

[errant summit]

the hotfix is intended for .4

the new protocol needs more discussion with other loaders

[mild spruce]

aah okay

thanks

[fair sandal]

I am working on it

[fair sandal]

I have started yet another hackmd document 😂

[drowsy sail]

Sorry to interrupt in here as an outsider, but are there already any concrete plans for NeoForge 1.20.4/1.20.5 to implement sending the client mod list (+ optionally mod version) to the server during or after NeoForge's network negotiation (if that doesn't already happen)? If not, is this something that could be looked into and PR'd by a non-maintainer (like me), or would the folks working on NF's networking protocol (you're doing a great job btw!) like to implement this themselves?

[errant summit]

it was discussed and is a part of our proposal but was met with quite a bit of backlash

[mental ingot]

This was discussed

And a large part of the community rejected this idea

[drowsy sail]

Ah was it? What sort of backlash/arguments against it were presented?

[red aspen]

iirc it mostly revolved around privacy. People didn't like the idea that a server could obtain information about the client that it didn't need to function

the counter point was that servers would use the information to help prevent cheating through client-side mods, but the privacy needs were greater than the anti-cheat.

that's the part i remember (assuming i'm getting that part right)

[drowsy sail]

Ah that would make sense, I guess now servers that are strict about players using cheat mods need to force clients to install a separate mod (which sends client mod information to the server), which is a bit of a bummer

[steady sonnet]

This won't actually solve anything because such a mod could trivially be patched to send a subset of the real list

[uncut eagle]

That is always the argument, but not everyone is a hardcore cheater, some people just don't realise that some of the mods they use are not allowed so the server being able to send them a message "Hey we know a lot of people use minimap mods, but this server disallows them, please disable [mod]!" would be nice

[drowsy sail]

Yeah, but I guess it would at least be better than nothing; I co-admin a small Forge server and we have caught plenty a cheater via the mod list sent to the server by Forge (though we surely also missed some due to mods editing the mod list, as you said)

[polar tapir]

Heavily obfuscated mod that scans mod folder for mods to parse and get their modid and send encrypted packet to server where server then checks the packet to see if all modid are allowed.

[steady sonnet]

sounds like a slightly more advanced version of blockfront lol

[spark saffron]

Iirc the best method of mod checking was the stuid sign gui thing

[errant summit]

yes but there is a mod that is made to prevent exactly that (even referencing actual chats in the description)

[iron ivy]

There will always be workarounds or bugs. Just because TLS may get broken at some point, doesn't mean we shouldn't use it. Same here. Its another layer that helps catch the blatant ones.

[mild spruce]

was this pr https://github.com/neoforged/NeoForge/pull/619 test against Forge?

[velvet whale]

something tells me "no"

[mental ingot]

No

As you can see in the list of tests

[mental ingot]

But it should work

[fair sandal]

Forge should be considered generic modded by our standards if they implement the dinnerbone protocol correctly

[errant summit]

"if"

[fair sandal]

It's the only platform I don't care about, but hey someone should test it

(someone = not us)

[velvet whale]

~~ do you care about quilt~~

[dim wagon]

lol

[mild spruce]

neoforge client, forge server
https://gist.github.com/mysticdrew/03b2674181bf59403d9c1596cbf0be33

[fair sandal]

so the error is on the neoforge client side?

looks like a decoder problem

[mild spruce]

Yes

[fair sandal]

remember that forge prefixes packet with an int discriminator

[mild spruce]

I am testing forge client, neoforge server right now

[mild spruce]

Oh, I thought i disabled my test packets, I missed one.. Oops. connection is a success.

[fervent token]

Just for forge client or also Neo client? This system is unclear what is a success

[mild spruce]

neoforge client and forge server, sorry getting pulled in a million directions tonight between mod stuff, family and work. Ugh

Sending a packet from forge server to neoforge client gets discarded by neoforge, this is the log out put.
[19:40:45] [Render thread/WARN] [minecraft/ClientPacketListener]: Unknown custom packet payload: commonnetworking:example_packet_one
I have confirmed the client does have that packet registered.

Kinda pathetic that you guys are all "we don want to work with forge" and forge is all "we dont want to work with them" fuck the modders, fuck the users.
I'll just hack this shit up on my own instead of dealing with trying to get people to play nice for the community. "Our fragile egos are more important"

[fair sandal]

how salty and misinformed can one be

the real question is whether neoforge detects forge as ConnectionType.VANILLA (bad) or ConnectionType.OTHER (good)

[restive forge]

Harsh but seems accurate

[restive forge]

reading chat here and in forgecord, it is pretty much how I see it too.

[fair sandal]

here

so if forge implements the standard mc:register protocol correctly it should work

[valid idol]

Btw how does detection work in the case of multiple? Like say neoforge client / fabric server, but also direct Minecraft:register channels are used

Either by a server mod directly, or say a proxy like velocity in-between

[buoyant pagoda]

i mean, do you know the history of how NeoForged came to be? and how that colors our relationship with Minecraft Forge?

[mental ingot]

That is not what this is all about. We just burned out on the networking topic. I honestly just forgot.

[dim wagon]

it's like.. the exact opposite lol

the networking thing was already good to go until it was determined compat issues arised and it was reopened for fixes

if we didn't care about compat with other loaders & stuff this thread wouldn't even be open

[mental ingot]

We care

But there is only a limited amount of Energy I can spend on a given topic.

People were complaining about hypixel, bungee cord and fabric

I literally forgot to check forge

That is all that happened

His reporting on not being able to send a payload

Seems to indicate that forge-neoforge connections are either not upgrading their connection type. Or that forge is somehow not sending the reg payload at the right time

[mental ingot]

You are on 20.4 right? Can you get me the exact versions? I am going to test and debug this.

[mild spruce]

First, Sorry for the harsh message yesterday. I was frustrated and I should have stayed off discord for the day. Apologies. It was not my day.
Results.
First Fabric's ClientPlayNetworking.canSend(message.id()) and ServerPlayNetworking.canSend(message.id()) no longer work with Forge and NeoForge - It used to work with Forge

Forge's channel.isRemotePresent(connection) check does not work with Fabric or NeoForge - It used to work with Fabric

With those checks disabled.
Forge Server:

• Fabric Client: Message received on both sides
• NeoForge Client: unvalid packet id error received on client, packet not handled. Nothing received on the server.

NeoForge Server:

• Fabric Client: Message Received on server, nothing on client, no errors or invalid packet id, though I see some invalid neoforge packet Ids (neoforge:register, neoforge:network 2x)
• Forge Client: Message Recieved on both sides

Fabric Server:

• Forge Client: Message received on both sides.
• NeoForge Client: Message Received on both sides.

This is my test code.
https://github.com/mysticdrew/common-networking/tree/Modloader-Networking-Test
Versions used for testing

forge_version=49.0.30
neoforge_version=20.4.162-beta
fabric_version=0.96.3+1.20.4
fabric_loader_version=0.15.7

[fair sandal]

for fabric play packets, you have to use a dynamic listener registration

i.e. registerReceiver instead of registerGlobalReceiver, and you have to send it late enough

in general, play networking will not work immediately

[mild spruce]

My test packets are sent very early.

[fair sandal]

maybe try sending them later

[mild spruce]

I have to put fires out at work now, I will look at it when I get some time.