Networking Protocol

[mental ingot]

More then 1 in 5 modders use eclipse

Or a VSCode derivative

So we need that support

Because it would alienate their ability to make PRs

[buoyant pagoda]

what I was more pointing at was that, even for a nicety like an .editorconfig which is optional and only supplementary, the previous management would veto it altogether, even if a vast swath of contributors would benefit

[mental ingot]

Ah

Yeah

[buoyant pagoda]

@mental ingot screm for not having pulled latest when you began work on the networking refactor
which meant it lacked the commit which enabled branch suffixing
which meant I had a brief moment of panic when I saw #builds and thought it was building and publishing the feature branch

why you gotta do this to me

[mental ingot]

I can restart all over again

All of my patches are broken

And it broke basically all other patch

[buoyant pagoda]

👀

[mental ingot]

I am going to make a new local

Cherry pick all shit

And then push

This is the primary reason why I do shit the way I do

Without PR

Without applying patches

So that i can do it all neatly when I am ready

At the end

And not halfway in the middle

[buoyant pagoda]

@mental ingot how does the networking refactor handle https://github.com/neoforged/NeoForge/issues/92 ?

[mental ingot]

Nope it won't do anything for that

[buoyant pagoda]

hmmm

[mental ingot]

Right now I need to find energy and time to fix my workspace

[indigo hazel]

As I mentioned on the issue, this is not a problem with the old implementation on 1.20.2 and if the new implementation uses a somewhat similar point in the vanilla code to tap off the payloads, then it also won't be an issue going forward with the new implementation

It's specifically an issue in 1.20.1 and earlier because the tap-off happens on the netty thread there. In 1.20.2 this is not the case anymore though

[mental ingot]

The code is currently completely on the netty thread

I think

[buoyant pagoda]

ah

this really begs the question of whether we should open a 1.20.1 branch

[spark saffron]

we should™️

[buoyant pagoda]

dewit

checks if repo rules allow it

it does

[fair sandal]

We should not develop features for 20.1 however

Only critical fixes

[errant summit]

yep because 20.1 is not really a neoforged version (just a frankenstein transitional version)

[spark saffron]

We should treat 20.1 with the standard lts policy we always have

[delicate dawn]

yeh, no breaking changes. at all. additions if people port them.

[marsh loom]

mfw

[iron ivy]

I can delete messages apparently, pranked, sorry for ping

[granite mortar]

all good

i think they're legit, so i'm guessing somebody just stepped on their keyboard (cat moment?)

[prisma garden]

smh spam pinging, ban her

[iron ivy]

her?

[delicate dawn]

[prisma garden]

oh wait

[delicate dawn]

what did I miss?

[prisma garden]

I think I misinterpreted that

my bad

Moderators ping

[iron ivy]

Yes, i pinged, sorry

[pastel pine]

Was it me? I got kicked 😦

[granite mortar]

Yeah

[pastel pine]

My cat definitely stepped on my keyboard

She doesn't understand that she cant 😠

[granite mortar]

I figured as much 🤣

[pastel pine]

I will lock my pc onwards

Sorry if I bothered

[granite mortar]

Nah, you're fine

[gaunt lark]

Curle was trying to get in contact and apologies, she went to timeout and instead banned

[pastel pine]

Ah I see! I wasn't on the server anymore so you couldn't DM me 😆

[errant summit]

what's the status here?

[mental ingot]

Working on restoring my branch

After the fuck up that was the attempt to rebase across the change of the patch format

[fair sandal]

hello there

how are we doing? 🙂

you'll probably need to rewrite each patch manually

(at least that's what I did to update the cap rework)

[mental ingot]

yep

that is what i a am doing

[fair sandal]

nice

https://tenor.com/view/kronk-emperors-new-groove-its-all-coming-together-gif-12813012

[mental ingot]

It is a bit tedious

But it is moving along

[fair sandal]

you know, towards the end of the cap rework I just wanted it done

so I know the feeling

if you need help don't hesitate to ask

[mental ingot]

Now that I am in better health work on porting this has started up again

Should be done by the end of the week

[delicate dawn]

https://tenor.com/view/awesome-yes-yeah-baby-cheer-gif-15859596

[errant summit]

orion if you have your patches ported and/or need a second pair of eyes just ping me

[mental ingot]

I think I got everything updated to 1.20.4 and am now generating patches

Cross your fingers

Pushing updated state

@errant summit and @buoyant pagoda Pushed my work to 1.20.4

Some stuff is still not implemented yet

Like the status packet exchange

But all of the normal networking and configuration phase logic is now

[fair sandal]

cool

[errant summit]

there are some compile errors and you need to run the update licenses task
https://github.com/neoforged/NeoForge/actions/runs/7196077011/job/19600089898?pr=277

[mental ingot]

Yes

schurli see the PR description

This thing is far from done

It is up for people to review the mechanics

Not for people to complain about licenses, formatting, or compileability

[fair sandal]

it's easier to review mechanics if the code compiles (and most importantly, runs)

for licenses and formatting it takes 1 min to run the relevant gradle task ;P

[errant summit]

I mean compiling would be nice at least, you know so we can test / write tests

[mental ingot]

Which is not happening anytime soon

As I said when I opened the draft PR

this is a fucking draft

Don't expect anything more from it

You all got the draft PR under those conditions

It simply is not ready to compile

But you guys wanted to see what I did

So that is what you get

[iron ivy]

Thats fine, we can see the concepts and the direction your going in. Review can happen when your ready, but early feedback on a critial system like this is important. Saves you some time going down holes

[mental ingot]

Exactly

It is there to see the concepts, the direction, and understand my thinking

Not for reviews

[fair sandal]

speaking of which, can you open a new PR?

[errant summit]

I mean I for my part already know the concepts as I was there when we made them

[fair sandal]

since you pushed to a new branch

[iron ivy]

Why does it matter?

[mental ingot]

No it is the same old branch......

[fair sandal]

oh, right

[errant summit]

how did the TC build succeed but the GHA one has compile errors? #builds message

[mental ingot]

No clue

Up next for me is the status packet

Since 99% of all of the logic there are associated with the errors

[fair sandal]

careful that any change in the TC settings will cause a commit to NeoForge 1.20.x

[errant summit]

if it does not build what does it do?

[fair sandal]

setup and clean

[mental ingot]

No.....

[errant summit]

so it just checks the patches

[fair sandal]

apparently

[mental ingot]

NVM

Yes it does

[velvet whale]

build does a publish

[mental ingot]

But I don't mind it there at the moment

[fair sandal]

well we need to disable versioned settings in that case ^^

[mental ingot]

it does

But you can make a commit to that branch

And then have TC load it for that build only

[fair sandal]

it seems to load from 1.20.x only from my experience

[mental ingot]

Most data yeah

There are some exceptions

[errant summit]

Cleanup work to do once it is implemented and works:

• run formater
• remove import changes from patches
• update licenses

[errant summit]

orion just a heads up so we don't work on the same thing,
I nuked PlayMessages and fixed EntityType to use the new payload, and now I'm looking at implementing the TierSortingRegistry sync in the configuration phase

[fair sandal]

you're working on the branch right?

[errant summit]

yes the question is if I should commit directly to it or if I should PR into it

[fair sandal]

@mental ingot ^

[errant summit]

hmm @mental ingot what do you think about a generic configuration task finish packet that just sends the id of the type to the server and finishes the task in the handler (reduces the need to have a packet for each task to finish it)

[mental ingot]

I thought of that, and rejected it

The idea is that Vanilla sends dedicated packets to the server to do that

And that is a much better design

[mental ingot]

Commit directly

[errant summit]

also the TierSortingRegistry has a allowsVanilla in its compat check how would we implement that in the new system

[mental ingot]

Ehm check the vanilla packet splitter

It has logic for that as well

During the config phase you can ask it what kind of connection it is

I think

Hold on

 public static RemoteCompatibility getRemoteCompatibility(Connection manager) {
        return NetworkRegistry.getInstance().isVanillaConnection(manager) ? RemoteCompatibility.ABSENT : RemoteCompatibility.PRESENT;
    }

You can pass the connection you are on to the NetworkRegistry

And it will determine if it is a vanilla connection or not

Question now is

Can you get that efficiently in the configuration phase event

The answer is no

Let me make a patch for you 😄

[mental ingot]

@errant summit I pushed an update now 😄

There is a boolean isVanillaConnection now on the listener in the event

That should allow you to trivially check during the configuration phase setup whether or not you want to add your task

Or if your task needs to run in compat mode

[mental ingot]

@errant summit You around for a call?

[errant summit]

yea

[mental ingot]

Sitting in dev-voice

[mental ingot]

Okey after fighting java generics again

We patched BundlePacket to support common super packets, like custom payload

And we now support bundle packets during configuration tasks

[fair sandal]

cool

is there an risk of blowing the packet size? or a requirement for the packets to arrive at the same time?

[mental ingot]

It is more of a grouping anc conceptual idea

For registry and configs

You want the individual payload packets (aka one for each config/registry) to be sandwiched between a start and end packet

So you get a single bundle with: start -> payload ---- payload -> end

And then the client can neatly answer: yes processed

That is why we put them in a bundle

[fair sandal]

you need the client to reply that the config task is finished?

[mental ingot]

Not a requirement

But it is a nice thing to do

Especially for critical components

Like the configs and the registry

But in essence you can just pepper the client from the server with all packets, and give a damn about it

[fair sandal]

hmmm cause packets are already ordered with TCP so I wonder why we need them to arrive at the same time 🤔

[mental ingot]

Is it a TCP or UDP connection

The default port is pretty high.....

For a TCP

But it should indeed be a TCP

In reality it does not really matter

The problem is Netty

Even in ordered TCP connections

Processing and reading of the packets can still happen in parallel

[fair sandal]

ah yes

you're saying that parsing the config from the bytebuf might require the registry to be already synced?

[mental ingot]

@fair sandal Do you have a minute in voice

I am stuck with the server-status packet

For the server list screen

And it is not looking good for that scenario

[fair sandal]

not possible sorry, I'm in at the "office"

[mental ingot]

Okey

Well then I am going to need to remove the whole "server status" ping login check

[fair sandal]

can't you keep the current hack?

[mental ingot]

Nope

The channel information is not there yet

[fair sandal]

I can have a call tonight

[mental ingot]

It is determined in the config phase

[fair sandal]

we can discuss it at 8 (probably a bit earlier works too) if you have time

[mental ingot]

Yeah

And I just realised that I have to rip the bundle shit from the config phase

Because of the vanilla lcient

It needs a custom packet

and that is a big nono on the vanilla client

[fair sandal]

yeah

well, rip

[mental ingot]

@opal ferry After talking it through internally, we figured out that we can do the "minecraft:register" loop again during status pinging to show the user an indication of compatibility.

Would fabric be also interested in that processes?

We have documented it currently here: https://hackmd.io/U_i1gWO8TT-scIDEuT7JMw?view#Server-status-pinging

Also @fair sandal Your input here would be valueable

[fair sandal]

hmmm if we have that info during the status do we still need it for configuration? or are these two things completely dissociated?

it does raise the question of "status networking" 😛

[mental ingot]

Sadly dissociated

Because the client can be bounced from one server to the next

Where one server supports the channels

And the next does not

[fair sandal]

ah yeah right that was the whole point

[opal ferry]

We dont touch the status packet atm

I dont see why minecraft:register has anything to do with that?

[mental ingot]

The idea is that the player has an indication of if he can connect to the server

Forge right now shows like a green check mark / red cross

[fair sandal]

asking thins because the message list is kinda useless if you don't actually allow sending messages? idk

[mental ingot]

To indicate that the server connection is good / bad

[mental ingot]

As with today, it would be mostly used for the tooltip

To indicate connectability

[errant summit]

or yellowish V for compatible vanilla

[mental ingot]

Right 😄

[opal ferry]

Id have to look at it in detail, but isnt it basically just a json blob in a single server -> client packet?

[mental ingot]

Yes and no, yes in practice that is what we do today, but that has some major drawbacks. For one due to the packet size, we have to do some really ugly encoding hacks to pass the compatibility information along, (vanilla puts the favi icon in it)

The other reason is that we have no real way of knowing if we are talking to a vanilla client / modded server

So the way me and schurli designed it right now

Is to only include a boolean in the status json

That indicates if the server can "speak" modded protocols

And then upgrade the connection

So that a normal negotiation can happen

[opal ferry]

That sounds quite complex tbh, the current single packet with a json is trivial. There are plenty of 3rd party tools/websites that people use to get the details on the server, this is going to be a pain for them. I would expect anything more to happen at the start of configuration.

[fair sandal]

what info are we using this for atm?

[opal ferry]

Some poeple have 10s or even 100s of servers saved, having a complex back and forward chat with the server could end up taking quite a while.

[fair sandal]

in principle it all happens in parallel

[mental ingot]

In practice the delay is minimal, the simple answer is, that without the clients channel information, there is way to know if it is compatible or not

The "minecraft:register"-loop is in the end negotiated on the server

And as such it is pretty difficult to handle this properly on the client side alone

Especially in such a congested packet as the serverstatus

[opal ferry]

Is it really the end of the world if the user tries to connect and then gets a nice error screen? Maybe I dont think the problem is big enough.

[mental ingot]

For me no

I would rip this in a heart beat

Just wanted to preserve functionality

That is all

[buoyant pagoda]

what problem are we trying to solve here, again?

[indigo hazel]

To be fair, the current approach probably isn't much less annoying for them since it uses a custom compression scheme to reduce the size of that JSON blob with all the mod and channel data

[opal ferry]

Document the compression scheme? I think it should very much stay one way. I would be worried about possible security issues as well, as this wouldn't have a high barrier to entry.

[mental ingot]

The status indicator on the server list. Forge negotiates this early on. With the new configuration protocol now being way later, that information is lost and the screen is missing the ability to show, yes you can connect, or no you can't

[opal ferry]

I wouldnt want this to be a trivial way to DOS a server

[hasty yew]

I'd honestly just rip out the compat check anyways.

[fair sandal]

that is still very much on the table

[hasty yew]

That status indicator is something:

• many mods don't work with
• it isn't that informative as it doesn't tell you why you can't join
• doesn't actually save much time - you'll not be adding servers you don't expect to be able to join anyways

[buoyant pagoda]

"negotiates"? I understood it to be that the server sends along the information to the client (a one-way message, through the ping reply packet), and the client determines if it's compatible using the information given

[fair sandal]

I asked what the status packet contains but you didn't reply 😄

[opal ferry]

Either way its not something Fabric is going to use, we currently don't check that mods are compatible at login. (Registry sync does have some stuff though). let alone put something in the server list.

[mental ingot]

Ehm mod and channel information basically

What the server has

[hasty yew]

I guess the question I have is: when does the indicator actually save the user time or alleviate confusion?

[buoyant pagoda]

the compatibility indicator is something that's been contentious for a long time

[hasty yew]

If the answer is "almost never", which is my understanding of it, I'd rip it. If that's not the case, let's list out those scenarios so that we have a good idea of the most important use cases for it

And then design the implementation with that in mind

Either way, having an idea for what the goal actually is is important

[buoyant pagoda]

because

• mods usually don't respect it enough to make it useful
• users don't heed it because it's usually wrong (because of point 1)
• pack makers have no control over it (without a mod intervening) outside of yelling at mods to fix it (point 1)

[hasty yew]

Beyond that, users don't generally add servers they don't think they should be able to join

[buoyant pagoda]

and that the compatibility indicator has always had the caveat of "this is only an approximation -- you may not be able to join even if this is green"

[mental ingot]

Well with the new system it becomes more usefull

Because it indicates the channel compatibility

[hasty yew]

By letting them try to join and getting a better error message, it alleviates confusion

[hasty yew]

I don't think that's the main question. Like, step back a bit to the user's perspective: in what scenarios does the user:

• want to know whether or not they are able to join a server before they click on it
• if they can't, not want the in-depth message they'd get by actually trying to join that details why

[buoyant pagoda]

as an aside, I do think that we shouldn't put too much effort into this compatibilty indicator, in the sense of it not blocking the major facet of the networking rework that is the breaking changes to the API due to underlying rewrites

[fair sandal]

@winged rain weren't you the original author of the server status check? or am I misremembering someone else? how much value do you think it adds compared to letting the player just try to connect to the server?

[buoyant pagoda]

just a thought (I've not read the PR yet): how is it that the proposed spec says that the custom payload packet is used in reply during the status protocol, even though the status protocol only has two sets of packets (status request/response, and ping/pong)?

a comment on the PR itself at this stage: why in god's good gravy does IPacketFlowExtension exist?

if the answer to that is for use as method reference, I raise Predicate.isEqual(PacketFlow.CLIENTBOUND)

[opal ferry]

Just had a look at the how this works, its not as one way as I thought it was. https://wiki.vg/Server_List_Ping Its still dont think its worth the time/complexity. There also dosn't seem to be a custom payload packet.

[buoyant pagoda]

yeah, I don't think this proposal flies, in the face of (a) the added complexity to the conventially simple status protocol (which most if not all versions of MC recognize) and (b) the expansion of an existing vanilla packet (the custom payload packet) outside of its normal protocol use

[velvet whale]

also why was config stuff moved around in this PR

[mental ingot]

Link?

[velvet whale]

the Range class was moved

[fair sandal]

Isn't it a different range?

[mental ingot]

I thought I did not port that over

No I originaly moved that damn thing out for reuse in a different place

But I thought I moved it back

[velvet whale]

it's the exact same class

[mental ingot]

that is a derp

If you can send me a quick link

I will patch that back out

[velvet whale]

https://github.com/neoforged/NeoForge/pull/277/files#diff-22e2d8a773b31ee6309310ca8cfc3a1d42bb8efe12dcf57c52da4246f449e10f

also i see a writeset / readset extension method was added

readCollection / writeCollection do the exact same thing

[mental ingot]

Can we stop reviewing the draft?!?

Yes I added that before I realised those existed

[fair sandal]

I mean might as well clean them up at any time

[mental ingot]

Actually the reason why I said I would not accept reviews, is because that puts social pressure on me to get it to be clean

When I don't even have it working

As covers said before

This is a draft to see the progress and the direction and to provide input on the core changes

To to go hunting for format, unrelated changes etc

[fair sandal]

I think it's fair game to point out these issues already since we're gonna be merging the PR eventually anyway

And that might confuse people looking through the PR

[mental ingot]

Yeah but I don't want the feedback now

If you have the feedback, nice keep it to your self, put it in a gist

And when it turns from a draft to a pr

You can add it all at once

But not now

Piecemeal by piecemeal

That was the condition for me opening the draft

[fair sandal]

I don't know, I don't really understand that reasoning... might as well leave it where someone can see it. Maybe on the PR?

[mental ingot]

Instead of you having to burrow through the branch

[fair sandal]

Just so it's written down somewhere and kept track of

[mental ingot]

My point is the whole world seeing it

[mental ingot]

Yeah you can do that in a notebook / gist / somewhere were it does not put social pressure on me to work on this every free hour that I have

I already feel bad enough for sitting on the couch during my evenings

And not working on this all the time

And when I opened the Draft you guys agreed to that

If you can't hold yourselves to that

Then I will close the draft and reopen it only when I am done

[fair sandal]

I'd rather have you not feel bad and let us place feedback on the PR whenever we notice something. There's no pressure to fix that stuff immediately, it's just nice for other reviewers so that we don't all repeat the same thing

To get that straight, you really don't need to feel bad about the PR taking time

[mental ingot]

That is simply not how it works man

It is in this particular case an either or

XOR

[fair sandal]

Everyone agrees that the PR will be merged eventually. Each comment is just a little TODO note before that can happen

[mental ingot]

Yeah

And that TODO is what is the problem

Again

Please stop

[buoyant pagoda]

at this point, I'm not even sure what's asking for feedback

[mental ingot]

The core

The general idea

If the API can be improved

If I overlooked some usecase

That is what I want to know

[buoyant pagoda]

one problem I forsee is that the only true way to figure out needed improvements to the API would be to try it out locally
which isn't currently possible since, as you've said, the PR is not in a working state

[mental ingot]

Exactly!

It shows a concept

Of what is possible

At least in its current state

And how that concept is used in Forge itself

And what I want to know, from the core team, and everybody that is interested, is if that core concept and its examplary usage in forge, is good

[buoyant pagoda]

at this point, does the networking PR compile and run the game?

[mental ingot]

No, the major breaking poiunt there is the status packet

If we can solve that

There are some small compile errors remaining

But that is the big major one

[buoyant pagoda]

in what way does the status packet impede the progress of compiling and running the game?

[mental ingot]

It is kind of ugly wired in every where in the login process

Because the channel setup changed

That information is simply not there anymore

[fair sandal]

did you ever get to the packettype kind of stuff we discussed a while back?

[mental ingot]

No

It can be bolded on top I think

In a seperate BR

PR*

[fair sandal]

well the idea was to not need the builder lambda

[errant summit]

we are down to 32 compile errors and most of them are related to the login phase or the status packet

[mental ingot]

Yet several people including me, think it is easier for newcomers to understand

The packettype thing is not as easy to understand since it requires multple system to work together

Including multiple events

Which need to be wired up for registration

[buoyant pagoda]

can the compile errors on the status packet be alleviated by nooping it for now (with liberal TODOs to mark it for later)?

[mental ingot]

Maybe

[buoyant pagoda]

have you tried?

[fair sandal]

so that is the current registration API, right?

https://github.com/neoforged/NeoForge/blob/11b9e0b880d6737cc16381d9c7a783bc6134418f/src/main/java/net/neoforged/neoforge/network/NetworkInitialization.java#L27

[mental ingot]

Yes

[buoyant pagoda]

i'd like to see the networking rework be compiling and running the game, even if the extraneous parts like the compatibility indicator are currently nooped out (for re-adding later)

[errant summit]

I think we can have it compiling this week if shartte and Technici4n stop distracting him with #neogradle-dev stuff

[fair sandal]

lol

[mental ingot]

I mean if we yeet it for now

[buoyant pagoda]

in that sense, we'd see if there's any lingering bugs (non-compiler errors) with the implementation that need ironing out, which may cause the API to change

[mental ingot]

Then it can have it compiling later today

[fair sandal]

(note that .client and .server won't be on the same line easily once spotless is applied)

[buoyant pagoda]

please do so

[mental ingot]

Why does spotless care here?

[buoyant pagoda]

yeet it in the PR (with TODOs as appropriate), mark it in the PR description, and move on to making it compile and run

[fair sandal]

maybe if you place both .client and .server on a new line each it's fine

continuation indent is always 8

[buoyant pagoda]

once we have it compiling and running, people can then locally test the PR to see if the APIs are satisfactory

[fair sandal]

the set of parameters that we pass to the API looks good. maybe we can do something about the nesting

[errant summit]

If there is something that can be nuked, do so for now we can add it back later, I want this in before I go on vacation on the 26th

[mental ingot]

Okey

Hold on to your hats

[fair sandal]

it's worth yeeting it for now I think

was waiting for daemon's reply but idk when he will see it so let's go ahead with the removal for now

[buoyant pagoda]

question: why is the status packet not feasible now?

as in, what changed in the networking rework to make sending data via the status packet not feasible?

[mental ingot]

The moment that channel negotiation happens is much later

And the other way around

We previously did the negotiation on the client

[fair sandal]

another thing is that the point of the networking rework is to re-evaluate our hooks

[mental ingot]

And basically immediatly during connection open, aka status/login

[fair sandal]

I'd see it as a large audit of the networking setup as well, on top of the changes

(cause we need to audit it anyway to be able to change stuff correctly)

[mental ingot]

But now the logic is completely in the configuration phase

To allow for a forge client to be disconnected from a server

And reconnected to a different one

[mental ingot]

All old hooks should be gone

[fair sandal]

👍

[mental ingot]

Like this is nearly completely all implemented in the configuration phase now

[buoyant pagoda]

what prevents the server from serializing the information used in the channel negotiation in the JSON payload of the status packet, and the client from then using that information + its own information to make the best-effort guess of whether the connection will succeed on that negotiation

[mental ingot]

With some minor touches to how packet protocols can be build

[mental ingot]

The json payload size limit

[buoyant pagoda]

outside of that?

[mental ingot]

Not really anything

[buoyant pagoda]

(since we do have the truncated flag for that -- we assume that if the truncation happens, the information is incomplete and likely to fail)

[mental ingot]

That is why I suggested to use a round trip packet to pick it up later

[mental ingot]

That hole system is a hack at best

And I personally find the additional round trip a way better solution then the encoding shit

The overhead is minimal

[errant summit]

yea encoding json as a byte array and shoving that back into the json is ugly af

[mental ingot]

It is not just ugly but error prone

[buoyant pagoda]

the additional roundtrip packet is conceptually cleaner, but the current code is more acceptable in practice

[mental ingot]

Why?

External webpages that monitor those servers will have a heard time processing this

[buoyant pagoda]

for one, we aren't expanding a vanilla packet to outside of its normal uses

for another, we aren't requiring external webpages to literally implement the protocol we're making to communicate with servers on a round-trip basis (instead of implementing an encoding scheme in a packet they already know how to handle)

[mental ingot]

I would like to argue that servers which already implement the forge negotiation stuff, already do custom work, so the roundtrip is not really taht much different

[mental ingot]

Yes but we are for sure in a modded-modded connection

So I see no real reason why the scope of a vanilla packet matters there

[buoyant pagoda]

I say, there is a difference between decoding data in a JSON payload from a packet you already know, versus having to implement handling for round-trip packets
and you say "servers", but you mentioned (and I followed up on) "external webpages" -- which is it?

[mental ingot]

"external webpages"

I should indeed be more specific

[buoyant pagoda]

it's the idea of going past the bounds of the vanilla network protocol, which we've always tried to work within the bounds of
such as our use of the custom payload packets (iirc during login and play) for sending data between server and client (without defining our own custom packets)

[mental ingot]

In general yeah I accept that, especially since we want to stay compatible with vanilla clients / servers on the other end

But here it does not matter

[hasty yew]

My question about the utility of the server indicator at all still remains

So yeah I'd definitely say yeet it for now if it simplifies stuff

Can always add it back later if it's something folks decide to keep around

[mental ingot]

@errant summit Can you pull from remote

And push your changes

I just removed the whole bundle shit again

And I started removing the SSP

[buoyant pagoda]

don't forget to note that in the PR desc (perhaps as a checkbox), so we don't forget to decide on it later

[errant summit]

pushed, down to 26 compile errors

[mental ingot]

Fixing those now 😄

[mental ingot]

Down to 4 errors

Purely related to channel mismatch screen data

So that should be doable

Well we are in main menu...

Lets see if I can get into a world

[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: minecraft:network to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: minecraft:brand to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry_sync_start to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry to a client which can not accept it. Not sending packet.
[20:01:28] [Netty Server IO #1/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Somebody tried to send: neoforge:frozen_registry_sync_completed to a client which can not accept it. Not sending packet.

Well that failed XD

[fair sandal]

Lol

[mental ingot]

At least config locating works

But the netowrk negotiation failed to store the results

And the system considers the connection to be vanilla

Skipping that packet XD

[fair sandal]

Ok so not too far off ^^

[mental ingot]

Well yeah

Oef the client is not answering with its known channels

Well that is interesting

The netreg did not initialize:

I am willing to bet you the bus is not registered to yet with the EBS instances

So this has to be a mod bus event too

Yep yep

EBS is not wired up to the forge bus yet

Lets see if a mod bus event works

Yep that works

Lets see if I can get channels

[fair sandal]

Did you post to the forge bus?

[mental ingot]

I originally did

But somehow that had no listeners yet

[fair sandal]

I bet it was just shutdown and dropped the event

[mental ingot]

Even though it was an EBS

I made it a mod bus event

Fixed a payload id

And there we go

Now we have packets

There we go 😄

[fair sandal]

Nice

[mental ingot]

And real bug one

[fair sandal]

😄

Use it!!!

[mental ingot]

It at least shows the correct error message

[errant summit]

how's it going?

[mental ingot]

We are in world

Without errors

And without warnings that packets are unregistered

Lets see if we can connect to a server

Well that is a clear no

[errant summit]

if you push I could also investigate the issue

[mental ingot]

I am not really at home

Was doing this on the laptop

And that is now in the bag already

I will push tomorrow morning

The default networking is working

But something somewhere in the negotiation is failing

While it is working for in memory connections

Which is weird

[buoyant pagoda]

\o/ i knew it was a good idea to push for compile and run

[mental ingot]

Derped the config file packet implementation

But yeah fully working now

[mental ingot]

That is a sad state

Not sure what is sending state here

And there we go

Vanilla client connected to Forge server

[errant summit]

now try with test server and neo client (should not work)

[mental ingot]

Why?

I see no reason why that should not work

[errant summit]

mod missmatch test server has the testmods present the neo client not

[mental ingot]

Ah test server

Yeah have not gotten that far

Currently still doing the vanilla dance

@errant summit We have a problem on the modded client -> Vanilla server connection

SOmehow the brand payload is not arriving at the client

Causing us to not detect the connection as vanilla

Which does not load the default configs

Causing an immediate crash

Because the configs have not been loaded yet

[errant summit]

I think it's time to add runs to projects/base to debug the vanilla server

[mental ingot]

I am not sure

I thought it would appear first

Because it is in the code first

But it is just being skipped

And there we go

Added a fallback layer to detect the vanilla connection when features are send (one packet later)

@velvet whale Within the test framework you have a SimpleChannel implementation

From the code that seems unused

Can I rip that for now

And you add it back later?

Never mind

I figured it out 😄

Reimplemented this myself

All good 😄

BTW If somebody is wondering, how do I send a packet?:

This is it:

Ignore the sendPacketIfOn wrapper

That is part of the test framework

But to send stuff you just grab a packet distributor

And hit send

[velvet whale]

well i see you figured it out

[mental ingot]

@errant summit Did you do the OpenContainer and SpawnEntity stuff?

[errant summit]

I fixed the patch for entity type

I did not touch open container

[mental ingot]

I can't find the entity type stuff

I only see you deleting the PlayMessage stuf

[errant summit]

well if it were still used you wouldn't be able to compile

[mental ingot]

They are API endpoints for modders

And they are used in the test sourcset.....

[errant summit]

at least the packet for entity type alredy had a replacement

[mental ingot]

?

Ah yeeah you are right

I need to fix that though 😄

[errant summit]

I fixed the actual writing of the new spawn entity payload and registered it, for open container I think we need a slight redesign

[mental ingot]

Yeah

[errant summit]

ok I re-introduced the open container stuff

now we only need to wire up the entity one

[errant summit]

@velvet whale can you grab the branch and fix the test framework

[velvet whale]

orion said he fixed it?..

[errant summit]

there are some SimpleChannel usages so if he has he has only done it locally

[velvet whale]

not sure if i can find the time to do it, hopefully orion will be able to push it at some point tonight

[mental ingot]

I have it

@errant summit please push then I will update my sjit

[errant summit]

my state is pushed

[mental ingot]

Perfect I will pull in like 20

And then push my fixes to th test framework

[velvet whale]

that sounds more like shit than git

[mental ingot]

It was intended to be a mixture

[errant summit]

orion clearly meant hours because seconds and minutes are over

[velvet whale]

years?

[errant summit]

weeks and months come before that but it could also be centuries

and that is just pulling, not even pushing

[mental ingot]

I fell a sleep

It will happen tomorrow morning

I have the code

Just need to pull it

[mental ingot]

@errant summit Did I miss something but why did you leave this method empty?

[fair sandal]

does serverplayer not override it?

[mental ingot]

Nope

I implemented it now myself

[errant summit]

ServerPlayer does override it

[mental ingot]

Not that I can see

[fair sandal]

Forgot to gen patches?

[mental ingot]

It overrides the normal openMenu

But not the one with the additional payload buffer

I now added it myself

So it is not a biggy

Just fell over it

While going over my PR

Making it ready for review

[mental ingot]

I can't tell you why

[errant summit]

you must have forgotten to setup

[mental ingot]

But I don't have that patch

Nope

I just ran it

See I am on the latest version of the branch

And I don't have that patch

[errant summit]

you dropped the commit

[mental ingot]

What?!?

Ooooh

I rebased and force pushed

FUCK

What was in that commit

?

[delicate dawn]

git reflog time

[mental ingot]

So just that?

[delicate dawn]

export as .patch?

[mental ingot]

If it is just that change

Then I don't care

I implemented it now myself 😄

So it should just work

Well it does work

Because the test mods work

😄

But I stabbed myself in the foot there

Sorry schurli for the work that I wasted 😄

[errant summit]

you undid it manually in 02519e8cbd74a2224c86d638b0cd7b04e542cffa

[delicate dawn]

git has a --force-with-lease

[mental ingot]

Commit message?

[delicate dawn]

which prevents a force-push if the remote has new commits

[errant summit]

Fix imports in patches

[delicate dawn]

avoids overwriting someone else's work by mistake

[fair sandal]

currently porting MI from fabric to SimpleChannel lol

will the IDs be used anywhere besides the registration?

[mental ingot]

In the payloads

Each payload needs to return the same id

[fair sandal]

Yeah ofc

So fair enough

[mental ingot]

@here: We are now ready for review: https://github.com/neoforged/NeoForge/pull/277

[marsh loom]

Mamma Mia.

[mental ingot]

Here we go again 😄

[marsh loom]

I fully support you getting this done :p

[mental ingot]

No i mean that was a reference to Mamma Mia 😄

The musical 😄

And Abba

I thought you would appreciate that XD

[marsh loom]

Oh, yeah, lol

I understand now

[gaunt lark]

https://github.com/neoforged/NeoForge/blob/bf89bc2c1847beaa4df2c35549879cc6175ee2f9/patches/net/minecraft/client/multiplayer/ServerStatusPinger.java.patch Whitespace only patch?

[mental ingot]

Fuck I missed one

I did a pass over it to remove those

But lovely missed it

@gaunt lark as usual make a list 😄

[gaunt lark]

I don't think that was who you meant to ping there x3

[mental ingot]

@gaunt lark shite you are right

[gaunt lark]

https://github.com/neoforged/NeoForge/blob/bf89bc2c1847beaa4df2c35549879cc6175ee2f9/patches/net/minecraft/network/protocol/common/ServerboundCustomPayloadPacket.java.patch#L13-L14 A few places where there seems to be this kinda whitespace change, can't remember how important that is

[mental ingot]

IMHO Not really

But that is not up to me

I mean if the patch is just whitespace

Then yeah

Like the one you linked before

But in this case, IMHO it really does not matter

[gaunt lark]

Same with me, the only reason I raised it was because I didn't know how much that will effect porting

[mental ingot]

The white space?

Nothing

FUZZY Will simply ignore it

What I not have done yet is the AT stuff

I currently use the patches to make fields and shit public that I need

The ATs will be last thing after reviews, and feedback rounds

<@&797656813583466526> It might be prudent for you guys to start looking here as well, so we can provide adequate documentation when this goes live. Please start by checking out the above mentioned PR, and its description. And ask questions.

[gaunt lark]

https://github.com/neoforged/NeoForge/blob/bf89bc2c1847beaa4df2c35549879cc6175ee2f9/patches/net/minecraft/server/MinecraftServer.java.patch#L128 Left over todo? Is it worth marking that with Neo or removing it
https://github.com/neoforged/NeoForge/pull/277/files#diff-6c6e6f18f4aa2f4332d0d1e8fa59c284837db6623f3b1f0ec9eb48dd4cd368e4R187 reaon

[mental ingot]

Which line the TODO?

[gaunt lark]

Whoops missed adding the L128 on it

[mental ingot]

It needs to be a //Neo TODO

It is not a //Neo: comment style thing

It is a real todo

That eventually might need to be addressed

[gaunt lark]

Think that was it from just a quick scan through, nothing jumps out to me, I'll try to find the time to sit down and dig into it more

[mental ingot]

nICE

Just leave the comments 😄

[gusty iron]

On a side note is there any way for us to have automated test for this?

[mental ingot]

Not really.....

What would you want to test here?

This is more of a convention thing

I am willing to put in the work to add the tests

But I am personally pretty unsure as to what

[gusty iron]

Well idk but like... Registering and sending a packet... All clients get the packet that need to etc etc.
More like a quick way when porting to see if it broke fully or not

Also does this contain the limited compatibility stuff with fabric? Cause in that case it would be interesting to test those cases as well

[mental ingot]

That already happens in many manual tests we have, automating that is pretty difficult. @velvet whale Any ideas here?

[mental ingot]

Yes it is implemented, but again tested via several tests all over the place

[prisma garden]

Will do so this evening.

[velvet whale]

Ish? you can attempt to send the packet to a mock player and then check if the dummy netty channel contains the correct packet object

but that's as much as you can automate reasonably

[mental ingot]

Do you have an example?

[velvet whale]

fence gate test in block tests plays an open sound to the player when right clicked and then checks if the sound packet was "received" (more like sent tbf)

[mental ingot]

Okey

I will need to figure out how that is done

And how I could check for network initialization code

But I might look at it

No promise though

[fair sandal]

TIL that github markdown can do this

[mental ingot]

It is new

Maty said that Documentation should be left to parchment in vanilla classes.......

I don't quite understand that logic

We do this in different places

[buoyant pagoda]

it's existed for quite a while as a public beta, and only recently became generally available (as in "this won't change without a good reason and advance warning, etc.")

[mental ingot]

Allthough I would agree that once we use that would be a valid argument

But we are not yet

So how do we want to deal with this

[fair sandal]

you should only add documentation if it's neoforge-specific, IMO

[mental ingot]

Yeah I add neoforge specific overrides

And I explain why, while also documenting the vanilla code

Because I noticed during porting, that knowing why something was overridden, or added, so I added the same kind of documentation to vanilla

[fair sandal]

I'd remove the vanilla doc as it will be duplicate once we introduce parchment support (which looks like it will happen soon)

doc for neoforge overrides is good though

[iron ivy]

👀 I will review this tomorrow when im more awake. :D

[mental ingot]

My point is: parchment documentation might not be there when porting, in particularly when you are looking at the patches

[iron ivy]

documentation in neo specific overrides and hunks seems fine imo

[mental ingot]

Yeah

[mental ingot]

but I also added some documentation to the vanilla method that I override

[fair sandal]

the @deprecated is enough IMO

[mental ingot]

Okey

Will modify the patches

[iron ivy]

Usually we do @Deprecated // Neo: Use xyzblah becuase asdf

[fair sandal]

also Orion you need to remove trailing whitespace 😛

normally intellij does it automatically when saving files so I suppose that you have that disabled

[mental ingot]

Where?=

[fair sandal]

between the methods here

there might be other instances

[mental ingot]

Sure, but again it does not matter in reality

[iron ivy]

The first one does, its a removal

Its not required in the patch

[mental ingot]

Yeah

[fair sandal]

it's just nice for consistency... if you press ctrl+s in intellij it should remove all trailing whitespace automatically

[iron ivy]

But i though we were matching vanilla output in patches, which does not have trailing whitespace

[mental ingot]

I tried that

No dice

[iron ivy]

Its in your formatter then

[mental ingot]

I don't have a formatter enabled in this repo

[iron ivy]

Probably in your default formatting rules

[fair sandal]

look at this

(bottom, remove trailing spaces on save)

[mental ingot]

That does not work though....

We are allowed to keep trailing spaces in none patched files

[fair sandal]

does spotless not remove them?

[mental ingot]

No

[fair sandal]

ah, mc sources

[mental ingot]

Because you are in patched space

[fair sandal]

well they shouldn't have trailing spaces to begin with

[mental ingot]

Which is the whole problem here

Yet it does

I would propose to not care for the trailing whitespace

The patcher should not care

And in reality they are not in the way during reviewing in my opinion

[fair sandal]

it's a pain because someone modifying the file again will change the patch

[iron ivy]

I don't think its wise to just disregard formatting rules because you dont feel like its relevent

[fair sandal]

this happens more often than not in NG commits as well where you reintroduce a bunch of whitespaces

[mental ingot]

I will address it

[fair sandal]

😛

just something I noticed, then the next contributor just undoes all of that

weird that the option in IDEA doesn't work?

[mental ingot]

Again, I personally don't care about whitespace shit, and I don't care if the next one undoes it.

Because during reviewing I just simply ignore them

I look at the actual changed code

[fair sandal]

well it adds useless lines to the diff, and can cause merge conflicts etc

[mental ingot]

I have it enabvled

It just does nothing

[iron ivy]

The bottom one, disable

or..

wait

[mental ingot]

I have the same setup as tech

Yet it literally does not matter

[buoyant pagoda]

you may not personally care, but apparently we do

[mental ingot]

Agreed, so I will be addressing it

[fair sandal]

try pressing ctrl+s

[mental ingot]

Once we figure out what is going on

[mental ingot]

Just did, no dice

[fair sandal]

for me if I add whitespace and then press ctrl+s it gets removed

[buoyant pagoda]

as an interesting note, iirc git diff shows trailing whitespace in bright red

[fair sandal]

it does yeah

or, I suppose it's vim rather than git

maybe try applying it to all lines instead of only modified lines

[mental ingot]

Spotless does not even have a "whitespace trail" check...

Oh yes it does

Hold on

Testing

[buoyant pagoda]

wdym it's vim

i don't recall having to :q! my way out of a git diff screen

[fair sandal]

the default git editor is vim

you can :wq well no it's read only

or ZZ

or :q

[buoyant pagoda]

for Vim, I just memorized :q! and :wq
i'm not likely to go use it for anything more than "it's the default tool for something" (or "it's literally my only option")

[iron ivy]

vim > nano

fight me

[buoyant pagoda]

[mental ingot]

Okey made at least a minimal formatter for the patched code to remove the trailing whitespace

Lets see if this does not fuck everything up

[buoyant pagoda]

I literally installed nano locally so I can use it in the (rare) case I need to edit something from the command-line

perhaps one day I'll learn to use vim

[mental ingot]

@buoyant pagoda and @fair sandal We have a lot of trailing whitespace stuff in the code:

For example here

[iron ivy]

Is this vanilla?

[mental ingot]

Yeah

[iron ivy]

VF bug then

[fair sandal]

[buoyant pagoda]

interesting

[mental ingot]

I added a trivial check in the spotless formatter

Which finds 165 files that have this behaviour

Over half of them

And a lot of them are not patched

Like that

Which is why IDEA is being fucky on my end

[buoyant pagoda]

any commonalities between those files?

[mental ingot]

I have formatting copy enabled

[iron ivy]

Okay, well just make sure its not inside your patches, we can get a VF bug fix later

[mental ingot]

It sees the indenents that come from VF

And then just uses them

[buoyant pagoda]

any chance you can send over a patch file for the changes needed to implement Spotless for the MC source? so we can replicate it locally, and potentially narrow down commonalities between source files with the bug ourselves

(i've sent a message in the VF discord to probe about this)

[mental ingot]

Subject: [PATCH] Clean up some patches and remove unused code.
---
Index: build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build.gradle b/build.gradle
--- a/build.gradle    (revision bf89bc2c1847beaa4df2c35549879cc6175ee2f9)
+++ b/build.gradle    (date 1702991092487)
@@ -64,6 +64,7 @@
 
         endWithNewline()
         indentWithSpaces()
+        trimTrailingWhitespace()
         removeUnusedImports()
         toggleOffOn()
         eclipse().configFile file('codeformat/formatter-config.xml')
@@ -89,4 +90,11 @@
         }
         bumpThisNumberIfACustomStepChanges(1)
     }
+    format 'patched', {
+        target project(':neoforge').fileTree('src/main/java', {
+            include "**/*.java"
+        })
+
+        trimTrailingWhitespace()
+    }
 }

@buoyant pagoda That should do it

then run the spotlessPatchedCheck task

[wary solar]

Is it still possible for the sender to check if a channel/mod exists on the receiving side so it can decide not to send a packet?

[mental ingot]

Technically but it is not exposed

[wary solar]

that's never stopped me before
good enough

[mental ingot]

In practice, the channeling system takes care of not sending payloads that are not known to the oposite side

[wary solar]

ahh

[mental ingot]

So you really don't need to worry about that anymore 😄

[buoyant pagoda]

IIRC, mods did use that functionality

[wary solar]

okay that's an improvement then

[buoyant pagoda]

JourneyMap I believe -- iirc Mysticdrew made a PR about that sometime in the past

[mental ingot]

Yeah but it should not matter anymore

You can just tell it to send the packet

It will immediatly noop

Because the channel has not been agreed upon

[wary solar]

yeah previously I'd have to check isRemotePresent or the receiver would logspam

[mental ingot]

Yeah

Won't happen anymore

It won't even write the payload

At worst you now build the payload object

So there is some memory overhead

And there is technically a way to figure this out from the Connection object

As the PR specifies

It is all stored on the connection now with attributes

[iron ivy]

Its not entirely about that. Some mods may wish to do different things on each side based if their companion exists.

[mental ingot]

So if you have a connection, you can use the internal API and Attributes to get the connection state

[mental ingot]

I am considering firing an event

That indicates what the results of the negotiation where

Just have not decided how

It makes sense to me

[iron ivy]

That would be sufficient

[mental ingot]

That it should be possible

[fair sandal]

yes

something like this will probably be needed for syncable chunk data attachments in case they are used with a vanilla client

IMO storing the negociation result in a place that is easily accessible is the nicest

[iron ivy]

Yeah, seems reasonable to me honestly, event may be slightly overkill, just being able to query doesChannelExistsOnOtherSide(RL) is good enough

[mental ingot]

Any custom data we send, should always be a seperate payload

Not part of the vanilla packet

Because it literally runs into the vanilla client problem

[fair sandal]

well that's why I want to know if the client is vanilla or not

[mental ingot]

Yeah, but you should not need to know that!

That is the point

[fair sandal]

there are cases where it's useful to know

[mental ingot]

Not really

[fair sandal]

another example I can give you is custom ingredients with a vanilla client

[wary solar]

you don't know that

[mental ingot]

Unless you are creating specifically different behaviour if your partner is there

[fair sandal]

it's more about gracefully handling vanilla clients

(or clients that don't have the mod present)

for example with fabric's custom ingredients, if the client doesn't support the ingredient type then it gets dissolved and sent as a vanilla-style ingredient automatically

(since the server does all the matching it's fine anyway that the client doesn't get the full picture)

[mental ingot]

Forge does this on an ingredient level, not on a connection based level

[iron ivy]

Hmm, I think I kind of agree with Orion here, its not exactly useful to know if the client is vanilla. But I also don't see any reason to lock that down. It should be just extra data given on the event/whatever where the channel sumarry is given.

[mental ingot]

You know if the client is vanilla

As I said

That is exposed

[fair sandal]

on fabric you can use an NBT ingredient with a vanilla client

[mental ingot]

But I simply don't agree that forge should expose systems that would make networking protocols different whether or not a vanilla client is connected

[iron ivy]

That should be the mods choice?

[mental ingot]

A mod can do that

[buoyant pagoda]

just to clarify, "vanilla" here means "not understanding the NF handshake/negotiation protocol" right?

[mental ingot]

For all I care

[fair sandal]

the point is that vanilla can't handle modded protocols and it might be desirable to implement a fallback for vanilla clients

[buoyant pagoda]

not actually determining if the other side is full vanilla, nothing bolted on (no mod frameworks of any kind which mess with the brand)

[mental ingot]

Simply said: The payload and packet have no idea who they are sending to.

And might even be used in a scenario where they are written to multiple IO buffers for different client types

[fair sandal]

that is the mod author's responsibility to handle

[mental ingot]

In other words, trying to squeze that idea into the payload/packet is going to get you into trouble from the get go

[fair sandal]

if someone wants to send a message to a specific ServerPlayer, they can check what that specific player supports

[fair sandal]

the info should be available from the ServerPlayer imo

not at the payload level

[mental ingot]

It is

Well you probably can get the raw underlying connection

And hack it to get the information out

Right now

It only exposes whether the configuration listener is vanilla

But not at runtime

At runtime

You have no idea what the other side is or is not

And I can see that as a problem

So I might actually add that functionality

So that you can query the network setup

[fair sandal]

a simple boolean ServerPlayer.canHandlePacket(RL) would be sufficient for most purposes I think

(and a similar thing on the client side to check if the server can handle a packet, although that is generally less useful)

[mental ingot]

Exists within the NetReg

Well it exists as packet

[iron ivy]

Exists as in the data is there, or exists as in its already accessible and we are wasting our time going over this again and again?

[mental ingot]

I should probably expose that for a specific payload

[mental ingot]

Exists as in, described in the PR as mentioned are exposed public internal APIs

And if people want to use that

[iron ivy]

I feel like ive asked the same question more than once and gotten a different response each time

[mental ingot]

Then then are free to do so

Okey

It does not exist directly exposed on the RL level

It exists on the Packet object level

But only in an API marked as Internal

Does that make sense?

https://github.com/neoforged/NeoForge/blob/a520c36685b0e666533badb7eaf886f2d1e4ad95/src/main/java/net/neoforged/neoforge/network/registration/NetworkRegistry.java#L590

[buoyant pagoda]

"public internal APIs" is an oxymoron

[mental ingot]

Nope

they are public, because they need to be for compilability reason

They are internal because we do not give any compatibility guarantee on them

[buoyant pagoda]

their access level is public
but they are an internal API, not a public API

[mental ingot]

Yes, which is what we mean with a public internal API

It is not an oxymoron

Because public and internal have different scopes

[iron ivy]

Internal here meaning: Not intended for mod consumption

[mental ingot]

I am not sure how we officially handle: @APIStatus.Internal

[buoyant pagoda]

["public internal API" is] an oxymoron when "public" is interpreted as "intended for consumption by 3rd parties"
which is how I interpret "public" when used with "API"

[mental ingot]

I thought we set it as, you can use it on your own, but we don't given API compatibility guarantees on this

[buoyant pagoda]

my understanding of something being part of the internal API is only the "we don't give API compatibility guarantees on this" part

being public is usually a side-effect of needing to be visible for our internal use

[mental ingot]

What I am planning on doing is making a public endpoint for this on the listener

Kind of like the "isVanillaConnection" endpoint

That then calls the internal endpoint

[buoyant pagoda]

what do we define as "vanilla" in this context, for the record?

[iron ivy]

I'd assume 'Not Neo'

[fair sandal]

which means that you are not supposed to use it

for me @ApiStatus.Internal means "we need this public for reasons™️ but don't use it"

[buoyant pagoda]

if we could, we'd make our internal methods only accessible to NeoForge

but we can't (it ain't module time for us yet), so we get by with @ApiStatus.Internal

looks at event constructors

[mental ingot]

For me it always ment: This is public for our use, however we can not prevent you from using it, so feel free. But don't expect us to care for your compatibility

[wary solar]

🤷‍♀️ that's never stopped modders before

[indigo hazel]

Just exposing whether the connection is vanilla is not sufficient. If we take JEI as an example, they need to disable certain features such as crafting table auto-fill when the server doesn't have the mod even if it is a modded server

[mental ingot]

I know

[wary solar]

https://xkcd.com/705/

xkcd: Devotion to Duty

The weird sense of duty really good sysadmins have can border on the sociopathic, but it's nice to know that it stands between the forces of darkness and your cat blog's servers.

[mental ingot]

I think for features it is perfectly fine to know

But people should refrain from doing that for networking

[fair sandal]

yes, basically the same thing

well JEI needs to disable its feature based on networking 😛

[mental ingot]

That is fine

Feature wise it is okey

[fair sandal]

well to do this they need to query "can the server process this packet"

[mental ingot]

But modifying your own intenral network protocol, as in the structure of the bytecode that you send that is something I am highly suspicious of

[mental ingot]

As I said, I am going to expose it somehow

Right now it is only "exposed" on a Packet level

[fair sandal]

for synced chunk data attachments what I had in mind was:

• either bundle packet with chunk data + attachment data if the client supports it,
• else normal chunk data (as in vanilla)

[mental ingot]

Bundle all the way

Don't even try the second route

It is really not worth the headache

[buoyant pagoda]

perhaps @mild spruce would be interested in this discussion (as he did a PR somewhat related to this in Forge)

[mental ingot]

My proposal would be:

isConnected(ResourceLocation) on the listeners

[errant summit]

yea a check like for loaded mods would also be my idea

[indigo hazel]

What would even be the point of spamming a vanilla client with the attachment data? Depending on how you intend to append the data to the vanilla chunk packet, it would also cause the client to disconnect outright because the buffer isn't empty after reading.
Putting a separate packet into a bundle has the benefit that you can re-use the exact same packet for later re-syncs when the data changes without having to send the entire chunk data again

[mental ingot]

Yep, additionally, if you do it as a seperate bundle, you can just use a packet distributor

And send the custom bundle / payload in one go

Because the core mechanics mean that the NetworkRegistry will filter them out

Allthough I might need to make a followup PR

To deal with the Bundles better.....

Their support is still pretty crappy

[fair sandal]

where did I say I wanted to append data to the vanilla packet?

if vanilla can't handle data attachments they should not be sent at all

[indigo hazel]

The second point sounded like you intended to append the data to the vanilla packet

[errant summit]

wdym with data attachments

[fair sandal]

synced BE/chunk/entity data attachments

[errant summit]

just send your own payload
you can't modify a vanilla packet at all because it checks the expected length

[buoyant pagoda]

no modifying structure of vanilla packets

we've had this already with the intent packet

[indigo hazel]

That's the intention behind the first point Tech made. We can use a bundle packet to combine the vanilla one and the custom one and the client will be none the wiser as bundle packets are handled transparently half way up the netty pipeline

[fair sandal]

exactly

[buoyant pagoda]

bundle packets sound so neat