Networking Protocol

[fair sandal]

the point is that it is pointless to send that data to vanilla clients so might as well have an easy check

[errant summit]

makes sense

[mental ingot]

Yeah

Which you again don't need to perform:D

Because the connection does it for you

Keeping your sending code as clean as possible

[errant summit]

the sending code discards the payload if the client does not have the payload registered?

[mental ingot]

Yep

Automatically

Currently for testing it logs with a warning

In the future that will be a trace message only

[errant summit]

well then the check is only needed if it requires expensive calculations

[mental ingot]

At most

Because in most cases you will do something like:

PacketDistributor.ALL_AROUND.args(new BlockPost(x,y,z).send(payload)

So you do not need to do any loops or what ever

[fair sandal]

does it discard parts of a bundle packet too?

[mental ingot]

It is something I need to test 😄

I don't 100% know

As I said

[fair sandal]

in an ideal world it would also de-bundle automatically if it went from >1 to ==1 packet inside the bundle 😄

[mental ingot]

Bundle support is limited at the moment

Because of time limits

But it is for sure something I want to support

[fair sandal]

ok great

as always: no rush

[mental ingot]

I added both points to the TODO list 😄

[fair sandal]

nice

good stuff

[errant summit]

probably not because bundles are handled via PacketBundleUnpacker and PacketBundlePacker and discard happens earlier on the Connection#send level

[mental ingot]

Yeah which is why we need some hooks in there 😄

[indigo hazel]

Does it discard if the packet is sent the way vanilla sends packets through Connection#send() or only through the PacketDistributor?

[errant summit]

technically we only need to filter the Iterable<Packet<T>> subPackets in the BundlePacket

[mental ingot]

It hooks into Connection#send()

[mental ingot]

Not really

That is the raw bundle

We need to filter what the bundle writes to the connection

[mental ingot]

Actually no

It hooks into the listeners send method

[errant summit]

which is done in BundlerInfo with a subPackets().forEach

[mental ingot]

Then we should likely hook there 😄

[mental ingot]

Hooking into connection might be a good idea

Unsure

[indigo hazel]

Right, which is what vanilla also uses. I got confused for a moment because the fields holding the listener instances are also called connection

[mental ingot]

Yeah

You can technically still go to the raw connection object xfact

And then send it anyway

Which was kind of done on purpose

[errant summit]

it currently forwards the parameter it gets from unbundlePacket which is a method reference to list::add

[mental ingot]

Well

Then we can easily add the filter there 😄

[errant summit]

technically we should add our own MessageToMessageEncoder<Packet<?>>into the netty pipeline that does the filtering

[indigo hazel]

That's what the VanillaPacketFilter used for packet splitting and some other stuff already does

Could potentially re-use that

Well then, time to build this thing and throw it at a userdev project and hope it works this time. I tried last night for shits and giggles and ran into a very strange recompile error in userdev: #squirrels-🦊 message

[indigo hazel]

For some stupid reason this change breaks recompile in userdev: https://github.com/neoforged/NeoForge/pull/277/files#diff-dc38a75ea023230e33017c6c53b2344bd301873665afc3d84e9d9508e26c9ddcR249. If I revert it back to the size check, it recompiles fine

[mental ingot]

WTF

[steady sonnet]

what do you mean by breaks? error?

[indigo hazel]

See the linked message in squirrels, it can't find the method

That's the entire error I get

[steady sonnet]

@since 2.10.1

is it possible there's a GSON version mismatch somewhere?

2.10.1 was released in january, but maybe we didn't bump it somewhere

[indigo hazel]

The userdev env in question lists GSON 2.10.1 in the External Libraries

[mental ingot]

I have 2.10.1 loaded

Force 2.10.1 even

Not sure if we force that on userdev

But if we did not

It would fail for everybody

So I doubt it

[indigo hazel]

The version JSON of vanilla 1.20.4 also lists GSON 2.10.1

[indigo hazel]

I at the payload handlers. The context comes first which means you can't use a method ref of a non-static method as the handler: https://github.com/neoforged/NeoForge/pull/277/files#diff-7af44baeb0850ab9660d8d55083a892c3eaf61d5ee85296911648846820db94bR24. Can we turn these params around in all three handlers, please?

[errant summit]

I'll swap them

[indigo hazel]

Thank you 👌

[fervent token]

I see that as well

going to try updating this line for science: https://github.com/neoforged/NeoGradle/blob/NG_7.0/gradle.properties#L19

given maybe neogradle when doing neoFormRecompile uses its own versions?

nope that didn't help

I wonder if https://github.com/gradle/foojay-toolchains/pull/52 is at all somehow related

[mental ingot]

I did not bump GSon this build though

Are you guys sure you do not see this issue in a normal build.....?

[steady sonnet]

interesting, LexForge added a very similar block when updating the 1.19.2 MDK to Gradle 8 without documenting why

[fervent token]

with it using configureEach instead of all (so that it doesn't have a stroke from resolving things too early) that block doesn't seem to actually change the fact of if that error occurs or not

[mental ingot]

It can't matter

You get an error on recompile

The compiler is for sure not using an NG dependency

It is a red hearing here

[fervent token]

yeah probably. I was just experimenting a little to see if there was an off chance with a bug that it somehow maybe was grabbing it from the wrong place

[indigo hazel]

Found the first bug: if you connect to a server, get disconnected in the config phase and then try reconnecting, then the client is spammed with this (might take a few tries, but almost always happens on the first reconnect already):

[18:37:32] [Netty Client IO #2/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Received a modded custom payload packet neoforge:frozen_registry_sync_start that has not negotiated with the server. Not parsing packet.
[18:37:32] [Netty Client IO #2/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Received a modded custom payload packet neoforge:frozen_registry that has not negotiated with the server. Not parsing packet.
...
[18:37:32] [Netty Client IO #2/WARN] [ne.ne.ne.ne.re.NetworkRegistry/]: Received a modded custom payload packet neoforge:frozen_registry_sync_completed that has not negotiated with the server. Not parsing packet.

and gets stuck at Joining world. Once this occurs once, it repeats until the client is restarted. I don't know whether this also continues if you connect to a different server though, I have only tested with one. Whether the disconnection happens on the client or on the server does not make a difference.

[errant summit]

ok so we forgot to reset some state on disconnect

[mental ingot]

Weird there seems to be an object on the connection that holds the communication

[mild spruce]

I saw I got pinged here, I will take a bit to read through this thread and chime in if needed.

[prisma garden]

here, yes

[indigo hazel]

Is there any particular reason why this is a plain Player and not a ServerPlayer if it's only ever going to be non-empty on the server anyway: https://github.com/neoforged/NeoForge/pull/277/files#diff-b993dd91adbc1836a354e5f95d029e7d16a78a68f6fae099b4255108d36522d5R31?
Also, there's a typo in the docs of that field, it should say ... will only be present if ... instead of ... will only be empty if ....

[mental ingot]

That is a good question

Probably i just copied stuff from the old code somewhere

I remember making it a TODO for me

But not really something I thought of after that thought

[mental ingot]

Outside of this issue, how is the API feeling?

[indigo hazel]

Very clean for the things I've tried (both simple packets and the config shenanigans)

[mental ingot]

Good that was the aim

[indigo hazel]

Maybe I'm being unnecessarily paranoid about streams in potentially "hot" code, but using them to look up the channels by payload ID feels like poor design to me when it could instead be a map lookup: https://github.com/neoforged/NeoForge/pull/277/files#diff-ce06ac58ce8fa1b936e891f029fb1d68fc42e0ec8c8b550e20bbc20103a0e966R190-R193 (same thing for the other nine cases this is done)

[mental ingot]

Fixed 😄 Teamwork makes the dreamwork :D:

[indigo hazel]

👌

[fervent token]

Not fully sure the docs on PlayPayloadContext is correct for the implNote in regards to when sender is empty as it seems to be like it used to be which is when receiver is the client.

[indigo hazel]

Yeah, I mentioned above that they are the wrong way round

[fervent token]

also as a side note it would be slightly convenient to provide Minecraft.player as part of the context on the client side

to avoid having to do things like:
LocalPlayer player = Minecraft.getInstance().player;
to avoid the class loading issues

granted on a quick skim I think I may use the level than the player more so maybe that doesn't actually help a ton. But certainly I think is something that is at least partially worth thinking about

[mental ingot]

Okey, just added support for querrying the payload transmittability and @fair sandal bundle packets transmitted are now filtered and unpacked if there is only one packet left.

[fair sandal]

Really good stuff

Thanks

[fervent token]

is a replacement for NetworkHooks#getEntitySpawningPacket? Like I see AdvancedAddEntityPayload but that doesn't help with overriding getAddEntityPacket

[mental ingot]

Entity#sendSpawnPacketTo(..)

[fervent token]

if you are saying to use that instead when adding the entity to the level... I don't even know where the hell the vanilla spawn packet is being sent and if I had to guess it might be inside vanilla code

[mental ingot]

Ehm

[fervent token]

but if you mean that handles it for me... neodev doesn't show that method as ever being called

[mental ingot]

Maybe I fucked something up here

I might have fucked this up

No

I can't find a caller for getEntitySpawnPacket

So I just added the method on the entity extension

Are you trying to tell me there is supposed to be a caller somewhere?

[fervent token]

I don't know??

that is what I am asking you

as in

where am I supposed to call it from

[mental ingot]

I have no idea

I just readded the endpoint to trigger the sending

I have no clue how this mechanic worked in the past

[fervent token]

ServerEntity#sendPairingData is what calls Entity#getAddEntityPacket

so like I don't understand how I replace that with the neo packet

[mental ingot]

At the moment

Neither do I

I will do some digging

And get back to you

[fervent token]

okay

[mental ingot]

@fervent token That method is currently completely unused on 1.20.2.

I have literally 0 idea how to use it

Where you using it in the past

?

[fervent token]

Entity#getAddEntityPacket

I was overriding it to return NetworkHooks.getEntitySpawningPacket(this);

I am asking what do I do now

for cases where I am implementing IEntityAdditionalSpawnData

(a couple cases I could remove as I noticed I wasn't actually implementing or using that interface, but I have two cases I do)

[mental ingot]

Hmm

that is a very good question

Let me dig 😄

Soooooo

@fervent token You dug up a stinky bomb 😄

[fervent token]

your welcome

I guess?

[delicate dawn]

better to find it now than later :P

[mental ingot]

Well

It is fixable

But really annoying

[delicate dawn]

what's the issue?

[mental ingot]

The entire PR is designed around modders not needing to send raw packets

But work entirely with payloads

The pairing data system

Works with a bundle

And solely accepts Packets

No payloads

[delicate dawn]

oh I see I scrolled up

hmm is IEntityAdditionalSpawnData the only use case currently for the custom spawn packet? or does mc still have entity types hardcoded in the client?

[mental ingot]

No clue

But it is not relevant for this case

[brave rose]

Are composed bundle packets flattened?

[delicate dawn]

well it could be relevant, if instead of using a custom packet returned from getAddEntityPacket, neo would process IEntityAdditionalSpawnData using its own packet sent in the same bundle right after the spawn packet, but I suppose that would be a separate refactor to be done

[mental ingot]

Probably, if vanilla does it, then we do it

I just added the filter to the encoder

[mental ingot]

That is kind of what I am trying to figure out

[mental ingot]

No: they are using the registry

[delicate dawn]

ClientPacketListener#handleAddEntity doesn't seem hardcoded in the way it used to be

[mental ingot]

So I could technically hard replace it

[brave rose]

No, vanilla doesn't. I am suggesting that Neo patch the bundle packet to flatten composed bundle packets.

[indigo hazel]

Vanilla sends a bundle start packet, the bundled packet and then a bundle end packet, they aren't all sent as one monolithic packet. The bundle stuff is just a neat trick to guarantee that all related packets are handled in the same tick, but they still go over the wire separately, which I assume is what you mean by flattening them

[brave rose]

No, I should clarify

As far as I remember, the bundle packet constructor receives or otherwise internally creates an ordered collection (probably a List) of packets inside the bundle. If there is another BundlePacket in that collection, it may not be sent over correctly. By flattening, I mean that the inner bundle's packet list of packets should be inlined into the outer bundle's packet list so all packets are sent properly.

Quilt uses a mixin to allow this. Patching or modifying the class isn't entirely necessary. A helper method could likely also accomplish this.

[mental ingot]

I don't see a reason why that would happen......

Well you are right

If the inner packet is a bundle

Then that will fail

[indigo hazel]

Yeah, if you nest bundle packets, then the client will get confused because the exact same packet is used as the start and end marker, making them indistinguishable

[mental ingot]

Yep

I am not sure that is something we should fix

Vanilla even protects against this case

And throws an exception if it happens

I feel like that is a good indicator that mojang does not intend to support it

[indigo hazel]

Where? I've been searching for that exact thing 😄

[mental ingot]

At least that is how I interpret it

[indigo hazel]

I don't think that blows up for nested bundles, I'm pretty sure that's for some other edge case where a protocol change happens while receiving a packet bundle or something along those lines

[mental ingot]

Hmm

Also possible

[fair sandal]

Entity spawn data can be sent as a separate packet in a bundle IMO

[brave rose]

I agree

[errant summit]

yep

but how do we get the entity being added in the handler then?

[fair sandal]

Heh good question

I think Quilt does this so you can have a look

Maybe the entity has a uuid that can be used to identify it

[delicate dawn]

it should have a network id?

after the spawn packet runs, it should be in the client's entity map

a bundle still runs the packet handlers one after the other, I presume?

[fair sandal]

yeah

[mental ingot]

That is not directly the point

The point of this system is that the entity gets spawned with the custom data

Immediately

Through s custom creator on the entity type

[fair sandal]

IMO it's fine if the data is received in a different method call

but yeah, spawning it immediately would be nicer

[mental ingot]

Yet it would be a major regression of the code

[fair sandal]

"major regression"

you mean trivial refactoring? 😛

[mental ingot]

No

Because you need a completely different way of processing the incoming custom data

And you now can not use this creator to create a specific client side entity

The basic concept was that this extension point is only called client side

And would allow you to create a sub type instance of the entity specific to the client

If we change that include the data in the bundle and not in the actual spawn packet

Then that becomes impossible

[fair sandal]

hmmm

giving out the data immediately is a nicer pattern IMO (like we do for Menus)

[mental ingot]

Yrah

We can relatively easily fix this

[fair sandal]

can't we add a new method, something like writeAdditionalEntitySpawnData(FriendlyByteBuf buf)?

[mental ingot]

Nope

[fair sandal]

and of course we'd still need a different packet if something is written

[mental ingot]

Yep

I have an idea

Which should be trivial to implement

[fair sandal]

this is a bit concerning design IMO

[mental ingot]

But that was how it was designed

You returned your custom spawn packet

And that is that

But I have a much better idea on how to do this

Just need some time

I have picked up a throat infection

So the starts of my day are late

And filled with a metric ton of teas

[buoyant pagoda]

why is it concerning?

[fair sandal]

well overriding a method just to return a forge-added packet instead is not very clean design IMO

[dim wagon]

My question is

Why cant it be player and be possible on both sides

[fair sandal]

there's a chance that the forge-added packet was only introduced after the custom packet method existed though

[dim wagon]

Make it an optional and try pass client.player if on the client side receiver

I do that in ml and I often wonder why its not done in loader

[errant summit]

currently the customClientFactory in the EntityType looks because it gives you the payload instead of the extra data

[mental ingot]

For fucks sake

The bundle does not accept custom packet payload packets

[errant summit]

oh that problem again

(time to yell at fry again?)

[buoyant pagoda]

even if we yell at our local Mojangstas, we'd only get any such fix in the next MC version

[mental ingot]

I implemented it now

[errant summit]

at least we don't have to patch the generics of each use from T to ? super T

[mental ingot]

It is still limited to the Play phase only

So no config phase bundles

But I will look into that next

[errant summit]

so time to role ping Problem Causer and ask them to fix it for 1.20.5

[buoyant pagoda]

file a bug at Mojira (/jk)

[errant summit]

I think such technical things were rejected in mojira in the past

[mental ingot]

I am fucking stuck

I hate Java generics

They are not coercive enough

Actually never mind

It is just me being shitty

[mental ingot]

@errant summit and @fair sandal Bundles in the config phase are not really possible directly, however I could add them as an optional state for modded <-> modded connections. If a configuration task were to try to send a bundle packet to a vanilla client it would error out. Is that maybe an option?

It would be a massive hack

But still possible

[iron ivy]

Bundle packets just guarentee that 2 packets are handled within the same game tick right?

[mental ingot]

Basically yes

[iron ivy]

Is that really important for config phase? There is no world.

[mental ingot]

They basically guarantee that they are processed together

Kind of like a reverse splitter

Also they guarantee order of processing etc

But that is more a concequence of the TCP connection

[iron ivy]

Thats already guarenteed by the nature of the connection

TCP being ordered, etc

[mental ingot]

Yep

[iron ivy]

packets will always arive and get processed in order

so bundles are for handling 2 packets in the same game tick

[buoyant pagoda]

what does vanilla use bundle packets for

[iron ivy]

which, i argue is pointless for config

[mental ingot]

Entity spawning

LIterally the only thing

[buoyant pagoda]

[iron ivy]

I can see some uses for it in play state, i have some ideas for my own stuff

[mental ingot]

It sends one massive bundle, with the following information:

1. entity spawn packet
2. attributes packet
3. Movement packet
4. Equipment packet
5. Passenger packet
6. IsPassenger packet
7. Leash packet

[buoyant pagoda]

I don't see why bundle packets would be useful in the configuration phase
isn't the configuration phase somewhat asynchronous? I remember seeing something about having to receive acknowledgements of tasks being completed, or something

[mental ingot]

It is

I just wanted to bring it up

Cause it can cause some confusion

That is all

[buoyant pagoda]

vanilla itself errors out (or makes it impossible to happen) when bundle packets are used anywhere outside of play phase?

[mental ingot]

Yeah

The generics prevent it

Well not directly

We simply don't offer a way to modify the protocol

So even though the modder could create their own instance for the config phase

It would generically not work

Because the listener is wrong

And there is no way to inject that custom bundle packet into the protocol

[buoyant pagoda]

well, I don't see why we should expand/modify vanilla's protocol in this case, so I say we don't poke that for now, and leave it to the future if/when someone gives a convincing case for us modifying vanilla like that to allow bundle packets in configuration phase

[iron ivy]

Still haven't had time to fully review today, hoping for tomorrow. I have some concerns, but will voice them when I review.

[mental ingot]

Okey

I agree it is something we can iterate on

If a modder comes with a valid usecase

We can reasses then

[buoyant pagoda]

(plus, it's less work to do for now, which means one step closer to getting that PR merged )

[fair sandal]

IMO bundles are kinda useless for config yeah

[mental ingot]

Okey

Annoying to fix the entity stuff

But I unwrapped the Entity#recreateFromPacket(...) by making it have an interface

[fervent token]

That is what I was asking yesterday!

[fervent token]

Well also some mods may rely on the data being present in order to render the entity properly on the client, so if it gets added without the data some mods might have issues

[mental ingot]

Okey, so I just checked:
I am going to rip the custom factory

And use the bundle mechanic mojang introduced

The process is ran synchroniously between two ticks

So there is no, "rendering" without the data

Additionally this architecture with injecting into the bundle is a lot simpler

[fair sandal]

yeah that was the motivation for using a bundle

[fervent token]

Nice

[mental ingot]

@fervent token Pushed the update

The only thing that you need to do, is implement the IEntityWithComplexSpawn interface

Add the two methods

And the rest will be handled automatically

[fervent token]

Great, will look at it when I get a chance

[mental ingot]

I can not find any evidence on github for this. Neither in the QuiltMC org nor in the FabricMC org

[brave rose]

https://github.com/QuiltMC/quilt-standard-libraries/blob/1.20/library/core/networking/src/main/java/org/quiltmc/qsl/networking/mixin/PacketBundleMixin.java

[mental ingot]

Already implemented something similar

[indigo hazel]

Btw, I was curious last night whether packets being handled on the netty threads still holds true and found a potentially confusing discrepancy: when a packet is received on the server, then it's both decoded and handled on the netty thread. When a packet is received on the client, then it's decoded on the netty thread but handled on the render thread. I haven't dug deeper into why this happens on the client though

[wary solar]

what happens when the packet is received by a LAN host

[buoyant pagoda]

any packet?

[mental ingot]

Interesting

Let me check

[wary solar]

C2S packet received by the integrated server

[indigo hazel]

I'd have to try but I assume it'd be the same behavior. Singleplayer shows the exact same discrepancy except that the write and read is absent since the "memory connection" transfers the packet instance directly and doesn't convert it to a byte buffer

[mental ingot]

It really should not

Because the channelRead0 even in the client

SHould always be executed on the netty thread

[buoyant pagoda]

i wonder how much impact adding trace level logging statements everywhere would have

[mental ingot]

There is technically

On the Decoder

[buoyant pagoda]

[mental ingot]

I can't reproduce your case:

[indigo hazel]

Put the breakpoint in the handler of your custom payload, not the packet that holds the payload

[mental ingot]

LOL

You are right

But I did not add that to the server side

Let me patch that out of the lcient side

Building a fix 😄

[indigo hazel]

Need to be careful with the vanilla custom payloads, they are expecting to be handled on the render thread

[mental ingot]

Yeah I just moved it down a line 😄

Actually hold on

Shite

Okey Question: Do we want to have it all run on the main thread?

It would be a lot easier

[indigo hazel]

That would indeed be easier, though I also like the idea of doing as much as possible on the netty thread if possible. Couldn't we move the thread "assertion" into the brand payload check, move the NetworkRegistry#onModdedPacketAtClient() call to the top of ClientPacketListener#handleCustomPayload(), give it a boolean return value and then duplicate the thread "assertion" below that:

public void handleCustomPayload(ClientboundCustomPayloadPacket p_295727_, CustomPacketPayload p_295851_) {
    if (this.isModdedConnection && NetworkRegistry.getInstance().onModdedPacketAtClient(this, p_295727_)) {
        return;
    }
    PacketUtils.ensureRunningOnSameThread(p_295727_, this, this.minecraft);
    ...
    <vanilla payload checks>
    ...
}

[mental ingot]

The problem is not the ClientConfigurationPacketListener

But the ClientPacketListener

For now I patched each listener call it self with a jump back

[indigo hazel]

Yes, that's the point of the snippet I posted, it would mean that you only need the assertion once in ClientPacketListener

[mental ingot]

Hmm

[fair sandal]

Why not IEntityExtension?

[brave rose]

It needs to be known whether the extra data is necessary. If not, then there's no point in sending an empty payload.

Of course there are other ways to check this besides implementing an interface

[fair sandal]

You can just check if anything was written to the buffer 😛

[brave rose]

This is true, but then you need to make the buffer even if it won't be used

(This is assuming that this data is sent separately using a bundle)

[mental ingot]

Pepper is correct

We only add it to the bundle when it passes the instance of check

That way we do not create not needed overhead

Cause I am willing to bet that 99% of all spawned entities out there do not use this feature or could use vanillas data synd feature

[fair sandal]

But I am willing to bet there is no meaningful overhead, and these magic interfaces are a PITA for discovery

[mental ingot]

The overhead is not unsubstantial

[fair sandal]

Disagree, allocating a near-empty buffer on entity spawn is pretty meaningless IMO 😛

[mental ingot]

There are at least two aspects to consider. The first is the compute power needed to determine if the packet needs to be sent. Creating an ffb, in an unpooled bytebuf, calling the method, then creating the byte[] and checking the size, is comparatively much more expensive CPU cycle wise, as well as memory pressure wise. Especially given the amount of entities that are spawned and despawned in Minecraft. Second is the network bandwidth needed. If you could argue that checking is to expensive and we should just send the payload all the time, then we need to consider the bandwidth needed. Which is not small. Especially in high volume scenarios like with item entities etc.

This feature is advanced, people should need to do some research and in that light a magic interface is fine

[brave rose]

There is also the option of adding the methods to the generic extension and also adding a boolean method for whether they are used

[fair sandal]

You can do a trivial cpu estimation and you'll see that it doesn't matter

At least IMO this looks like premature optimization

[delicate dawn]

entities don't spawn often enough for a single instanceof check to matter

[fair sandal]

I would just do what I suggested, and we can easily remember which classes actually override the method in the future if it really is a performance problem (which I strongly doubt)

[delicate dawn]

if we still don't want to go that route, IMO choosing based on whether anything was written to the packet is the wrong approach
you could have a boolean hasAdditionalSpawnData() that defaults to false, which is a lot less ugly, but maybe hard to discover, but realistically just as hard to discover as the interface

[fair sandal]

Why is it wrong to check if something is written?

[delicate dawn]

because you have to allocate the buffer just to discard it later if it wasn't used

that feels wrong to me at a very primal level

[fair sandal]

Well you don't need to know about it 😛

[mental ingot]

There are thousends of entities spawned in fractions of a second sometimes

A simple instance of

Is much much much much much much faster

Then to allocate the FFB, Unpooled BB, Byte[] Call the writer method, validate the empty ness etc

I would only consider removing the magic interface if and only if I added a method to the extension interface

Which would really make that a "magic" method

An no better then the interface

[delicate dawn]

It's not a magic interface, it's kind of what interfaces are for :V

But yes

IMO there's little difference between this and the alternatives and plenty of people are already using the interface

So why not keep the known feature instead of replacing it with something that's not substantially better?

[indigo hazel]

The interface is also one of the things which are actually documented

[steady sonnet]

my drive-by comment here is: knowingly allocating a FriendlyByteBuf only to throw it away sounds like a bad idea

[mental ingot]

Yep

[fair sandal]

Premature optimization 🤷

[fair sandal]

The FBB itself is a cheap wrapper object

I'd have to check how big an unused unpooled bytebuf is by default

[steady sonnet]

it still smells like bad coding regardless

and I can imagine scenarios in which entity spawning is rather hot (e.g. mob farms)

[fair sandal]

The actual entity movement and AI is way more expensive

[fair sandal]

Yet it is the cleanest on the consumer side. "Here's a buf if you want to write data, feel free to use it or not"

[steady sonnet]

In general, if I were going to do something like that I would make sure the provided object was lazy about allocating

For instance, Embeddium's chunk mesh event doesn't actually allocate the backing list of mesh renderers until the first mod tries to register one

[fair sandal]

This is really not a meaningful amount of allocation though

For me this is about basic complexity theory and order of magnitude estimation

[indigo hazel]

I honestly just don't see the point of changing it. This interface has existed for years now, it's documented and works fine. I would argue though that it should keep its old name

[fair sandal]

The interface method?

[indigo hazel]

Yes

[fair sandal]

Well the idea is to not need to maintain a custom spawn packet, rather only the data adding part

[mental ingot]

Not an option!

We need to maintain vanilla compatibility!

[indigo hazel]

Hmm? I don't see what the spawn data interface and needing a custom packet have to do with each other.

[fair sandal]

I'm aware but I don't see your point?

The current interface is pretty much overriding the getCustomSpawnPacket method? Or does that require an additional interface on the entity?

[fervent token]

you already had to be implementing IEntityAdditionalSpawnData

[fair sandal]

I guess that's the bound on the NetworkHooks method?

[fervent token]

yeah otherwise the networkhooks one was basically a carbon copy of vanilla's packet if that isn't implemented

or if you don't write anything to it

[fair sandal]

Well in that case...

¯_(ツ)_/¯

[indigo hazel]

It's worth noting that in the past the networkhooks one was required for modded entities due to how the vanilla one worked

[fervent token]

that too

[fervent token]

well it compiles... and from an extremely quick test it seems like it works

[mental ingot]

Well that is a start

[pliant stream]

If we really are concerned about allocating the buffer, we could have a long-lived one that sits empty and replace it whenever something uses it

[errant summit]

Question: Are all the todos for this PR or can some of them be done separate?

[mental ingot]

The checkmarks on the PR need to be completed before merging

I left several //Neo TODO: in the code

To indicate stuff that we can work on

That does not need to be in this PR

[errant summit]

so the todos in the PR desc need to be done 👍

[mental ingot]

Yeah I also need to add more documentation to the PR description as well

Primarily @errant summit It would be nice if a second person ran the tests as well

And maybe we can think of a way to write tests for stuff like this?

[fair sandal]

You should also think of writing a blog post 😄

[mental ingot]

Once I am done, the PR will most likely be rewritten a bit and that is the blog post

That is primarily why it is that long 😄

[fair sandal]

well we can copy/paste the PR description as a good starting point I suppose

[mental ingot]

Yeah

I will need to add some more examples probably

And some guidelines of things people need to pay attention to

But the PR description is pretty good

[fair sandal]

for the blog post I mostly had:

• how to replace simple channel
• how to pass menu dadta
• configuration networking tasks
  in mind

[mental ingot]

That second point can just be scrapped

[fair sandal]

probably also the version stuff

I mean, I want to know how to update my NetworkHooks.openScreen(player, this, this::writeScreenOpeningData) 😛

[mental ingot]

player.openMenu(...)

Again I personally think both the entity system as well as the screen system

Are really stupid

Especially since the payloads are now supposed to be records

And just write to an FBB

[buoyant pagoda]

I for one welcome our new configuration phase task overlords

[mental ingot]

Like I can see the entity data stuff, a little bit. Allthough I really can't imagine what you need an entire FBB for during entity spawn

I mean in reality

I think it would even make sense to rip the entity spawn stuff

And just add a callback to the Entity

That gets called to populate the bundle

And a modder should just create a nice proper payload

With proper fields

And handle it properly

[fair sandal]

¯_(ツ)_/¯

[mental ingot]

Actually

You know what

[fair sandal]

we could do that as well

if you want to do it you should do it in this PR 😛

[mental ingot]

I like that system way way way way way more then what we have right now

Because it means that the modder can send dedicated payloads

[fair sandal]

but keep in mind that I need the packetbytebuf to arrive at menu creation time on the client

not later

[mental ingot]

The menu stuff will stay

I have no solution for that right now

Since mojang does not use a bundle for that

But a single packet only

[iron ivy]

Personally I HATE creating structs just to serialize them to the network. I much prefer to write to the buffer directly

[mental ingot]

I know covers, we talked about this before

But in practice that is what Mojang wants us to do. That is how their system is designed

And there is nothing standing in your way to use a payload that just takes a byte[]/FBB

And writes that to the network on write

[iron ivy]

Since when did we start doing what mojang wants us to do?

[mental ingot]

Always....?

[fair sandal]

I personally think that writing to a FBB is fine for menus and entities

[mental ingot]

I think it is okey

It is not bad

But giving the player the option to write more then one payload

In those scenarios is even better

I mean in practice we could even go the middle route

[fair sandal]

well you can trivially do that by writing more to the FBB?

[mental ingot]

You could

And I might have an idea that make both of you happy

What do you guys think of this:

[fair sandal]

I mean, this sounds like a case of trying to change something that doesn't need to be changed

[mental ingot]

Instead of patching an instance of check into the serverentity sync call

I make a method on the entity extension

Which by default does that instance of check

And then potentially writes the forge packet

So in practice it behaves like I implemented yesterday

But if you really want to take fine grained control

You can

You would not implement the magic interface

But override the bundle building method

And add your own custom payloads to the bundle to process

That way, people that want the FBB can simply do what they do today

Aka implement the interface and be done with it

But people that want to mimick what mojang does

With all their specific packets and bundles

Get the chance to do it

And everybody is happy?

Okey implemented

That makes both possible

[iron ivy]

So, if im reading the PR correctly, to send packets we now just register channels, then implement CustomPacketPayload and yeet it out a connection.

[mental ingot]

Yep

[iron ivy]

fantastic

[mental ingot]

You can use PacketDistributor to help with it

But that is basically the gist of yeah

[iron ivy]

Yee

I like that the channelActivate callback crap was removed

that will cleanup MineTogether's 'share to friends' stuff

Does it just now use channelActive/inactive directly?

[mental ingot]

Nope

There is no such thing anymore

[iron ivy]

Ehh, probably fine

As long as theres no outside crap to a Connection

[mental ingot]

I recommend using the listener

[iron ivy]

as we make them and throw them on a different netty stack at some point

[mental ingot]

Because those have endpoints where you do not need to wrap them in the packets

I added overloads to the Listeners to accept straight Payloads

[iron ivy]

I'm talking about something different

[mental ingot]

And then wrap them in the custom packets

Okey, I am not sure what you are talking about then?

Can you show me/clarify your situation a bit. So that I can get a bit more context

[iron ivy]

this is not sending packets. We implement an alternate netty stack, people can share their single player -> Proxy -> friend

'Share to Friends' opposed to 'Share to Lan'

[mental ingot]

Hmm

I mean in practice, as long as you allow the vanilla listeners as sending/receiving endpoints for the eventual processing of the packets, it should just work

Nobody should be touching the raw netty connection object anymore

[iron ivy]

Anyway, as long as that activate callback crap is gone (previously it injected forge's shit to the channel handlers), etc

fantastic

[mental ingot]

We set some attributes on the channel and the channel handler context to keep track of what custom payloads exists

See the NetworkRegistry

[iron ivy]

Or it captured the Connection or something.

[mental ingot]

But that is about it

[iron ivy]

Yeah, from what you've said, it should be fine

[mental ingot]

I mean as long as your new connection runs through the same handshake process as vanilla, with the configuration phase, it really does not care what netty stack it is running against

[iron ivy]

Yeah, it just adds the vanilla netty pipeline pieces and away it goes

[mental ingot]

Yeah then it should be fine

The NetReg will add the attributes, filters etc the moment it sees a pipeline come in

There have been some reports that it is not clearing the state on a crash

But I have not been able to reproduce that

@errant summit That is another thing we need to test

XFacts report that aborting a configuration task with a disconnect prevents clients from reconnecting

Because they are supposedly stuck with the old connection object

Which makes no sense to me

But we might need to run a clean up job on disconnect

Allthough I could only find codepaths which create new connection objects and connection futures when a connection is build

I might have missed a code-path

[iron ivy]

Well, overall quite happy with the PR. There is a LOT of formatting nits for patches. Iirc i skimmed past some imports, but not sure if you want those comments yet, or I could just do it myself and send you a git diff

[mental ingot]

Yeah there are new imports now

Because I am still refactoring the entity shit

I did a complete cleaning before I started that back up

But thanks to spotless and ideas option to create fully qualified statements instead of imports, those are cleaned up in minutes

[iron ivy]

Kk, ping me if you want me to fine tooth the patches and send you a diff, otherwise i'm confident this PR is solid.

[mental ingot]

Nice

A lot of work and design stuff coming together

Good to hear that some of the people I look up to, think I did no shitty job on this 😄

[buoyant pagoda]

when is it grillin' time

[mental ingot]

Added a game test for the custom spawning payload logic

Lets see if this works

How do I test these?

@velvet whale ?

[velvet whale]

is that test meant to be automated, or

[mental ingot]

Yeah it is supposed to be a fully automatic test

I have it enabeld

But how do I run it?

Ignore the desc

I just need to run it

And figure out if this works

Figured it out

And crashed 😄

[indigo hazel]

@mental ingot Here's a minimal reproduction for the config phase issue: https://gist.github.com/XFactHD/f982a13b1323a65112126f3be1b5d5e5. If the task from this example runs first (which is the case in the FramedBlocks environment I just wrote this in due to alphabetical mod sorting), then it complains about the example payload being un-negotiated. If Neo's reg sync task runs first (which was the case in the environment I originally noticed this in) then it complains about the reg sync packets instead. To reproduce it, connect to a server, let the example payload sent by the config task kick you and then try connecting again and you should get stuck at Joining world. It may take more than one try to get it stuck at Joining world though.

[mental ingot]

Does this requires a dedicated server

Or is a single player world fine?

Like to test

[indigo hazel]

I only tested with a dedicated server, don't know whether it also happens in SP

[mental ingot]

Okey

I will check it out later 😄

Currently working on tests for the entity spawn stuff

[indigo hazel]

👌

[mental ingot]

Do we have a massive codec user arround?

[buoyant pagoda]

i only use codecs recreationally, sorry /jk

[mental ingot]

LOL

My point is: Maybe not in this PR, but maybe in the future: Do we want to invesitgate a CODEC wrapper layer for the networking

And how the API for that would work

[timid fern]

aren't codecs too slow for networking?

[mental ingot]

Yes and no

If you do high throughput, probably

since they go through JSON and the what not to achieve this

But for payloads that get synced, say from a datapack, were you already read with a codec

It might make sense to also send directly with that codec

[timid fern]

ah, the classic "it's slow, but not done often so it's fine"

[mental ingot]

It is a convenience thing

In the same way that we have a convenience data provider for working data codecs

[iron ivy]

Might be nice for some things, but I'd always want the raw payload route to be provided

[mental ingot]

It is not ment to be used in high through put situations

[mental ingot]

I would never remove the raw payload route for sure

But you know as a wrapper

Some mechanic to do that internally

[iron ivy]

Sure

[buoyant pagoda]

"advanced access"

[iron ivy]

But

[buoyant pagoda]

i.e. you better know what you're doing when you use this

[iron ivy]

There is the potential that some modders will just full send it, without acklowledging the performance concerns

[timid fern]

yeah, I mean, I am using codecs for networking for my datapack stuff, exactly because it only runs a couple times and it's fine if that takes a bit longer

[iron ivy]

or shrug them off and nuke their network perf

Might be fine to have clear documentation on the wiki/javadoc, etc for it

[mental ingot]

Yeah

I mean we have several other systems like this

[iron ivy]

I think the key question atm is: would it be useful for Neo's own networking?

[mental ingot]

Neo's own no, but that same thing can be said for the entity spawning and menu stuff

Yet we still provide that none the less

[iron ivy]

Hardly a fair comparison, but sure

[buoyant pagoda]

do we think the benefits outweigh the costs?

[timid fern]

as in dispatch codecs?

[mental ingot]

Well, more or less

We need to ty a codec to a handler

Like what would the receiving end do

[timid fern]

Yeah ok, so instead of the usual class, encode, decode, handle just a codec + handle

[mental ingot]

You would still need to build an id for the codec and handle comby

But in practice yeah

because normally the payload has an internal id() method

Which returns the id that gets written as payload

[timid fern]

that doesn't sound too hard to do then

[mental ingot]

Cause in reality I would love to have something like send(Codec<T> codec, T instance) 😄

But without an id that is pretty difficult 😄

[mental ingot]

@velvet whale Just a heads up, the current testing framework is not really useable for entities

Because the player chunk sender is not cleared

And as such still is awaiting a bunch of payloads that need to be send

[hasty yew]

Shouldn't really need that much of a wrapper all things considered - just write to NBT and sync that, or write to JSON and sync that, depending on the nature of the data

Wouldn't be too hard to add

[errant summit]

if you register the codec you could do a key lookup for that

[mental ingot]

Obviously that is logical..... The point is that it still requires an id() for the type of codec, and to identify the handler on the client side. This requires a good thought out API and infrastructure.

Simply consider Codecs which are not static. In general those can not be used for example as keys.

[hasty yew]

Ideally you'd just have a wrapper that makes whatever (de)serializer the current API expects out of a codec

[fair sandal]

yeah pretty much

[hasty yew]

Because presumably the current API has users provide a bytebuf (de)serializer somehow, so you'd just give a helper method that turns a codec into one of those

Which is pretty simple all things considered

[errant summit]

but we should provide something better than writeJsonWithCodec because that blows up past the string length limit fast

[mental ingot]

It is the best you can do 😄

Well technically you can do CompoundTag

[hasty yew]

Well yeah, that or NBT are your only options

[iron ivy]

Should probably use nbt for packets not json, no limit and still as compressable

[hasty yew]

And NBT doesn't work for all cases because of non-homogeneous lists

So ideally you'd provide methods for both

Or a utility to better serialize JSON in byte buffers in a non-string way

But yeah, 9 times out of 10 you'd want NBT instead

[errant summit]

the one where you can use nbt is deprecated tho (see writeWithCodec)

[hasty yew]

Non-homogeneous lists are pretty rare all things considered

[hasty yew]

Hmm, that's a vanilla utility?

[timid fern]

yes

[hasty yew]

I'd say just let the folks wrapping it decide how they want to use stuff

[timid fern]

mojang deprecated nbt for json

[hasty yew]

So they provide a codec, and a consumer of a codec and a byte buffer

They can use JSON if they want, or NBT if they want, or some fancy JSON-but-not-as-a-string if they want

[iron ivy]

inb4 they completely drop nbt for json everywhere

[mental ingot]

I think the codec over the network stuff is solved

We still need a way to get an id() for the codec

Like how do you identify on the receiving end, which codec to use to read, and which handler to invoke

[iron ivy]

send(ResourceLocation, Codec, T)?

[mental ingot]

It is a trivial option which I was considering yes

[hasty yew]

Why? You wouldn't use an actual codec when sending! You'd wrap the codec in a byte buffer (de)serializer with an ID or whatever the current system expects, then send/receive it just like anything else

[mental ingot]

I triggered the discussion to see if some expert out there knew a better solution

[iron ivy]

can internally just do send(new CodecCustomPayload(id, codec, t));

[mental ingot]

The network payload needs it!

[hasty yew]

But yeah otherwise do what covers said

[hasty yew]

Yeah that's what that wrapping would do

[mental ingot]

No

[buoyant pagoda]

question: is this functionality being discussed something that needs to be in the rework? or can it be deferred to another PR?

[mental ingot]

Because I am actually talking about the protocol

YOu still need a protocol id

[mental ingot]

It can be deferred

[hasty yew]

How do you provide an ID if you're not using a codec?

[mental ingot]

During registration, and on the payload object. Please see the PR that is being discussed in this thread

[hasty yew]

Yes, that question was rhetorical, sorry. You do the same thing with a codec - that's the "wrapper" I'm talking about. A codec gets wrapped into a registerable thing, that provides/accepts a generic payload that wraps the codec's target. Or you do what covers suggested

[mental ingot]

Hmm that might be reasonable

Something like a NetworkCodec object

Which combines a network id, and a codec of some instance

[hasty yew]

Basically yeah, that's what I'm suggesting

[mental ingot]

And then to send you would invoke send(NetworkCodec codec, T instance)

I will keep it in mind

And add it to the PR description for future research.

[steady sonnet]

glad that NetworkConstants is gone, that should hopefully fix the random deadlock with some mods

[errant summit]

I think if we do something like this we should do it in a separate PR

[mental ingot]

That brings me to one more thing I wanted to discuss, a feature that can also be bolted on in future PRs but something I none the less want to put up for debate.

Most larger mods that I know use packet splitters which they build themselves to handle large data sets that are being swung around.

Would introducing a packet splitting framework native to neoforge be an idea we want to research?

[mental ingot]

100%, as I said, should be bolted on to the top of this PR, as a future work item

[steady sonnet]

hold on a sec, didn't we already have a packet splitter for stuff like the registry packets?

[indigo hazel]

We do, yes

[mental ingot]

We have one for vanilla packets only

It has a whitelist for specific packets

[steady sonnet]

oh

[mental ingot]

To enable it for modded packets is trivial: Add the custom payload packets to the allowed list

And go home

[steady sonnet]

so on the one hand it seems useful to not have to reinvent the wheel in our own mods, on the other hand it makes it easier for people to send massive packets without thinking

[mental ingot]

Yes

It is like a trade of

[iron ivy]

I think it should absolutely be enabled for modded packets.

That'd be super useful

[mental ingot]

For me it falls in the same category as the entity, menu and potential codec improvements

Where it is not something neo needs in and of its self

But would be really beneficial to a whole host of mods

[iron ivy]

It will be needed in neo if registries get super huge

[mental ingot]

And as such it should really be considered in Neo

[mental ingot]

True true

[steady sonnet]

I'm not against adding it

particularly if the functionality is already mostly written

[mental ingot]

Allthough that would be a massive registry, like 5000+ entries..... since each registry is syned on its own

Given that the server can send much much much larger packets these days to the client. Just not receive them

[iron ivy]

You know Kitchen sink packs exist right?

:D

[mental ingot]

I know covers

But still

Even with 200 mods.................... Well you are potentially right XD

[iron ivy]

iirc it has happened in the past

although, dont know if registies were uber, or split packets back then

[steady sonnet]

a 5000 entry registry is essentially guaranteed post-flattening

[mental ingot]

I think including the packet splitting in the main PR might make a lot of sense

It is a minor change

[indigo hazel]

I like the idea of providing the option of leveraging the splitter to mods but I think it should be opt-in. There's no reason to blast the 99% of packets that are never oversized through that thing

[mental ingot]

And an immediate improvement to modders all around

[steady sonnet]

does it apply if the packet is small enough?

[iron ivy]

Just make sure you don't try and split Neo's packet splitter packets :D

[buoyant pagoda]

opt-in with a debug logging statement for any packet class that is opted-in

[mental ingot]

Let me check how it currently works

[buoyant pagoda]

iirc, no

[iron ivy]

Yo dawg, we heard you liked packet splitting

[buoyant pagoda]

iirc the packet splitter is a a Netty channel-level handler

[mental ingot]

There is basically no overhead

[buoyant pagoda]

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

[mental ingot]

And the way it is currently coded

[indigo hazel]

It does some work since it needs to determine whether the packet is indeed small enough. It does that by copying the byte buffer and if it's small enough it pushes the packet through directly and discards the copied buffer again

[mental ingot]

Does not really lend it self for modder specific filtering

As it is a processing step on the raw Packet

Not on the payload

It would require custom packet processing

But you are right

It allocates an additional write cycle

To do the check

[iron ivy]

packet size check and bail is free imo

[indigo hazel]

Not really. If the packet you get is a custom payload packet, then you can look up whether it needs splitting by the payload's ID

[steady sonnet]

Hmm the copy is not great but given that network stacks do a ton of copying anyway, don't know that we care

[mental ingot]

It would still require that processing, I am not saying it is a lot

But it is still something that would be needed

[indigo hazel]

Fair enough

[iron ivy]

Opt-in modder packets is overkill, just enable it transparently, if the packet is not big enough its never split, so whats the issue?

[steady sonnet]

does that mean it will be able to split any vanilla packet too?

[iron ivy]

The current one does, yes

iirc

[steady sonnet]

That seems problematic for vanilla client -> Neo server compat

[mental ingot]

It is disabled for vanilla compat

[iron ivy]

that

[steady sonnet]

perfect

[indigo hazel]

Though, if we expand the splitter to custom payloads, then it should probably handle C2S as well and therefore needs to handle the smaller payload limit of C2S custom payloads

[mental ingot]

@indigo hazel If we want to do this transparently, without the write overhead we can do that too. Instead of making it a filter, we would patch the relevant encode(...) method in the PacketEncoder, and run it through there

That would make the write call exactly once

And then continue processing it from there

The overhead here would be an additional byte-copy at worst

but it would be trivial to do

[indigo hazel]

Hmm, that could work. I'll have to look into that when I'm home tonight or tomorrow

[mental ingot]

Basically patch that location

We have the channel handler context

And as such can ask the NetReg if it is talking to a vanilla channel

And can as such disable the extra processing when on a vanilla connection even

[indigo hazel]

Yeah

[mental ingot]

Once I have this test framework up and running

I will see what I can do on this topic

I think doing this for all packets regardless of contents, if on a supported connection, is very much worth it

[buoyant pagoda]

I don't think that also needs to be under the networking rework PR

[mental ingot]

People need to rewrite a significant amount already. Them not having to deal with splitting if we can offer this trivially, is at least something worth researching and not just delaying

[buoyant pagoda]

I want pictures of Spider-Man reworks of networking!

[mental ingot]

Because it means that they need to port less

Like I agree that the codec stuff

And other additional concepts which we have discussed in here, should not be in it

But this is something which is a small change, since we have the code

And a change that would make the lives of a lot of modders a lot simpler

But I understand your concern

[buoyant pagoda]

I thought the premise of this was that it would be transparent to the modder
and that since the status quo after rework is still the same pre-rework with regards to max packet size and splitting, we can simply push this feature to its own PR, where it gets its own amount of scrunity and round of "this is a new feature you should know about!"

[mental ingot]

feature creep and the what no

[buoyant pagoda]

yep, feature creep

just to keep the PR on track on its core premise -- reworking the networking API -- and not delay it more by tacking on new features

btw, have you tested connections between pre-rework and post-rework server<->client?

[mental ingot]

No, because they are by design not compatible

[buoyant pagoda]

in that, those connections should properly terminate with a good disconnection message, and not have some garbled response spit back at the user

test it fails gracefully

[mental ingot]

Not something I can control sadly, because the older clients expect certain answer

It likely will see it as a vanilla connection

Cause all of the FML metadata is missing

[buoyant pagoda]

can we insert that FML network version attribute on the channel with an incremented channel version?

[mental ingot]

No

Because it is not the attribute that is the problem

[buoyant pagoda]

so older clients recognize "hey, we can't connect here"

[mental ingot]

The packets and payloads are just not the same

[buoyant pagoda]

I'd like it seen too, as to what it actually does

[indigo hazel]

Speaking of FML metadata: do we still need the client intention packet patch with the stupid host name concatenation?

[mental ingot]

I literally have no idea

I looked at that

And as far as I can tell no

But I fucked that up twice too many

[buoyant pagoda]

yeah, but that doesn't matter if the connection is terminated way before that by virtue of the detection that the FML network versions are not the same

[mental ingot]

That is what I have right now I think

I still have the FML Net version on the intent packet

And that is set to what ever Forge version spec we are running with

[indigo hazel]

Gonna play with that a bit tomorrow then

[mental ingot]

If that is sufficient to prevent FML from connecting

Then I think that it is nice that it should stay

[mental ingot]

Okey updated the game test helper to now also configure its networking properly

[mental ingot]

Fucking finally :

@velvet whale I think we make your test framework a bit more cringe 😄

I had to do a decent amount of finagling

But it at least now tests the new entity spawn code completly 😄

[velvet whale]

gametests are cringe already, you can't do worse

[mental ingot]

Well .D

I had to make the connection an actual in memory connection

And then actually start sending all the chunks 😄

[steady sonnet]

That's pretty cool to be able to do in a test though

Mods can also use the test framework, right?

[mental ingot]

Sure

It looks a bit funny with the test players and such :d

But yeah you can

@velvet whale I added a lookup for payloads as well

And unpackets bundled packets

So that things like the entity spawn logic are now testable 😄

Which is nice 😄

[steady sonnet]

i for one welcome our test-driven development overlords

[mental ingot]

I am trying to figure out what else we can test 😄

The problem is that there is only a server player

So testing the protocol is pretty hard

[steady sonnet]

in theory we can run a client on CI, modmuss did it before

[mental ingot]

It is not that

It is that the gametest framework does not support "creating" a client that connects to the server

[steady sonnet]

I see

[mental ingot]

Like what you really would want is two things

One is a reactive connection

Right now the connection that the server player has is like a sink

It captures all packets

And that is it

You can't really send anything back

There is no way to react to it

That is partially because for the client listeners you need a bootstrapped minecraft instance

Which as previously discussed is immensly hard to come by

Especailly in a unit test scenario

[velvet whale]

https://github.com/neoforged/NeoForge/commit/38cf07f5860a00f41c8fc58613898139297362dd#diff-ce06ac58ce8fa1b936e891f029fb1d68fc42e0ec8c8b550e20bbc20103a0e966 @mental ingot you did it again :P

the whitespace

[mental ingot]

We talked about this in the last few days

I won't be fixing that there

It is because IDEA is stupid

And because VF is stupid

VF adds these whitespace on empty lines on inner classes

[velvet whale]

that's not a patch

that's our code

[mental ingot]

Which IDEA then detects as the indent logic for the project

So it does this for every class

Even though I have the checkbox set to remove the whitespace

And remove them manually

On save it will still add them

[velvet whale]

do you have this ticked?

[mental ingot]

No

And I have the remove whitespace on empty lines checked

Which should remove it

Even if I add it manually

No clue why it it is simply not doing what the fuck it is supposed to do

[velvet whale]

are you sure? from what i remember you only changed the remove trailing spaces option, this one is a code style option

[mental ingot]

I checked twice now

It is not set

Because it is stupid that it does this

But I have the "detect formatting from project" box checked

And I am not removing that

Because I work on too many different projects

I am removing the whitespaces from patches though

I spend an hour this morning trying to figure it out

[velvet whale]

this one?

[mental ingot]

Yep

[velvet whale]

you can copy the config to the project and disable it just for this project

[mental ingot]

That would mean writing that file into git

I have like 6 copies of Forge

And not just on this machine

Well actually

Would that make sense?

To introduce a formatting file for idea?

That said

I need to run spotless anyway

Which will rip those whitespace changes back out

[velvet whale]

we already have the eclipse fromatting config in the repo, you just have to import it

codeformat/formatter-config.xml

[mental ingot]

Okey added it now

Thanks for the headsup

[velvet whale]

https://github.com/neoforged/NeoForge/commit/38cf07f5860a00f41c8fc58613898139297362dd#diff-b2cbd87425a89f49cd38c2bda363ee32fa46ae27ae42e67b15245efe18911f62 ah yes, the linux debugging problem

[mental ingot]

I know, I know

But this started to really fucking annoy me

I had to kil idea 4 times in an hour

Because of this shit

[velvet whale]

i believe mcjty shared a bash script to release the mouse?

i wonder if we can somehow detect IJ's agent pausing the debugger

[mental ingot]

Maybe

That should be in another PR

This is basically the minimal I need to properly do testing and debugging

[hollow canopy]

corosus

[Reference to](#modder-support-1․12 message) #modder-support-1․12 [➤ ](#modder-support-1․12 message)```required once:
sudo apt install xdotool

required each mouse grab fix:
setxkbmap -option grab:break_actions
xdotool key XF86Ungrab```

matyrobbrt referenced · Jan 28, 2019, 5:11 AM

[mental ingot]

That only works on X

Which I don't run

I run wayland

[velvet whale]

how dare you not use musk's platform

you should be ashamed

also did you somehow forget to genpatches

[mental ingot]

No....

Let me check

Nope

I have no pending changes

I just ran the unpack task again

And no new patches

[mental ingot]

Why are you thinking that?

[velvet whale]

i was confused where that ismemoryconnection was coming from

[mental ingot]

Ah it is just an override on the connection

Vanilla has it

But only checks for its own special local connections

[velvet whale]

(no we can't really, nevermind)

[marsh loom]

It's possible to hook into JPDA to see if the debugger is pausing a thread. But you can't see if the current thread is paused, because the current thread is paused and thus can't run code to check anything.

[velvet whale]

you mean jdwp?

[mental ingot]

No

JPDA Java Platform Debugger Agent

https://docs.oracle.com/javase/8/docs/technotes/guides/jpda/

[marsh loom]

It's possible to hook into JPDA to see if the debugger is pausing a thread. But you can't see if the current thread is paused, because the current thread is paused and thus can't run code to check anything.

[mental ingot]

So I tried this

And I can't reproduce it

[fervent token]

given the AdvancedAddEntityPayload gets created when adding to the bundle (and as part of that constructor the additional data is converted to a byte array), do we want to just skip making the built AdvancedAddEntityPayload get passed to the consumer if the payload is empty?

[indigo hazel]

Color me surprised but I can't reproduce it anymore either after pulling and building the latest commit as of this message, so this must have been fixed somewhere in the last 11 commits, zero clue which one though, none of them seem like they would fix something like this.

[indigo hazel]

I did find another issue though: if the client is disconnected in the config phase, then the frozen registry snapshot is not applied. If the server you tried connecting is missing registry entries which the client has, then this leads to an exception when you join a singleplayer world because the SP server then sends a snapshot of a broken registry state to the SP client where the entries that were missing on the server have an ID of -1. The server with the missing entries would still let you connect though (which kind of makes sense). If the mismatch is the other way round (server has entries which the client doesn't have) then this issue doesn't occur and you can also connect (which should not be possible IMO due to the issues that arise when for example the server sends a block which the client doesn't know) because the reg sync handler ignores the missing entries returned by the snapshot application.
The reason why the snapshot is not applied is that the method used for it only works if a level and a player exists, the former due to the null check it's behind and the latter because the connection object it uses comes from the player: https://github.com/neoforged/NeoForge/blob/1.20.x/patches/net/minecraft/client/Minecraft.java.patch#L416.

As far as I can tell we have two ways to fix this: either move the application of the frozen snapshot into ClientCommonPacketListenerImpl#onDisconnect() after the Minecraft#disconnect() call and don't do it in Minecraft at all or do what vanilla does for server resourcepacks by keeping the case for disconnect during play phase in Minecraft#disconnect() and putting the case for disconnect during config phase in ClientConfigurationPacketListenerImpl#onDisconnect() after the super call (the latter option worked in a quick test with a non-suspending breakpoint that executes if (!connection.isMemoryConnection()) RegistryManager.revertToFrozen();).

It's also noteworthy that the two added ID getters in MappedRegistry are not overridden in DefaultedMappedRegistry which means their behavior deviates from the vanilla getId(T) method. Ideally they should forward to the vanilla method instead of accessing the map directly: https://github.com/neoforged/NeoForge/blob/1.20.x/patches/net/minecraft/core/MappedRegistry.java.patch#L163-L171. This deviation is why the bug is easily visible because the snapshot constructor uses getId(ResourceLocation) to build the ID map. Without the deviation, the corrupted entries would return the default entry's ID if the registry is a defaulted one (which could cause other issues), so you'd need the mismatch in a non-defaulted one to even notice this.

[steady sonnet]

The first option sounds cleaner

[indigo hazel]

Would be my preferred option as well, yeah

[steady sonnet]

these writeups are quite good

[indigo hazel]

Took me the better part of 2 hours to collect all that crap
(Fuck exception-driven codeflow, especially with very generic exceptions, it's really annoying when you need to use an exception breakpoint to even find the point of origin of an exception)

[indigo hazel]

Before I forget, there are two other things I noticed during testing:

1. Due to NetworkConstants being gone, some Netty stuff is getting loaded significantly later which also means the Netty platform testing debug stacktraces now happen in the middle of the initial resource load logging. I think we should finally shut that crap up for good
2. Very rarely the registry snapshot application from the reg sync packet throws a "registry already frozen" error. I have not been able to dig into the specifics yet as it happens so rarely, I'm gonna look into it more after some sleep. Currently my best but completely uneducated guess is that it's caused by the snapshot application now happening on the netty client thread. It may make sense to defer that to the main thread, including the map clear calls and the response packet transmission

[hasty yew]

I could get 5000+ entries in a registry pretty easily with Excavated Variants and a few big content mods

[dim wagon]

or just aoa on its own

[hasty yew]

Christ, I don't want to know how. At least I have the excuse of it being a quadratic problem

[marsh loom]

Clearly tslat is a quadratic problem :p

Convenient, I'll grant you

[mental ingot]

@indigo hazel Will address ASAP

Also:

I like the new API to do work synchronization 😄

Build in exception handling, and post processing 😄

Amazing 😄

[buoyant pagoda]

witchcraft!

CompletableFuture witchcraft!

[mental ingot]

Yes

But this is really really really cool 😄

[buoyant pagoda]

I designate you the CompletableFuture receiver
now off you go to debug world and chunk loading! /jk

[mental ingot]

Even though this sounds like padding my own back: But the more I am working with the API of the networking, the more I am liking it.

[buoyant pagoda]

i'd be concerned if you didn't like the API you yourself wrote

[mental ingot]

It has happened 😄

[buoyant pagoda]

[mental ingot]

I mean most API designs happen on paper

[gaunt lark]

Chunkloading? We don't talk about that spaghetti here

[mental ingot]

And in reality I only know for real once I and others start using it

[velvet whale]

I hope that it doesn't swallow exceptions

[mental ingot]

If you don't handle them....

[buoyant pagoda]

[mental ingot]

Well it is a completable future

I am not sure what the default throwing behaviour is in MC

It is a bit all over the place

Let me check

[velvet whale]

that was my point, most CFs in mc codepaths (fml included) swallow exceptions...

networking exception swallowing has been a recurring problem

[mental ingot]

Okey

So just checked:

Any exception thrown will get logged

With respect to completeable futures

So if you do any synchronization work, you need to add something like I have shown above

To capture the event

[velvet whale]

yes that's my point, relying on the modder to catch exceptions in packets that touch the world or whatever is a problem

[mental ingot]

Nothing we can do about that

Any synchronization call in MCs event loop uses ComFut

So you will always run into this

It is just explicitly exposed

That said I should add some documentation on this

[buoyant pagoda]

iirc, there's no way to give a "default" exception handler for CFs

[velvet whale]

yes we can. whenComplete(($, ex) -> if (ex!=null) LOGGER.log())