#Networking Protocol

1 messages Ā· Page 5 of 1

buoyant pagoda
mental ingot
#

But where ???

#

Correct

#

We can't add that into the event loop it self, because some call locations, including the one shown above, require the exception to bubble up and not be handled untill they do

velvet whale
#

I vaguely remember there was a patch in the eventloop handling swallowed exceptions, you should first try throwing an exception and see what happens. if it's swallowed we should wrap the submits

mental ingot
#

And no we are not wrapping the submits

#

Well it is not swallowed

#

It is captured in the CF
But if you do not attach an appropriate handler it won't work

#

@velvet whale How about this: We add a third method to the work handler:

default void execute(Runnable runnable) {
   submitAsync(runnable).exceptionally(ex -> LOGGER.error("Failed to process payload", ex));
}
#

That way the original API of the Executor is preserved

#

And people that normally know how to properly work with CF's can do that

#

But poeple that just want to run a task, get this handholding method

#

Mojang does something similar in their event loop

#

This extra method in the event loop has an exception handler build in

#

Do not get me wrong

#

I understand your concern

#

But blindly capturing all exception is a bad idea API wise

#

This should just be properly documented

#

And CFs and their exception processing behaviour is a known factor and component

#

As you yourself pointed out

gaunt lark
mental ingot
#

If you want to chain of it, then IMHO you should know how it handles exceptions

#

Especially ones down the line

#

Because if you want to chain, then an exception can again happen in your next handler

#

Which is then again swallowed

#

If you want to chain, then IMHO the modder should use submit and be aware of the fact that he or she needs to add an exceptionally statement, or wrap his runnable in a try-catch themselves

mental ingot
#

@velvet whale Would that be acceptable?

errant summit
#

So... these are the open todos:

  • Implement registry sync failure disconnecting and messaging
  • Discuss futher contextual components to the ServerConfigurationPacketListener, as this is the only piece of information given to the OnGameConfigurationEvent. We already exposed on that listener type if the connection is vanilla or not. Maybe the players UUID should be exposed as well? Please discuss this in the comments.
  • Test further scenarios where mismatches can occur
  • Is modlist mismatch testing even needed? Should we not more think about this in the form of content, aka network payload types, registries and their contents, tier sorting etc?
mental ingot
#

I think I got the first point now

#

I addressed @indigo hazel comments about registry revertal

#

And it should be fine now

#

It is nothing fancy

#

But it will just gracefully terminate and disconnected with a simple message

indigo hazel
#

I found the reason why the registry sync can fail and it's indeed a race condition: https://gist.github.com/XFactHD/b85910268a091d9e0de87188ce251920.
Vanilla sends the brand packet, ClientboundUpdateEnabledFeaturesPacket, ClientboundRegistryDataPacket (the culprit) and the ClientboundUpdateTagsPacket outside of tasks (why Mojang, just why???). This means that modded tasks, including Neo's reg sync, start immediately instead of waiting for these operations to be completed. Because of that and the fact that all the mentioned packets are handled on the client thread while the FrozenRegistrySyncCompletedPayload is not, the snapshot application can happen while the ClientboundRegistryDataPacket builds the RegistryAccess to apply the transmitted data onto on the client thread, which calls freeze() on all registries. We'll have to move snapshot application and the response packet to a task submitted to the main thread.
This issue is a ridiculous pain in the ass to reproduce, especially once you add stack walkers and loggers for debugging. I'm lucky I got it to log that crap at least once, all subsequent attempts failed

#

Also, @mental ingot can you push your latest changes by any chance? I'd like to break more stuff after (late) dinner šŸ˜„

mental ingot
#

I would push

#

But

indigo hazel
#

Heh, fair enough šŸ˜„

mental ingot
#

So I pushed

indigo hazel
mental ingot
#

Okey

mental ingot
indigo hazel
#

šŸ‘Œ

indigo hazel
#

I just did a quick test how the old and new network implementation interact. Here's the results (gist because Discord sucks and can't render MD tables): https://gist.github.com/XFactHD/9bc0fcab59b4cba2f8013ac41affed66
TL;DR: it's fucked and whether the ClientIntentionPacket is patched or not doesn't matter
(Tested with commit 0f066b3 of the network rework against NF 20.4.40)

mental ingot
#

Okey well that is fine by me

#

Nothing we can do

#

@velvet whale @fair sandal @indigo hazel Can you please review the PR again

fair sandal
#

Will probably take me a few days at least to do a full review

mental ingot
#

And @indigo hazel You still had an idea for the ClientPacketListener right, where I can patch the ensurerunningOnSameThread differently

buoyant pagoda
#

@velvet whale @mental ingot #1170668658813575230 message kekw

#

camelot why you no reference

#

i'm quite surprised I remembered my message on that kek

timid fern
buoyant pagoda
#

ah

hollow canopyBOT
#
sciwhiz12

[Reference to](#1170668658813575230 message) #1170668658813575230 [āž¤ ](#1170668658813575230 message)a comment on the PR itself at this stage: why in god's good gravy does IPacketFlowExtension exist?

buoyant pagoda
#

#1170668658813575230 message and the follow-up

hollow canopyBOT
#
sciwhiz12

[Reference to](#1170668658813575230 message) #1170668658813575230 [āž¤ ](#1170668658813575230 message)if the answer to that is for use as method reference, I raise Predicate.isEqual(PacketFlow.CLIENTBOUND)

mental ingot
velvet whale
#

the method ref option involves a patch which involves adding an interface onto a class, which is one of the annoying types of patches because (as it happened before) it can lead to us forgetting to implement interfaces vanilla adds during porting

mental ingot
#

We have several other locations where these kinds of methods are patched onto enums for the same reason

velvet whale
#

if the interface serves no functional purpose then we can have our code a bit less "clean" for the sake of maintenance

mental ingot
#

It is not just our code

#

It is everybodies code

#

And the maintanance is 0 in this particular case

#

If this was a class with a lot of interfaces, sure

#

But it is not

#

It is an enum, which we have regularly patched like this

#

I see no reason to change that now, unless you are arguing that a single line patch is too much, in a PR which touches 10000 LOCs?

iron ivy
#

I don't see the issue with the extension interface for those functions.

velvet whale
velvet whale
#

do you have an example of such enum thonk

mental ingot
#

Any enum that is extensible gets an interface added

#

Any extension object that gets an interface added

velvet whale
#

those are functional

#

and rather "mission critical"

#

this is stylistic

buoyant pagoda
#

in my opinion, that extension interface and its methods are superfluous -- they do nothing by themselves, are easily replaced with Predicate.isEqual, and therefore have no other reason for existing than your personal preference

buoyant pagoda
mental ingot
buoyant pagoda
#

the minecart patches? the same patches which have existed since before 1.12 iirc, and only continue to exist, despite multiple people stating they need to be reworked, because nobody has put in the effort to rework those patches?

#

based on a patch spelunking, the minecart patches has existed in largely the same form since 2013, see https://github.com/MinecraftForge/MinecraftForge/pull/370
and at the time, those methods were made because the minecart types were based on an integer discriminator, rather than an enum (which happened around 2014, it seems)

#

they only exist because nobody has yet found the time to investigate the minecart patches and rewrote them to modern standards

#

this is not the first time the outdatedness of the minecart patches have been brought up (Curle knows; she brought them up a couple of times before), and I dare say it won't be the last

mental ingot
#

Fine I will remove it

#

I think we do modders a disservice

#

Because having properly named and documented methods to use a method refs

#

Seems like a good idea to me

#

Especially when the patch size is that small

#

Actually

#

I will leave it

#

This method is functionally needed

buoyant pagoda
#

and the two methods above that method?

mental ingot
#

Do they really matter

#

?

#

I mean I would have argued that if the patch was only those two methods

buoyant pagoda
#

you have to justify why they should exist over a simple == CLIENTBOUND

mental ingot
#

You two would be right

#

Readability of code

buoyant pagoda
#

the enum is dead-simple -- it literally has two values, and a single method for getting the opposite value
there is nothing substantial to document on isClientbound() that could not be done by documenting the PacketFlow enum or enum values themselves

mental ingot
#

What is the downside to keeping them?

#

I understand that you have a preference for == CLIENTBOUND

#

Or the opposite

#

And when I thought that those where the only two methods in that interface

#

I would have agreed, see here: #1170668658813575230 message

#

But given that it is not the only thing that that interface is used for

buoyant pagoda
mental ingot
buoyant pagoda
#

having two ways to do the same thing is repetition of code, essentially

buoyant pagoda
#

but on this case, I think those methods are entirely superfluous and give no additional value other than preferring method calls to == comparison

mental ingot
buoyant pagoda
#

I did not argue that

mental ingot
buoyant pagoda
#

I am arguing that isClientbound() and isServerbound() are entirely superfluous, because they can be 100% replaced with a normal == comparison with their corresponding enum values (or Predicate.isEqual(Object), if one wants to avoid writing a lambda for a simple comparison in functional programming)

#

other extension methods should be judged based on their context, and removed if appropriate -- that is the normal way for software to evolve over time, as new features are made and old ones are revisited (and rewritten or removed)

#

getReceptionSide(), I can see the merits of
but the other two methods have no such merits

mental ingot
# buoyant pagoda I am arguing that `isClientbound()` and `isServerbound()` are entirely superfluo...

Sure they are superfluous, but we do a lot of things to make the life of the modder superfluous, see runtypes, unit tests etc. And I am argueing that this makes the life of a modder easier. Is that wrong no. Is your opinion wrong, obviously not. Are they a maintanance burden, no. Is there any risk of these two methods becoming in issue during porting, no. So what is the harm in keeping them. They cause us no problems, make the life of modders easier, and allow for better readable code.

errant summit
#

I wondered why the other methods were there

buoyant pagoda
#

does this make the life of a modder easier? I would say no. they are perfectly replicable with == and Predicate.isEqual() (and that presumes they have reason to ever need to interact with PacketFlow on their on)
there is no ease of use offered by these methods as compared to a bog-standard == comparison

"this doesn't have a maintenance burden" is not a reason to accept something which doesn't hold any value
every change adds a maintenance burden, make no mistake -- we should keep a critical eye on our changes to ensure that what we add, we add with reason and purpose, to avoid possible bloat

it may be a subjective opinion, but flow() == PacketFlow.CLIENTBOUND is no less readable than flow().isClientbound() (and the same for SERVERBOUND); in my opinion, the former is more understandable because it's a more simple thing to parse: "is this equal to another value"

#

and to note: nothing in the PR actually uses those two methods as method references, as far as I can tell
they're always used as if (packet.flow().isClientbound()), which makes them even more superfluous

mental ingot
#

Okey agreed fine

#

But I will do this only under protest

#

I find it rediculous that you are basically saying that my programming style is worse then yours, and that the two methods give nothign

#

They might give nothing to you

#

But they do to me

buoyant pagoda
#

it's the concept of not wanting to put things into the API that don't need to be there
like, we both know and agree that isClientbound() and == CLIENTBOUND do the same thing here (particularly because the former literally just does the latter)
the question becomes, what is the worth or merit of that method versus just the straight comparison?
in my eyes, there is no worth or merit

mental ingot
#

I understand that comparison

#

But in my eyes there is

#

I personally find == CLIENTBOUND an order of magnitude worse in readability

#

Which is why I tend to add these methods to any enum I touch

#

So that I can have smooth code

#

Both in my if-switches as well as my streams

#

That is just me

#

And I understand that you think the opposite

#

And that is perfectly fine

#

We are basically on an impase

buoyant pagoda
#

at what point do you consider adding is<value> methods on an enum unreasonable? thonk

mental ingot
#

I tend to put them on any enum I have created in the last year or two

#

Especially if I know that the enum will likely never change

buoyant pagoda
#

to note: it is not just me who seem to be at least hesitant with regards to those methods: maty (as the person who left the review on the PR) and Schurli (see comment above about wondering why those other methods are there) both seem to lean in the same direction as me

mental ingot
#

And again, if that other method did not exist

#

I would have removed them without discussion

#

Because then there is a whole other interface, patch etc that we need to maintain

hasty yew
#

I'm sorry, is someone arguing for adding methods to an enum... that check if a value is a given value of an enum, and nothing more? That's, like, the definition of API bloat

mental ingot
#

But that method is there

#

So adding the methods hurts nobody

hasty yew
#

Also yeah can we please stop with extension interfaces when they're not absolutely necessary and just add methods to the class

mental ingot
buoyant pagoda
#

i understand its your personal preference, but our project style seems to veer away (according to the opinions of at least 3 maintainers above) from your habit of is<value> enum methods
it's not an indictment against your preference -- it's simply not a fit for NF and our (attempts at ) curation of our APIs

buoyant pagoda
hasty yew
hasty yew
mental ingot
buoyant pagoda
#

extension interfaces are meant to correspond one-to-one with their extension target

mental ingot
#

And have only default methods

buoyant pagoda
#

that's how self() exists on these interfaces without fear of accidentally ClassCastExceptioning ourselves

hasty yew
mental ingot
hasty yew
#

And in both cases there's only one place targeted to inject stuff into

buoyant pagoda
#

and one of the points of extension interfaces is to move code away from being in a patch, which is harder to read and review than regular code

#

particularly good when the methods being patched in are very long, and need no reference to e.g. protected or less-visible members of the target class

#

(when they need such references, they usually are defined in the extension and implemented in the target)

hasty yew
#

Regardless, my point about isSomeEnumValue methods still stands

mental ingot
hasty yew
#

Because vanilla added one

iron ivy
#

This is not the thread/place to be having architectural discussions about extension interfaces.

buoyant pagoda
#

#neoforge-github thinkies

hasty yew
#

Yes, that's fair. The major, more relevant, question was around the isSomeEnumValue type methods - and for those I still think it's just API bloat

iron ivy
#

I honestly think its being blown out of proportion. The reasoning comes from a valid place: keep the api small. But your forgetting what these functions actually do, they do an equals check. These are standard in out enums (see Dist and others) and don't add any weight to maintaining them. This is pure bike shedding over 2 simple helpers.

mental ingot
buoyant pagoda
#

i consider them superfluous regardless -- they add nothing

hasty yew
#

And exactly, they just do an equals check

#

They're utterly superfluous

iron ivy
#

They are helpers for method references

#

and quick freehand

#

they are super useful for keeping code smelling good

hasty yew
#

Eh, not really all that useful. The only place they're useful and "cleaner" than an equals is as method references in place of a lambda. And even there the lambda isn't that much uglier

#

And they're also nothing someone can't add their own helper for if they really want it, since it's, as you've pointed out, a single equals check

iron ivy
#

I'm ending this discussion here. We aren't going to have a bike sheading argument over 2 helper functions. If you want them gone, propose it as changes to the general code style guidelines.

hasty yew
#

Regardless, you're right that the whole debate is bikeshed-y. Seems like the best bet is to just have maintainers make a decision on what you're doing about that style wise in the future, and then just go with that

#

But just saying the PR is good as it is because the issue is bikeshedding isn't really a solution. That just means you should have the relevant people make the decision, and the relevant people are "maintainers", not "Orion and us whoever else has popped by this thread"

buoyant pagoda
# iron ivy I honestly think its being blown out of proportion. The reasoning comes from a v...

I did a quick check, and out of the enums in NeoForge and FancyModLoader (+ Dist), here are the following enums which have the is<Value> method pattern:

enums in NeoForge which have is<value> methods (out of ~40):

  • ConnectionType -- isVanilla() (no isModded() counterpart though)
  • IFluidHandler.FluidAction -- execute() and simulate()

enums in FancyModLoader which have is<value> methods (out of ~17):

  • LogicalSide -- isServer() and isClient()

model case:

  • Dist -- isClient() and isDedicatedServer()
#

so by no means are they 'standard'

iron ivy
hasty yew
hasty yew
#

Because people will use that as an argument against changing stuff in the old style

#

And my point is that if we know this is a style question that should have a decision made on it... why not make it now?
After all, are there plans to merge this PR in the next 24 hours? If so, you've got a point. If not, just open up a vote in one of the maintainer channels about the style stuff or whatever and call it a day

iron ivy
#

How many times do i have to state we are dropping this topic.

#

If you want to continue the debate, move to another thread.

hasty yew
#

You seem to be implying that the debate is not connected to the topic of the thread. It is - that style decision is one that ought to be made before the PR is merged, unless there're plans to merge the PR within the next 24 hours.

iron ivy
#

It is not connected to the topic of this thread. This thread is for the Networking protocol, not BIKE SHEDDING ENUM HELPER METHODS.

#

I'm sick to death of every single fucking thing we do turning into bike shedding. It HAS to stop.

#

If Matty wanted a reson for noone reviewing PR's. This is it.

hasty yew
#

It has been noted that there is an impasse regarding whether those methods exist: #1170668658813575230 message

I am simply questioning why the resolution seems to be "ignore it and go with what's in the PR". Yes, bikeshedding isn't good. No, the proper solution is not "ignore it and go with what's in the PR". It's "have the proper people make a decision and stick with it instead of debating in threads for ages"

iron ivy
#

I understand fully what you are saying, and im telling you to go pound sand in another channel.

hasty yew
# iron ivy I understand fully what you are saying, and im telling you to go pound sand in a...

Christ man, no need to be rude. I understand fully what you're saying and in saying that it is relevant to this channel, as much as you argue otherwise. However, I'll leave it be given that the proper resolution is for maintainers or whatnot to just decide on a style; my complaint has been about the heavy bias towards just doing whatever the PR author picked for something that takes breaking changes to change

velvet whale
fair sandal
#

What's the matter here?

#

I like isXxx methods for enums

mental ingot
#

Lol

velvet whale
mental ingot
fair sandal
mental ingot
#

@fair sandal and @iron ivy Can i get a review on this. Something more thightly please. I want to round this out during christmas

iron ivy
#

I can probably review tomorrow evening or the day after

fervent token
#

I havenā€™t done any code review and likely wonā€™t have time, but from the small amount of testing I have done it seems things work fine for mods in regards to custom packets and the additional entity spawn stuff

#

Only real comment that I will reiterate is it would be niceā„¢ļø if on server to client packets the optional was of the client player. That way in cases where the client is needed mods can more easily interact with and get the player without having to deal with worrying about class loading shenanigans and storing the variable as a ClientPlayer instead of just Player

mental ingot
#

I thought of that pup

#

But getting the player is not as easy on the client side from the listener alone

fervent token
#

As the two things client side I sometimes have to access which are a little annoying and may trip up some new modders who donā€™t understand sides and class loading as much, is the client player and the client level (though level isnā€™t as convenient to provide)

#

Because you would have to handle the class loading prevention when creating and passing the optional? Otherwise why canā€™t you try grabbing it from Minecraft.getInstance().player which is what mods would have to do

#

Like sure for configuration packets probably not possible, but also the player doesnā€™t exist yet for those so it is fine to just be an empty optional

mental ingot
#

I can check if I can get from the client listener somehow

fervent token
#

Given I agree avoiding the class loading shenanigans may be a bit annoying, but doing it Neo side to make it easier for mods to interact and not make mistakes with it would be amazing and worth a little extra effort our side

indigo hazel
# mental ingot And <@165621075642679297> You still had an idea for the ClientPacketListener rig...

I just tested the approach I mentioned here #1170668658813575230 message with the modded connection check and our handling at the top of the method. NetworkRegistry#onModdedPacketAtClient() would return false for all the cases where it currently exits early and return true at the tail, which is only reached if the packet is handled. As far as I can tell that works perfectly fine as long as the aforementioned method bails early for packets with a vanilla payload ID namespace.

#

Also, I would really like to get rid of that netty "platform dependent" crap. This now prints in the worst possible place in the middle of the initial resource load

fervent token
mental ingot
mental ingot
fervent token
#

would be slightly convenient to also expose ClientPacketListener.level as an optional but I don't think that would fit quite as well into the existing payload info that is passed, and only effects two spots I use, which can also potentially just be replaced by going off of the level of said optional player instead (but I need to look into my code more to figure that out)

mental ingot
#

Making a Level Optional is not a problem

indigo hazel
prisma garden
#

Orion, if you have time sometime in the next few days, I'd be available to sit down with you to go over the changes and update the docs

fervent token
#

will look more tomorrow if I broke something or if it is an issue with this PR, but for some reason when trying to open a menu it doesn't have minecraft set so it seems like init isn't being called before it starts trying to render the screen

indigo hazel
#

Minecraft not being set is actually a downstream failure from the actual error, which is RenderSystem throwing for being called off-thread during Minecraft#setScreen(). The actual error is swallowed by netty though

fervent token
#

ah okay

buoyant pagoda
#

i've been reviewing the PR on and off a bit (mostly off) thinkies

#

i'll try gather enough of my thoughts to a concrete review

velvet whale
#

I want to get pr publishing setup first before I review it kekw

velvet whale
fervent token
#

I feel like I am missing something

mental ingot
#

If that is an XFact message

#

Then I am about to commit those šŸ˜„

#

I am marking them resolved

#

So that I don't try to do them double

fervent token
#

okay

#

well I will have a few more comments soonā„¢ļø

image.png
buoyant pagoda
#

i need to continue my review

fervent token
#

wait should I use ISynchronizedWorkHandler#execute or ISynchronizedWorkHandler#submitAsync if I don't care about the result? The javadocs are a bit confusing especially when it is run synchronously on the main thread when it has async in the name (and the fact that execute calls submitAsync in its impl

mental ingot
#

They are async executions from the perspective of the calling thread

#

If you want to do something on the main thread you can call either one

#

The executeAsync just does not return the future, but adds a log message when the async executions fails

#

The submitAsync returns the raw future, and you are on your own when it comes to handling exceptions

fervent token
#

okay so I probably should use execute even if I don't expect any exceptions from my code. Thanks

mental ingot
#

Yeah

#

Forge uses submit everywhere

#

And then adds its own exceptional handler to disconnect the client with an appropriate message

fervent token
#

I will comment that in response to your thing on the tracker as well (though it will just queue the comment for when I finish my current review, which is why I mentioned it here as well)

mental ingot
#

Done šŸ˜„

fervent token
#

I just meant something along the lines of this: https://gist.github.com/pupnewfster/f85d13bf6de744626c039d10225d0533 (potentially slightly cleaner handling in regards to if the payloads length is 0 and erroring then and also maybe having it explicitly only allow calling the method for if it is client bound(?) but it at least shows the general concept)
is what I was going to write on my comment (in case you want to handle either of those cases that my quickfix example doesn't have)

#

you may also want to edit your comment saying it isn't possible to say that statement was mistaken xlurk

mental ingot
#

Done šŸ˜„

fervent token
#

well more comments incoming from me as soon as I finish looking at the NetworkRegistry (currently have 18 pending ones for this review, not counting the ones I posted a couple days ago that you are working on addressing already)

mental ingot
#

First pass is done

fervent token
#

there you go

#

have a handful more comments

mental ingot
#

Fixed

fervent token
#

what the hell

#

I am putting a break point in my deserialization code and it doesn't get triggered, but if I put a breakpoint in the handling code then that gets triggered and the data is fine...

#

though actually maybe that is just the fact that single player and netty

#

so ignore me

#

though interestingly enough... that means one of my variables isn't getting set, while I believe it used to get set just fine before the changes even in single player

mental ingot
#

Yeah

#

In singleplayer the payload is not serialized

#

But handed through in memory

fervent token
#

so that is going to be an absolute nightmare to track down how many places I might have a variable for the client value get set in deserialization on an object... that I need to make get set while writing it or whatever as well

#

did the old system serialize it for single player for custom packets?

mental ingot
#

Nope

fervent token
#

hmph

mental ingot
#

That has not changed

fervent token
#

yeah I didn't think it did

mental ingot
#

It does as soon as you turn it into a LAN server

#

I patched that memory leak more then once šŸ˜„

steady sonnet
#

that memory leak is so annoying

fervent token
#

but I am 98% positive in the old system this code of mine functioned properly and set the client side storing of the value even in single player, whereas now it isn't functioning as the read/write never is being called. Which makes me confused how it functioned previously

mental ingot
#

I double checked

#

In old cases the Simplechannel did write and unwrite the payload

#

Even if the actual packet was send in memory

fervent token
#

and I am guessing we don't want to do that?

mental ingot
#

No, even if I wanted to, I have no option to do that

fervent token
#

given I am going to have an absolute nightmare trying to track down all the cases where I say set a client variable

mental ingot
#

That extra layer of indirection is simply gone

fervent token
#

okay

mental ingot
#

I can understand that this is an issue

#

But it is not one I can fix

fervent token
#

I am very sad as odds are there are going to be a bunch of bugs introduced in my code.. and stuff but oh well

#

it definitely is something we need to document

#

as it is a very sneaky bug

#

that most people porting will likely entirely miss

mental ingot
#

I will add it to the PR

fervent token
#

great, and yeah it is going to be super annoying to fix, but I do agree not having the added level of indirection is probably better. Though it does make doing initial tests of packets more annoying (and likely will cause a good number of modders to have things with broken read/writes as I don't know how many modders bother testing everything on a dedicated server)

#

do the payloads in single player at least get handled immediately or do I need to make sure to ensure all block positions and stuff in my packets are immutable? (I think they are but I don't remember)

steady sonnet
#

Can we not introduce an option to test serialization by doing a pointless serialization and deserialization on the receiving end?

steady sonnet
fervent token
steady sonnet
#

hmm true

mental ingot
mental ingot
fair sandal
#

We could add a debug option to always serialize packets

#

Vanilla has it but the if (false) clauses get stripped by javac šŸ˜¦

#

Somewhere in the netty pipeline it must be possible

velvet whale
#

SharedConstants moment

fervent token
# steady sonnet hmm true

which actually brings up another issue... of when things might have the serialization say read something from the server side block entity and wants to write it to the client side block entity... depending how it is done you need to be super careful to make sure you are modifying the correct object and stuff. (my case isn't necessarily with block entities but with different sorts of objects where I may want to mutate the client side one before a confirmation to sync back to the server happens... and if it is just a direct reference to the server side one....)

mental ingot
mental ingot
#

They get added to the Netty queue

#

To be processed

fair sandal
fervent token
#

which is what happened in the past as well right?

mental ingot
#

In practice they are processed as fast as possible

fair sandal
#

I have no idea what the stripped vanilla code does, I just know it's possible

mental ingot
fervent token
#

okay that's what I thought

#

so I don't have to bother validating about mutable positions as I shouldn't have them in any of the spots where it would cause issues like that

#

but other stuff.. headache

velvet whale
mental ingot
#

The simple argument is: treat the payload in the same way mojang does

#

Because it is processed in the exact same way packets are

#

If mojang does not do any extra processing then we do not need that either

fair sandal
#

They have a boolean to do the extra processing in dev

#

(In SharedConstants)

fair sandal
mental ingot
#

Yet it introduces a massive change in behaviour between production and dev

#

I vote no

#

The packet processing should stay identical

velvet whale
#

a debug option

mental ingot
#

Why would that option be needed!?

#

Like in what case would you need the debug option?

velvet whale
#

pup gave an example. your packet accidentally has a mutable BlockPos passed somewhere

fair sandal
#

The only reason not to do it is that I have no idea how to implement it

#

If someone knows we should definitely do it

velvet whale
#

and it's likely to be a problem with any mutable object in a packet anyways, so if you have no idea where, a debug option to make sure that it's not being mutated is useful

fair sandal
#

If you say "I don't know" maybe we leave it for later

#

But if you know please write it cause it's immensely useful!

mental ingot
#

An assumption I could make is that they change the connection channel implementation type

fair sandal
#

Maybe we ask a problem causer

steady sonnet
#

question

#

when are packets normally serialized in multiplayer

#

in the same call stack where send is called, or later, by the netty thread?

#

I would like to see under what conditions the mutable blockpos situation is a problem

mental ingot
#

The only one that I can find

#

Is somebody sending a packet in SSP

#

In which a mutable object exists

#

And then they update the object

#

Something like a loop over positions

#

Which then get put into a packet

mental ingot
#

As if it is stripped

#

Or as if they added a custom processor or something

#

Something I have no control voer

fair sandal
#

Javac probably strips if (false constant)

mental ingot
#

But all classes and indicators of use are gone

#

Like I would expect there to be something

#

Anything

#

But nope

#

So

#

The Connection is simply not configured

buoyant pagoda
#

hmmm yes, conditional compilation

#

my favorite kek

mental ingot
#

I could add it

#

Probably

#

But not in this PR

#

It would require a significant amount of testing

buoyant pagoda
#

does MC have any (visible to us) usages of the FAKE_MS_LATENCY and FAKE_MS_JITTER from SharedConstants?

mental ingot
#

I will check

mental ingot
buoyant pagoda
#

so it's also conditionally compiled away

#

makes sense

mental ingot
#

I assume it belongs to the same codeblock

brave rose
#

Fabric API does it using a JVM flag and enables it by default in dev; its implementation can be checked.

fervent token
# mental ingot No, even if I wanted to, I have no option to do that

Why do we no longer have the option to do this? Given the more I think about it the more I believe it will lead to cases where various mods have broken code or bugs that they don't realize, as now you definitely have to test on both dedicated and in single player as the two behave functionally differently rather than being able to rely on the fact that you have a function in and a function out

Like consider the relatively simple case where you want to sync the size of a map from the server to the client. At some point you need to store the size in a variable on the server now and keep it up to date rather than being able to just grab the already stored size from the map and then set that on the client side. Is it at least possible to potentially make some sort of method that gets fired when the memory transfer is occuring to then set any data that is needed clientside that the server does not have in the same form as the client expects?

brave rose
#

I agree; this feature would be useful for developing Create.

fervent token
#

yeah my biggest concerns by far are that:
a. it used to function that way so mods that are just being ported (especially large mods) are going to have an absolute nightmare of a time making sure everything still functions properly and it didn't break in some unforseen way
b. it adds a lot more states that mod devs have to actively test for to get a good sense of if their thing works as expected or not (which I doubt the average modder will bother to do)

shell matrix
#

Could that be solved by good documentation?

fervent token
#

part a most likely yes, part b probably partially

fervent token
#

Actually orion, is part of the reason both AdvancedOpenScreenPayload and AdvancedAddEntityPayload stash parts of the data as a byte[] (rather than just proxying the calls) to effectively force those to be serialized and deserialized?

mental ingot
# fervent token Why do we no longer have the option to do this? Given the more I think about it ...

So i looked it up. Turns out this behaviour is at best a implementation detail of SimpleChannel.

The way SimpleChannel worked is that your SC-payload would be written to a FBB, which then grabbed the byte[]. From there this data was added to the vanilla CustomPacketPayload instance with the int descriminator prefixed before the byte[].

This means that this only worked because you were writing to an intermediary buffer, not sure why it worked liked that but that is what SC did.

I can not find any evidence of this in EventChannel, where the raw buffer was passed.

This behaviour is additionally nowhere documented, to be honest I have heard to oposite more often: "Watch out for SSP packets, they are transferred in-memory." Now that might be because I worked a lot on networking memory leaks in SSP LAN Enabled worlds, but in reality I am not sure why this was never documented.

Now as with can we re-implement this: this exact behaviour of SC: No, the extra layer of indirection is simply gone, we use the native FBB that Mojang gives us. I also see no reason, as it was not done in EC and not in Fabric.

Now with respect to a debugging option: We can add it later on in an extra PR sure, probably only for in-memory-connections, in-dev, and with a big fat warning, since the overhead is a couple of orders. But we can do it as a channel processor probably.

mental ingot
# fervent token Actually orion, is part of the reason both `AdvancedOpenScreenPayload` and `Adva...

Kind of yes, kind of no. I remember discussing this, the primary reason why it is a byte[] is the SSP-LAN world scenario.

There the same objective payload (as in the exact same instance in-memory) is send to all players. Important of note here is that this happens in the order of the players joining. Meaning that the local player is always first. This caused an issue when we kept the FBB there, because it ment that one thread could be reading from it, while the other was also copying its contents into another buffer (See the SC message above). In the end this was a bit weird of a behavbiour, and did cause issues with freeing up the buffer, or a significant increase in memory consumption due to over allocation of the buffers, and their immediate destruction. It was in the end considered simpler, easier and safer to write the entity/screen custom data once to a byte [] and treat that as an immutable data structure that is then passed to new FBB instances, or written to the network thread.

#

That way each buffer could be created and freed on its own terms

#

And without the memory thrashing overhead

fervent token
#

hmm interesting, what times should mods end up writing objects of theirs in a similar way to how we handle the entity payloads and stuff?

#

Also there are a few commits of xfact's you marked as resolved the other day but didn't address. Should I mark them as unresolved or add a new comment? @mental ingot

mental ingot
#

So.....

mental ingot
fervent token
#

unresolved the two and then added a few of more comments of my own

mental ingot
velvet whale
#

so @mental ingot is your trailing whitespace problem fixed after you imported the formatter config? if so you should remove it from the patches thinkies

mental ingot
#

Me and covers tried finding as many as we could today

#

Anything that is left

#

Will stay

#

I can't look at them anymore

#

@fervent token Can I ask you for a rereview

fervent token
#

just finished with an appointment

mental ingot
#

Okey

fervent token
#

IServerCommonPacketListenerExtension

#

the send method has FQN in it

#

you can switch it to an import

#

the problem with spotless. If you move the exceptionally to the previous line OR if you move the () -> { of the submitAsync to the next line like it is in the advanced entity payload handle method it should hopefully look less derpy and bugged out

#

but other than that I think it is decent? Assuming nothing blows up in my face when I do a test on a dedicated server, and also the fact that at least a couple of my spots still have issues in regards to it no longer serializing and then deserializing so I need to fix that in my code

mental ingot
#

Fixed

fervent token
#

okay, I am heading for a walk but will look afterwards

mental ingot
#

Okey

fervent token
#

if you haven't don't forget to add the warning to the description about the serialize deserialize that simplechannel used to do not happening anymore in singleplayer

mental ingot
#

Already done on the registrar

#

As well as on the PR description

#

And I will add it to the blog post

fervent token
#

is this meant to be translated?

image.png
#

I don't think you included english values for any of your new translation keys

mental ingot
#

Correct

#

I have not yet

#

Still need to do it šŸ˜„

velvet whale
#

Registering payload handlers outside of the event handling scope will result in those payloads not being known to the system and not being sent over the connection.
can we get it to error

mental ingot
#

Probably....

mental ingot
#

@velvet whale and @fervent token Addressed your concerns

velvet whale
mental ingot
#

For fucks sake IDEA

#

WHY ARE YOU SO STUPID

velvet whale
#

tbh I've always found auto import or auto format on save and co to cause too much trouble in any project, that's why I always disable them

#

it always manages to break something or format classes needlessly making contributing painful

mental ingot
mental ingot
#

All registrars will become invalid immediatly after the event

fervent token
#

If self causes performance issues due to casts at times should the byte buffer extensions stash the self in a variable per method rather than retrieving it multiple times per method?

mental ingot
#

I started patching in the method after the 1.18 phase when covers ama and I did research into a new chunk rendering pipeline and we noticed a significant hit to performance due to this.

#

Now how and when it comes into effect I don't know.

#

It could be because it of the class hierarchy

#

But I just go in a better safe then sorry route when I add new ones

iron ivy
#

We should really have an internal marker annotation we put on the self() method, and just strip their usage with a coremod, they should be replacable with a ALOAD 0 at runtime

velvet whale
#

not without a checkcast, and we should see some numbers first

iron ivy
#

It will work without a check cast

#

if the cast is required it's far faster than the invoke

mental ingot
#

The point is

iron ivy
#

Orion and I have numbers from when we worked on the alternate rendering system

mental ingot
#

I know from past experience that it does really influence the speed of the call sites

iron ivy
#

it is ungodly slow to have that self() call

mental ingot
#

Yes

#

But right now we have no hard numbers

#

However it is trivial to circumvent

#

WIth the implementation of the method in the class

iron ivy
#

There is still a perf hit to it slightly as its invokeinterface

mental ingot
#

Yeah

#

But the invokeinterface would also be hit on the default method as far as I understand it

#

Because it could be overriden / implemented

#

So that would be a constant performance penalty payed by every extension interface

velvet whale
iron ivy
#

the cast would be faster regardless

#

We could probably also hois the interface default impls onto the parent class at runtime with a coremod

#

but we are into tin foil perf optimizations here

mental ingot
#

Yeah

#

The simple patch suffices for now

#

We need to do some more research later

#

And then figure out how to with all of this

#

I wish we had mixins for these

#

Would solve the problem immediatly

velvet whale
#

if we're targeting this for sunday (tomorrow, 31st, say 21 UTC) then we need to do a final RFC announcement today Thinkies

mental ingot
#

I am going to address your comments as fast as I can

mental ingot
velvet whale
#

#announcements (i wouldn't do #dev-announcements until after the PR is merged for the sake of better visibility)

mental ingot
#

Can you do that

#

?

#

My brainpower is currently exhausted from dealing with this PR and my sick wife, and afraid dog

fervent token
#

if you are concerned that it might be a performance issue

mental ingot
#

I am concerned to the point that I patch the methods in newer extensions

#

But at the moment not enough that I go retroactively changing it in existing extensions

#

I am still on the fence about it to be honest

#

For the new ones I air on the side of caution

#

For the old ones I leave it be

steady sonnet
#

in the past self was not overriden by the implementing classes, it was just left as a default method, right?

mental ingot
#

Yep

#

With a cast inside

indigo hazel
mental ingot
#

And because neither javac nor the jvm had any idea that inserts a lot of overhead code to check cast ability etc

mental ingot
indigo hazel
indigo hazel
steady sonnet
#

Maybe we should work on a JMH test and determine a conclusion on the problem once and for all šŸ˜„

indigo hazel
#

šŸ˜„

velvet whale
mental ingot
iron ivy
#

the perf will likely depend on the number of classes implementing the interface and how far deep the interface is in the hierarchy

velvet whale
iron ivy
#

Can you make a varint of both that has some hierarchy to it?

mental ingot
#

That is also not the test maty

#

Make the method on SomeExtension default with a cast of this to SomeObject

#

And remove the implementation of SomeObject

#

Allthough

#

I don't understand why in your test testPrivate is faster then testDefault

#

Well no

#

The private method is probably faster because it is not a virtual invoke

#

I am guessing

iron ivy
#

It is a vinvoke, but not through the invokeinterface table

mental ingot
#

Yeah

#

Interestingly enough

#

They are for sure within each others margins

iron ivy
#

We did not have those private methods when we did our testing

mental ingot
#

Yeah

#

That was on J8

#

Because we made the pipeline for 1.16.x

#

I checked

iron ivy
#

yeah

mental ingot
#

But yeah I would like to see the JMH test fully

#

With all three variants

iron ivy
#

private method is probably good enough in J17 then

mental ingot
#

And some complex inheritance trees

iron ivy
#

as jit will inline it if its a hot path

mental ingot
#

Because we only really ran into this in BakedModel

#

Which has a really annoying inheritance tree

iron ivy
#

Yeah

mental ingot
#

And the point is, that it had to check for castability

steady sonnet
#

the extension interface wasn't added to BakedModel itself?

mental ingot
#

It was

#

But that says nothing really

#

Because the compiler has no way of knowing that any given object returned from the self method is a bakedmodel at all times

#

Because the extension interface does not declare that

steady sonnet
#

ohh yeah because bakedmodel extends the extension, not the other way around

mental ingot
#

For all it knows a modder added that interface to their random class

iron ivy
#

becaise its a invoke interface, it has to hit the invokeinterrace table on each concrete class in the hierarchy, etc

mental ingot
#

Yep

#

We did not have the time or the infra to dig deeply then

#

We scratched our heads for the bad performance

#

Got a trace

#

Saw the hotspot on that call

#

Patched it

#

Made a note

#

And moved on

iron ivy
#

Like 90% of the time was in a function that was using self() 3 times

mental ingot
#

And even if we made a field

#

It was terrible

#

That was the weird case

fervent token
#

Interesting

iron ivy
#

i moved the self impl into BakedModel, amazing, all went away

#

The interesting thing was that self() was not listed as a hot spot, the function calling self() was

#

I think private method for self() will work fine, it should be invokespecial

mental ingot
#

Should

iron ivy
#

as its not a virtual call with hierarchy then, jit will inline in hot paths

mental ingot
#

But if we are on the testing path now anyway

iron ivy
#

it will be 100% fine

mental ingot
#

We should just get the numbers

steady sonnet
#

so the problem wasn't even the cast, it was the repeated virtual dispatch?

iron ivy
#

yes

steady sonnet
#

šŸ¤¦ā€ā™‚ļø

mental ingot
#

And then open a pr next week and fix it for every single extension all at once

iron ivy
#

or rather, just the overhead of invokeinterface

mental ingot
iron ivy
#

regardless, numbers

velvet whale
#

you'll have numbers in 12 minutes and we can stop with this "it did... or didn't... or doesn't.." once and for all

šŸ“Ž message.java
iron ivy
#

can you gist that

hollow canopyBOT
velvet whale
#

use the 'button' šŸ˜›

iron ivy
#

I don't understand why that needs to exist, but okay

velvet whale
#

the reaction? for gisting duh

fervent token
#

And because not all users understand how to gist

#

Even if Maty could have

indigo hazel
iron ivy
indigo hazel
#

Yes

iron ivy
#

What happens if you have 2 interfaces, each with a private self() method, that does something different, and implement it on a single class

steady sonnet
#

honestly I would have expected java to devirtualize an invokeinterface if the method isn't implemented anywhere except the interface itself, hearing that it doesn't is a bit disappointing

#

I guess, to be fair, it's likely nobody designed the JVM infrastructure around interfaces with this in mind

iron ivy
#

remember, @mental ingot and i tested back in J8

velvet whale
steady sonnet
#

think covers was probably more interested in the generated bytecode

fervent token
#

And also potentially the case i2 extends i1 instead of being applied in parallel. Granted the parallel might be different yet given we are talking bytecode

velvet whale
#

it's still invokeinterface

image.png
iron ivy
#

this actually works as expected, I feel like it really shouldn't

steady sonnet
#

hot take: in Java 25 they should deprecate interfaces and just allow multiple inheritance

fervent token
#

Wait when default instead of private is the invoke interface call referenced any differently in bytecode?

iron ivy
#

No, both of those get() calls are invokeinterface

indigo hazel
#

Nope, looks completely identical

iron ivy
#

I assume when resolving the method call at link time, it see's its private and probably hotwires the table

#

as invoke interface is meant to search from the root of the tree down

#

but its clearly not here

velvet whale
hollow canopyBOT
velvet whale
#

the private method is faster when inheritance gets involved..

iron ivy
#

interesting default gets faster with inheritence too

velvet whale
#

default is twice as slow as override sure, but the private method is faster harold

velvet whale
#

0.113 (override) vs 0.211 (default)

iron ivy
#

reading the wrong number

velvet whale
#

but yeah, the way to do it is with a private method

iron ivy
#

yep

#

very clear

#

oo

#

can you make a variant that does (Cast) this instead of an invoke at all?

#

so we can try and see if the jvm inlines the private self()

velvet whale
#

oh you want a return (X) this in the runs?

iron ivy
#

ye

#

in the run() func

fervent token
#

So @mental ingot change the new extensions you are adding to be private on the interfaces for the impls of self instead of override

iron ivy
#

doesn't make sense for the others, its the same test

steady sonnet
#

the override is faster than private but only in the non-inheritance case

#

if I'm interpreting this correctly

velvet whale
#

great so i don't need to wait 14 mins ha

iron ivy
#

Somewhat releated, how do i search a repo on github for commit keywords?

mental ingot
#

So

steady sonnet
#

Usually I just search and then choose commits on the "Filter by" sidebar

mental ingot
#

I need to make the self() private and that fixes it

#

Cool

#

Thanks @velvet whale

iron ivy
steady sonnet
#

neat, so we should be changing all of them to private, I take it?

iron ivy
#

The second one is the most interesting, if the invoke interface targets a private method, the jvm overrides it to an invoke special

steady sonnet
#

looks like it was also backported to java 17

indigo hazel
iron ivy
velvet whale
#

#announcements message punny announcement done

hollow canopyBOT
#
matyrobbrt

[Reference to](#announcements message) #announcements [āž¤ ](#announcements message)Hello!
As you might have heard, a networking rework was due for 1.20.4. And we're offering it to you as a New Year gift!
The Networking Refactor PR is in a final RFC period and is scheduled to be merged <t:1704056400:R>, if no critical issues are brought up.
Any comments and reviews are appreciated! If you want to test the PR, you can make use of our new (:Catjam:) PR publishing setup. Installer links, repository declarations and MDKs for this PR can be found here, so you can be ready for the LANding of the rework.

Happy... networking?

steady sonnet
#

why does that announcement feel so cringe at the end

velvet whale
steady sonnet
#

I just read through the PR description fully, very well written IMO. It doesn't seem to say what the final decision on modlist mismatch handling was - did we decide that?

mental ingot
#

After testing: Right now it is simply not part of the protocol modmuss designed

#

And for functionality it is not needed

#

As such it was left out of this PR

steady sonnet
#

šŸ‘

mental ingot
#

Now with respects to whether we want that back

#

That would be a community decision

steady sonnet
#

so as a side effect: servers no longer know what mods a client has, just what payloads it registers, right?

mental ingot
#

Yes

iron ivy
#

Had this discussion with Orion last night. It's worth exploring as future work. Theres a few use cases for the server knowing what mods the client has. Server networks/public servers, etc, etc.

steady sonnet
#

Personally I like the approach of going by payloads/registry contents

mental ingot
#

See the Follow up PRs section

mental ingot
steady sonnet
#

It's very good, practically 80% of the blog post right there šŸ˜„

#

if not more

mental ingot
#

That was kind of the idea

#

šŸ˜›

buoyant pagoda
#

what's this I hear of excessive PR descriptions thinkies

mental ingot
#

I mean have you read the thing šŸ˜„

#

It is infinitely longer then Techs šŸ˜„ XD

velvet whale
#

we still need a blog post Stabby

steady sonnet
#

blog post is just copy Orion's PR description, remove some implementation details that don't matter for a modder, fix a couple mistakes, shipit

buoyant pagoda
#

btw Orion, since I can't complete my review, Imma make a comment here:
your doc comment for your overload in BundlerInfo#unbundlePacket is incorrect

mental ingot
#

I am writing that and documentation prs tonight šŸ˜„

#

Okey

#

somebody leave that as a PR comment somewhere

buoyant pagoda
#

the method's logic isn't to write the bundle packet, the logic is to convert any bundle packet into the series of original packets it contains (with delimiting packets for anything downstream that cares about it), and pass through packets which aren't bundle packets

#

i had a pending review comment in the past, but it got wiped harold

mental ingot
#

Ahh right

#

Okey

velvet whale
#

there's already one download of the github package of this PR!

fervent token
mental ingot
#

No I am already on it

mental ingot
#

@fervent token Addressed your request

#

@velvet whale and @indigo hazel Addressed your requested changes as well

#

@buoyant pagoda Addressed your changes as well

velvet whale
#

uh

#

you pinged the wrong person Thinkies

mental ingot
#

Sorry geo, wrong person in the box

velvet whale
#

don't edit the ping, it will be more confusing Stabby

fervent token
#

I will take a peek in like half an hour

velvet whale
#

"opinions on networking refactor were requested from modrinth developer"

#

i can already see the drama headlines /s

mental ingot
velvet whale
#

hmm? it was just a reaction to you pinging geo šŸ˜›

topaz parrot
#

lolol

mental ingot
#

Ahh okey

#

Yeah my mistake

fervent token
#

ahhh

#

orion why are so many of your patches with random spaces on blank lines

mental ingot
#

Well and VanillaFlower

#

Turns out vanilla flower will add the indents when it deals with inner classes, or lambda callbacks

#

Idea detects them

#

And then thinks that the rest of the file should have that as well

fervent token
#

so I don't have like 30 comments on your PR can you search in files on the patches that you are modifying for \+\s+\n (using the regex find in files) and then fix them

obsidian nova
#

Or what is VanillaFlower?

mental ingot
#

Vineflower

mental ingot
#

And as already talked through with Maty

#

I spend a good several hours with Covers trying to fix this 2 days ago

#

I won't be further touching those whitespace changes

#

Either institute a global formatter over both the MC and Forge code

#

Or fix VineFlower to not output shit

fervent token
#

fine, though there are only 23 files total (when doing a find in files on the patches folder for \+\s+\n) that have this issue some of which aren't touched by your PR so it would just be a subset of it and not 160+ files

mental ingot
#

But do not hold your breath

#

I will only do it if I have the time

fervent token
#

that's fine, still have other comments I will post after I finish going through files that have changed since I last viewed them

#

do you mind if I make a commit fixing the spacing thing orion?

mental ingot
#

No be my guest

#

But be aware when I edit the file again

#

They will be there again

steady sonnet
#

Maybe better to fix it after all changes have been made and before merging then

fervent token
#

yeah or given some cases aren't touched by this PR I can just make a PR after the fact that fixes all the cases it happens

mental ingot
#

I would do it after the fact

#

Personally I would like to discuss having a formatter run in NG7 to standardize the entire sourcecode

#

That way shit like this where we depend on the formatting of an external tool on which we have no, or extremely little, influence has no influence on our patches anymore

steady sonnet
#

Does that mean the patches get polluted by the formatter's changes though?

#

Or does the formatter run before patch application

mental ingot
#

Run before

#

It would basically be as if the decompiler was formatted in that way

#

Or not it

#

but its output

steady sonnet
#

Seems reasonable to me

fervent token
#

two things. One the reason doesn't seem to be localized, and second is the reason it shows the modid as the channel name so many times that it shows once per registered payload? (and if that is the case should we maybe show the payload's rl?)

image.png
mental ingot
#

That is indeed wrong XD
That should be the payload id

#

Not the namespace

brave rose
mental ingot
#

Yes it can be easily fixed

#

But no the rule on ensuring compatibility has been weakened

#

We try to stay compatible where possible (and here I missed adding the field, so I now pushed the change as you indeed suggested)

#

But if there is no direct obvious way, like in several other locations in this PR to remain compatible, then we will patch method signatures.

brave rose
#

Alright

fervent token
#

meh in this case if we patch in the other constructor it would allow removing the patch in the same class to createInitial and iirc remove a patch adding the , false in one other class

#

so maybe worth it? idk

#

I was indifferent so didn't make a review comment about it

mental ingot
#

I am addressing it now

#

And running some test

fervent token
#

okay so locally you are removing the now unneeded patch to CommonListenerCookie#createInitial and ServerCommonPacketListenerImpl#createCookie(ClientInformation)?

mental ingot
#

Yes

#

Already done

fervent token
#

okay great

mental ingot
#

Currently running the packet splitter test

#

Which takes a bit of time

fervent token
#

I'm almost done fixing the things in my code that were relying on the serialize/deserialize of SimpleChannel xlurk. Though the last few cases are definitely super annoying logically when it comes to deciding how to address them

fervent token
mental ingot
#

No I am adding that still

mental ingot
#

Okey

#

Test past šŸ˜„

#

We now have a fully working packet splitter in NeoForge

#

For all packets šŸ˜„

#

Modded or not šŸ˜„

mental ingot
#

@fervent token All addressed

fervent token
#

I can check when back at my pc but at a glance I donā€™t think you fixed the missing translation key from my picture (just the payload name thing)

fervent token
#

nvm seems it is localized now, so must have missed it in the commits I skimmed

fervent token
indigo hazel
#

I just spent far too many hours trying to find why a Neo tests client can't connect to a Neo tests server. Turns out the packet splitter has two bugs which meant the oversized registry packet wasn't received:

  1. It doesn't actually write the prefix data (i.e. the "state" and, in case of the first packet, the vanilla packet ID) to the packet slice
  2. It doesn't trim the state off the received packet slice, so the decoding of the collected slices then produces nonsense
    Here's a quick patch: https://gist.github.com/XFactHD/489fafc6986cc3c410e1f9843a561c29
iron ivy
indigo hazel
#

Right, good call šŸ‘Œ

static kelp
#

So.. staring at the write-up. Lots of noise. In dev voice proofreading.

image.png
#

Also.. I'd argue that implementing the functional interfaces for payloads via constructor can be messy, and they could be final fields on records instead.

image.png image.png
#

You can breakpoint the FI version, and it looks similar to how I've seen quite a bit of modder packet code (less wrestling everything into one line)

#

Is it possible to just offer up a codec, and have the reading+writing handled by that internally?

static kelp
#

ChannelHandlerContext#sender - mentions this is a player optional. Does the platform have a player UUID yet, during configuration? If so, passing it here as well means mods can do some lazy init for when the player gets fully constructed. Also means that ban lists and similar features are more possible.

static kelp
buoyant pagoda
#

is this Obsidian

static kelp
#

yes

mental ingot
mental ingot
mental ingot
#

It is eventually planned

#

But not as trivial as you might think

errant summit
#

@mental ingot I sanitized registry snapshot a bit so we are not creating and copying FBBs everywhere, should I commit to the branch or send it to you as a patch?

mental ingot
#

Commit

mental ingot
mental ingot
errant summit
mental ingot
#

Weird it worked for me

errant summit
#

pushed

errant summit
mental ingot
#

okey

#

@indigo hazel I applied your patch and now I have a broken ref counter

#

@errant summit Your PR be broke šŸ˜„

#

I think I got it šŸ˜„

#

You released the snapshot buffer before grabbing the data array

#

You fucked something up

errant summit
#

Oops (that happens if you refactor the same code twice and forget what you did in the first)

mental ingot
#

I also now have a netreg error all of a sudden with unknown packet id!

errant summit
#

Weird because the read and write code are the same except for the try-finally block

#

I'll fix it after lunch

mental ingot
#

Fixed

#

You used the wrong reading and writing methods on the FBB to write and read the snapshots

#

Just a PSA: Do not use readBytes / writeBytes if you want to write a bytearray

#

It does not do any size, prefixing or terminating

#

So you frequently end up reading and writing over other data

#

Which is what happened here

#

Properly using readByteArray() and writeByteArray(...) fixes the problem

errant summit
#

I did that on purpose because I am reading the data directly and not wrapping a byte array in another FBB

mental ingot
#

And in reality it does not matter

errant summit
#

That is what it did before except it stored an FBB instead of the contained data in a byte array

mental ingot
#

Yeah

#

But as soon as I used writeByteArray()

#

and readByteArray() it was fixed

#

The problem with readBytes and writeBytes() is that it has no termination logic

#

It will read and write as long as it can

#

Which is problematic

errant summit
#

Hmmm how did that ever work then

mental ingot
#

It did not for me šŸ˜„

#

As soon as I added the writing and reading via the byte array it was fixed

errant summit
#

I mean before my changes

#

Because I just removed the FBB indirection when writing

mental ingot
#

Because it was ByteBuf to ByteBuf

#

I just know that Byte[] are really problematic

#

And should only be used with their corresponding methods

errant summit
#

writing bytebuf to bytebuf does the same as writing byte array to bytebuf with writeBytes

mental ingot
errant summit
#

it uses setBytes under the hood for both which copies the data over

#

(at least for unpooled which is what is used here afaik)

mental ingot
#

No it does not

#

writeByteArray writes the length of the byte array first

#

where writeBytes does no such thing

errant summit
#

ik the old code never used writeByteArray

mental ingot
#

Basically writeByteArray should always be used

#

Unless, and only unless you know exactly what you are doing and want to use writeBytes without its terminator

iron ivy
#

I think there is another issue at play here, Schurli's intention was to append the raw buffers together. provided you still read in the exact same sequence it should be fine. Something is not incrementing/resetting reader/writer indexes, or there is an issue with readRL

errant summit
#

but it technically isn't a byte array but just the cached versions of the buffer write operations

mental ingot
#

I don't know what to tell you man šŸ˜„

errant summit
#

I am doing exactly what the old code did but storing it as a byte array instead of the FBB directly

mental ingot
#

Supposedly there should be no difference between writing a byte[] and writing a FBB

#

Figured it out šŸ˜„

#

There is a significant difference between readBytes and readByteArray

#

In particularly because readBytes reads as many bytes as will fit into the buffer it is given

#

So if you do not make exactly sure that your BB from which you copy and the BB to which your read the bytes again are of the exact same size

#

It will just copy anything

#

Causing it to misallign

#

That can not happen with read/writeByteArray

#

Because it prefixes the size on its own

#

@errant summit Does that explain the issue?

errant summit
#

but it did not do any readBytes or readByteArray it only wrote

mental ingot
#

Hmm

#

Weird

#

You are right

#

Then I am out

#

And really I don't have time today to further investigate

errant summit
#

I'm investigating rn

mental ingot
#

If you figure it out, I would be really interested

errant summit
#

I know what the problem is... the .array() call returns an array that is longer than the actual data in it and thust increases the writer index too far

iron ivy
#

.array on an array backed BB will return the actual backing array, not a buffer of the readable data

mental ingot
#

Lol

#

Okey

buoyant pagoda
#

that's probably documented behavior kek but the method is so tempting

iron ivy
mental ingot
#

Or is the PR okey for now

#

?

errant summit
#

I'm going to patch it back if I find out why the client is missing the damage type registry

mental ingot
#

It was not on my end.....

errant summit
#

did you test with testmods?

mental ingot
#

Yeah

#

I ran the loginsplitpackettests

#

To figure out if the big registries worked

errant summit
#

I'm getting Unknown registry ResourceKey[minecraft:root / minecraft:damage_type]

mental ingot
#

Does it happen without the test mod?

errant summit
#

during handleUpdateTags

mental ingot
#

Hmm

#

Weird

#

Maybe a test that was not properly ported somewhere?

errant summit
#

works without testmods, it seems like ClientboundRegistryDataPacket#handle is not called either

#

when testmods are enabled

#

I pushed, it works in neo idk what is wrong with the tests I'll check later

prisma garden
#

@mental ingot Would you be available sometime tomorrow (evening) to go over the changes for the docs?

mental ingot
#

I am writing both blog posts and documentation now

#

Not sure how far I will get before the deadline

#

But yeah sure

prisma garden
#

alright

#

ping me when you're done, then I'll have a look over it and give some first time reader input

indigo hazel
fervent token
#

But given schurliā€™s second commit is after Orionā€™s that mentions fixing the bugs you mentioned, does that mean it isnā€™t actually fixed?

indigo hazel
#

Depends whether he pulled before testing

mental ingot
#

No i fixed that

mental ingot
fervent token
#

btw orion not sure if they have been resolved or not but according to the PR there are still two unresolved conversations

#

one I am fairly certain is out of date and is resolved, the other not sure

mental ingot
#

I can't resolve those for what ever reason

#

They have no button for me

fervent token
#

are they resolved? if so I can try

#

given I can see the buttons for them

#

looks like they are

#

marked them as resolved

#

though there are still the comments I added this morning that need to be addressed

mental ingot
#

Yep on it

fervent token
#

I forget if spotless disallows wildcard imports but if so it will be angry at you for the change you just made to GenericPacketSplitter

mental ingot
#

It will be

#

But I did not make the wildcard import

#

Hold on

#

Fixed

fervent token
#

also I didn't get an answer to #1170668658813575230 message (doesn't really matter from a PR ready perspective, it is more me being curious and wondering for the packets I have that can be on the larger side)

mental ingot
#

No opt-in

fervent token
#

okay

mental ingot
#

Given that the new limits for packets is 8MB

#

I doubt it will be used much

#

So..........

fervent token
#

ah

#

that is quite large

mental ingot
#

Both ways btw

fervent token
#

did mojang up that

#

or is that from our changes

mental ingot
#

Combined

#

Mojang upped the protocol limit

#

But due to the new way we transmit stuff we are not limitted to the max size of the "unknown" custom payloads

#

Because technically we are now known custom payloads

fervent token
#

useful

mental ingot
#

Yep

fervent token
#

just a few conversations

image.png
mental ingot
#

šŸ˜›

delicate dawn
#

I just pictured like, a bloody sword, and orion like "ANYONE ELSE HAS SOMETHING TO SAY?!"

mental ingot
#

Yes and no

#

I am happy that a lot of people are involved

#

But I kind of slowly have enough

fervent token
#

if it makes you feel any better orion, you are at 190 comments (according to github), the registry rework was 199, and the cap PR was 250

buoyant pagoda
#

then it sounds like we must go even further!

velvet whale
#

let the nitpicking begin

fervent token
#

Is the configuration packets in general for use cases where you would previously instead send a packet from the PlayerLoggedInEvent?

#

(getting to the point where I am evaluating if any of my packets should be moved to configuration)

mental ingot
#

Yes

fervent token
#

why does SyncRegistries not capture the listener and notify as a completed task but the other two syncs we add do?

mental ingot
#

Because the Sync sends acks

#

And the acks can just complete the config task via the context

#

The mod phase start and end tasks, call internal methods

fervent token
#

so for modded tasks is it recommended to do the listener thing or is it not necessary?

mental ingot
#

Not needed

fervent token
#

also is it recommended for mods to include the if (event.getListener().isVanillaConnection()) return; if the mod is expected on both sides?

mental ingot
#

No

#

Because configs don't run if your channel is not set to optional

#

So as long as your channels are configured properly that is not needed

fervent token
#

so then you can't actually make use of configuration packets for those mods?

iron ivy
#

if you have a non-optional channel, then it will only allow the client to connect if that channel exists the other side

fervent token
#

ah so it won't get to the configuration phase

#

I am still a bit confused on when to mark configuration tasks as finished. As in:

  • registries have a packet sent back from the client that it is complete, which then that packet being handled marks the task as complete
  • configs just immediately mark the task as complete as soon as the packet has been sent
  • tier sorting does some weird (which might be a bug??) thing where the task is only added for modded connections but then if it were to somehow be a vanilla connection when the task runs it finishes the task or disconnects. And otherwise sends it and then the client sends a packet back that marks the task as complete
#

but given the config one just marks it immediately is the proper approach to have a packet that is a response or can we get away with just marking it as complete immediately?

#

Also is the fact the tier sorting task doesn't get added when it is a vanilla connection a bug (I am guessing yes? Given otherwise the disconnect thing wouldn't be a branch in the configuration for it)

mental ingot
#

You can capture the listener and immediatly complete

#

You can send a packet and complete in the handler for the ack packet

#

You can do weird combinations

#

It is up to you

#

And your needs

fervent token
#

okay

#

and is the tier sorting thing a bug

#

in terms of the task not being added for vanilla connections

mental ingot
#

No that is correct

#

I think the extra check in the task it self

#

Is just out of pre-caution but is really not needed

fervent token
#

but

#

if the task doesn't get added and TierSortingRegistry#allowVanilla is false

#

then the client won't actually get disconnected for it?

mental ingot
#

That is correct

#

I need to change that

fervent token
#

that was my point of why it seems like a bug

mental ingot
#

Fixed

errant summit
fervent token
#

okay, and yeah I tested after and it seemed fine

fervent token
errant summit
#

so we are good to go for the deadline in 2hours 40minutes

velvet whale
#

well the blogpost still has comments to address thinkies

mental ingot
#

I am on them

fervent token
#

well I will be back in a couple hours (not sure if before or after the target deadline), so hopefully nothing more is really needed from me xlurk

mental ingot
#

@velvet whale Addressed your comments

velvet whale
mental ingot
#

Oeps now it is pushed

fair sandal
#

Don't break everything while people are out celebrating new years eve šŸ˜„

velvet whale
#

uh @mental ingot you may want to consider batching the changes

mental ingot
#

I can't

#

It does not allow me

velvet whale
#

you have to go to the files tab

mental ingot
#

Did I mark that as resolved?

velvet whale
#

yeah, i unresolved them

mental ingot
#

I have too much of this shit in my brain

#

I could have sworn I never saw those two

velvet whale
mental ingot
#

LOL

#

Is being rebuild