Networking Protocol

[spark saffron]

its "magic static modid"

[mental ingot]

Then start an argument, preferrably not with me, about it

But as long as it is there

I think it is infrastructure we can and should use

It lowers the bar to entry for this particular api

I have no problem in removing it

I have no opinion on having that api either way

I can write a trivial wrapper helper that gives me registrar for my mod id

[buoyant pagoda]

how much of a lowering is that? could we not have registrar() in the event require a String modid?

[mental ingot]

Yeah

I would just give you both

registrar() would just call registrar(String) internally

With the current modcontainer id

Alternatively, if you are all really so vocally against that we can do enforced id..........

[buoyant pagoda]

I would consider the option of an added registrar(String), so if in the future we removed the mod loading context system and the contextual modid thingy, mods which used the String variant are unaffected and we can deprecate the no-args one for a good while

i leave the decision of a registrar() with no-args to others, as long as we have the String modid variant

[mental ingot]

Agreed

If @spark saffron and @hasty yew really feel like it should be gone, for some reason, rather then be optional, we can remove it

[fair sandal]

Contextual modmid might be removed in the future

[spark saffron]

magic static context is bad for a slew of reasons

[fair sandal]

But not in 20.2

[spark saffron]

Readability is a huge one

[mental ingot]

And @buoyant pagoda I will likely do what you said: if you register a one sided handler then it will automatically set the packet flow

[buoyant pagoda]

is the "might" good enough that we shouldn't offer the no-args variant in the first place, to ease one part of the API if that does come to pass

[spark saffron]

yes

[buoyant pagoda]

actually, that begs the question: what other APIs do still rely on the contextual mod ID system?

[mental ingot]

Let me check

[spark saffron]

not much for the modid helper method

more for the mod container access

[mental ingot]

So all of configs

[spark saffron]

this is 20.1

[mental ingot]

And a good bunch of the client registration stuff

As well as all registry helper stuff

Is that still in there after the registry rebuild?

[spark saffron]

all of it is trivally resolvable

[buoyant pagoda]

someone file an issue on FML about this (removing contextual mod ID)

so we can keep track of this

shadows, I'm looking in your general direction

[spark saffron]

It should probably be deprecated in FML and then removed from forge usages, and then actually removed

also uh

what is

    @SuppressWarnings("unchecked")
    public <T> T extension() {
        return (T)languageExtension;
    }

[buoyant pagoda]

for language providers, as I understand it

[spark saffron]

but its stapled to the ModContainer

so why is it accessible here

[buoyant pagoda]

wdym by stapled?

[spark saffron]

    public void setActiveContainer(final ModContainer container) {
        this.activeContainer = container;
        this.languageExtension = container == null ? null : container.contextExtension.get();
    }

[buoyant pagoda]

also, language providers provide the subclasses of ModContainer

[mental ingot]

Okey I will remove it

[spark saffron]

like it has a separate field for the extension and the container instead of just the container

[mental ingot]

Makes it a normal event then anyway

1 less modbus event

Is a better event in my opinion

[spark saffron]

even though the extension is accessible from the container

[buoyant pagoda]

cpw-ism? perhaps a form of premature optimization? who knows

but we should probably have further discussions on contextual mod id somewhere else, like #neoforge-github

and an issue filed on the FML repo, for tracking our work

[spark saffron]

its the source of FMLJavaModLoadingContext get(), so I guess its just casting helper

but yes yes I'll fiddle with networking here, I just need to ensure magic-static-things die

aiui basically all of the static magic is resolved if the mod loading ctx is passed as a mod ctor arg

and you pass modids around properly

So lets ensure we do that second part here

[buoyant pagoda]

sounds like something Tech would be interested in

(the mod ctor arg part)

[spark saffron]

and I would like to see if we can have a message registration system akin to this, because I feel like this is intuitive https://github.com/Shadows-of-Fire/Placebo/blob/1.20/src/main/java/dev/shadowsoffire/placebo/network/MessageProvider.java

[fair sandal]

FMLJavaModLoadingContext should probably be deprecated

And I want to move to a single event bus in the future which would mean that the active mod context would not be available in events

But there's still some questions to be resolved...

[delicate dawn]

most notably: how to deal with mod dependency ordering wrt firing parallel events

[mental ingot]

Yeah no, I personally feal that that system is not intuitive at all

And it is also not how Mojang does things, so it does not really fit that well

But it is actually pretty close

But I agree that the API currently is to complicated

The particular thing I really don't like about these API styles is that they are inflexible from the get go

Any change to them is a binary break

Want to add optional functionality to it? Binary break

Need to change a type? Binary break

So an interface "provider" based system is not an API that we should generally use

But a Builder style is much more forgiving in those areas

[mental ingot]

@buoyant pagoda , @fair sandal @spark saffron @hasty yew

What do you think of this?

• Gone is the mod id inferance, you can just pass anyone in you like, not even a default, as requested by shadows.
• Gone is the explicit declaration of the flow, it is now inferred from the handlers you set. If you use the short-circuit method to set a single handler, it will treat the payload as bidirectional, however if you only set one direction then it will enfore that packet as bound to the flow that targets that handler. If you set both handlers again to something then the payload is again bidirectional.
• Versioning is now different, it will return an actuall copy of the registrar not poluting the state of the builder anymore. It is now also grouped under a subbuilder, and has a short circuit method as well to simply set the version to some fixed int value. In the case that you do not care for ranges etc

Also classes are gone

The ID is still needed

Because that is what Mojang natively uses to look it up

[fair sandal]

is there a way to declare the handler separately from the packet itself?

[mental ingot]

In what way?

[fair sandal]

in the sense that the handler for a s2c packet will usually reference client-only code, so registering that in a common event is not ideal

[mental ingot]

I asked that 3 days ago, and supposedly it is fine

Because I argued that the whole flow detection is not needed and you should do an if-this-then-static-invoke anyway

Because you always had to register the listener like this

Both event, as well as Simple had this restriction

But I was told it did not matter

[fair sandal]

I think the flow detection is useful

it's not the worst thing, but it would be good to slowly start thinking about separating client-exclusive code properly instead of relying on lazy classloading

[mental ingot]

That is not really possible here

Changing this would basically destroy any hope of allowing people to use any kind of system similar to simpleimpl

Because that is its API

I am willing to discuss seperating it

But that will become very annoying

Because you need the id to line up in two places:

The place were you register the packet

And the place were you register the handler

In principal it is indeed a good idea to do the handling seperately

But then it becomes a, in my opinion, difficult system for newcomers

With a bunch of implicit systems that are only linked via the id

There would be no chance for type safety at compile time

So the removal of the class type

Would need to be reintroduced

[fair sandal]

there are ways around that with the help of extra objects (e.g. PacketType)

[mental ingot]

Yeah but that becomes a tripple chore

[fair sandal]

experienced modders wanting proper side-safety can use a common interface with a single client-exclusive implementation, like mojang does it

[mental ingot]

Yeah but there is no good api for newcomers

Experienced modders already know how to do this

They have been doing this for years

With side-safety

[fair sandal]

either we encourage newcomers to follow a similar pattern and they will need to work with the dispatch interface, or we let newcomers deal with seemingly random crashes when they try to run their code on a dedicated server

[mental ingot]

That can still happen tech

We have nothing, literally nothing, in any of our toolchain that prevents you from using client only code, on the server side. Like you could register a handler that uses client code in the second line

And there is nothing any API we design can do to prevent it

[fair sandal]

I agree, we have nothing at the moment like loom's split source sets for example

[errant summit]

can we change the inner builder in the lambda into multiple methods instead

[mental ingot]

Yeah and this was discussed internally many many many times, and for the foreseeable future we won't have that, cause our patch system was not designed for such a split

And I am not going to design an API for something that might onyl happen 2-3 years down the road, or not at all.......

Especially not if that makes the API much much much harder to use and understand

But I understand were you are comming from

If we had the tech to do split source sets

I would do it in a heartbeat

[mental ingot]

I could, but should I?

Because that would make the API significantly messier

[fair sandal]

yeah that is fine

[mental ingot]

I mean, if we had any idea how to implement it

I might consider it

But I would not even know where to start on the splitting......

It is something I still have on my wishlist for the toolchain though 😄

So we might get it eventually

[errant summit]

I'd like to not have to use an inner builder in a lambda... then we also could make the direction a parameter instead of a builder method

[mental ingot]

It is actually only an inner lambda because I did not want to make 4 methods all virtually the same

I can see the versioning.....

Let me make that public

Cause internally the registrar implementa both of the interfacea

[wary solar]

I reallly hate this kind of builder, it's not intuitive at all

really getting spring web security config vibes from it

[mental ingot]

I already reduced it

But I am adamant that it will stay some form of builder

[errant summit]

that's what it was reminding me of

[mental ingot]

@wary solar #1170668658813575230 message this is what it is right now

[fair sandal]

maybe we could do

registrar.configuration(FrozenRegistrySyncStartPayload.ID, FrozenRegistrySyncStartPayload::new)
        .clientHandler(ClientForgeRegistryHandler.getInstance()::handler);

[mental ingot]

It would break the chainability

And also no

You need something that in the end registers it

[fair sandal]

chainability is not something I particularly care about

but yes the configuration needs to be in a supplier/consumer otherwise users have to manually register

[mental ingot]

I will think about it

Specifically about the handler

But I see no benefit

Other then it being another syntax

If you have a good reason

We can add it though

There are ways of making such a syntax work

[wary solar]

okay that's better

[mental ingot]

Also you don't need the handlers -> callback

If you have the same callback on both sides

Or don't care for side validation checking

[wary solar]

the thing I didn't like about the previous form was like... builders going back and forth between different contexts as you chain the methods? is I guess how to describe it

[fair sandal]

we could have

private static final PacketType<ForgeRegistrySyncStartPayload> TYPE = PacketType.create(ForgeRegistrySyncStartPayload.ID, ForgeRegistrySyncStartPayload::new);

public static void register(RegisterPacketHandlerEvent event) {
    registrar.clientConfiguration(TYPE, ClientForgeRegistryHandler.getInstance()::handle);
}

don't you need sort of a packet type to post the packets in the first place?

[mental ingot]

No

[mental ingot]

Nah an instance of the payload suffices

[fair sandal]

ah yeah I just saw CustomPacketPayload lol

thank you mojang

unfortunately their handling is kinda shit

[mental ingot]

XD

I fixed that already 😄

But we will likely use a custom interface

[fair sandal]

I like the idea of reusing mojang's own interface 🤔

[mental ingot]

I already adapted the read logic

So that you are aware of what the negotiated version is of the channel

But write is still open

[fair sandal]

if we make a custom interface we can use a PacketType 🤔

[mental ingot]

Why?!?

I would just make it a super type of the CustomPacketPayload

[fair sandal]

the idea is that the PacketType contains both the ID and a generic with the correct packet class

[mental ingot]

What is the advantage?!?

[fair sandal]

so you'd go from

public record BrandPayload(String brand) implements CustomPacketPayload {
   public static final ResourceLocation ID = new ResourceLocation("brand");

   public BrandPayload(FriendlyByteBuf p_296001_) {
      this(p_296001_.readUtf());
   }

   @Override
   public void write(FriendlyByteBuf p_294892_) {
      p_294892_.writeUtf(this.brand);
   }

   @Override
   public ResourceLocation id() {
      return ID;
   }
}

to

public record BrandPayload(String brand) implements IPacket {
   public static final PacketType<BrandPayload> TYPE = PacketType.create(new ResourceLocation("brand"));

   public BrandPayload(FriendlyByteBuf p_296001_) {
      this(p_296001_.readUtf());
   }

   @Override
   public void write(FriendlyByteBuf p_294892_) {
      p_294892_.writeUtf(this.brand);
   }

   @Override
   public PacketType<?> type() {
      return TYPE;
   }
}

but now you can register with

registrar.clientConfigurationHandler(BrandPayload.TYPE, <handler here>);

and get rid of most of the builder cruft

[mental ingot]

Like I really only need to create a write(FriendlyByteBuf, ChannelInformation)

And that is it

[mental ingot]

So that PacketType makes 0 sense!

[fair sandal]

sorry I forgot the generic

[mental ingot]

If it is just a damn resourcelocation internally anyway, not even generic, then it can stay a resource location

[fair sandal]

edited now

[mental ingot]

Still no use for that Type

If you have a usecase for that

Then sure

[fair sandal]

the point is that the handler receives the right type in the registrar call

[mental ingot]

But given that the serializer on Mojangs end only works with resourcelocations it is a mood point

[fair sandal]

registrar.clientConfigurationHandler(BrandPayload.TYPE, <handler here>); here we get packet type information

[mental ingot]

However then: <T extends IForgeCustomPayload> clientConfigurationHandler(ResourceLocation, IConfigurationPayloadHandler<T> handler) gets you the same security at compile and runtime, and you don't have the extra overhead

So why do you need the extra type?

The reason i am asking this, is technically the only thing that links a handler to a given packet of a given type, is the resloc

Because that is what is used as the map key

[fair sandal]

the extra type gives a bit more security

[mental ingot]

And the values are generic consumers without the type information

[fair sandal]

not perfect, but assuming you don't get the class wrong when creating the PacketType you are fine

[mental ingot]

Not really

The packet still has to go acress the network

[fair sandal]

we could also request a class:

public static final PacketType<MyPacket> TYPE = PacketType.create(MyPacket.class, resloc);

then we can have a debug check when sending that the class of the packet matches the class inside the type

[mental ingot]

I did that

[fair sandal]

yeah I know, that can also cause problems... but assuming the client and server have the same version of the mod it would be basically impossible to make a mistake

[mental ingot]

But people complained that they needed to provide resloc and class

See shadows above

[fair sandal]

I mean we could give them PacketType.createUnchecked(resloc) I suppose

[mental ingot]

So I ripped that out

[mental ingot]

Hell no

Either we do something right

Or we don't do it at all

[fair sandal]

IMO: we do it right for the 99% use case and if some people want to bypass our checks because they think they know better we can give them an escape hatch

where is that example that you linked a few times? I am tired of rewriting what I see on an image? 😛

[mental ingot]

NetworkInitialization

[fair sandal]

ok found it

[mental ingot]

I like the idea of the Type

[fair sandal]

so the point of having a packet type is to separate the handler from the definition of the packet:

        registrar
                .configuration(
                        // these two are the definition of the packet:
                        FrozenRegistrySyncStartPayload.ID,
                        FrozenRegistrySyncStartPayload::new,
                        // these are the handlers:
                        handlers -> handlers.client(ClientForgeRegistryHandler.getInstance()::handle)
                )

and then we can get rid of the builder, and have a single call per handler because we have a nice abstraction for the "definition" of the packet

[mental ingot]

You would need a builder for the PacketType

[fair sandal]

for the network version, right?

[mental ingot]

That

Whether or not it is optional

And you need the packetflow definition

Because those need to be available as values on both sides

[fair sandal]

do you need info on the packetflow even if no handler is registered on your side?

actually you need the packet type to be registered for version negociation

[mental ingot]

Yes

[fair sandal]

we can have a DR for packet types 🤔

(not an actual DR, but same idea)

[mental ingot]

You are starting to overcomplicate things

[fair sandal]

I'm just moving the complexity around 😄

[mental ingot]

Not really

[fair sandal]

you wouldn't need the registrar anymore for example if that info is stored in the packet type - the methods would be on the register event directly

private static final PacketRegister PACKETS = PacketRegister.builder("mymod").version(42).create();

public static PacketType<Hello> HELLO = PACKETS.register("hello", Hello::new);
public static PacketType<Response> RESPONSE = PACKETS.register("response", Response::new);

PACKETS.register(modEventBus);

for the handler registration you'd have the choice between an event

public static void registerHandlers(RegisterPacketHandlersEvent event) {
    event.handleConfigurationClient(HELLO, <handler>);
}

or maybe even just registering directly in the PR? (this last part I'm not sure about)

public static PacketType<Hello> HELLO = PACKETS.register("hello", Hello::new)
    .handleConfigurationClient(<handler>);

[mental ingot]

But that would result in you need to event handlers

ANd you need a registrar to deal with the packets.....

[errant summit]

this looks like the simple channel builder

[mental ingot]

Yet it feels very complicated

[fair sandal]

I modelled it after DR but maybe

[mental ingot]

That could be problematic due to class loading reasons 😄

[fair sandal]

yeah... 😅

[mental ingot]

I always found that problematic with DR

[fair sandal]

you mean that you load the class that the DR is declared in if you reference one of the fields?

with a client-only RegisterPacketHandlersEvent handler you solve the problem of separating client-side code at least

[mental ingot]

Yeah yet you also need that on the server

It is a common event!

Well you likely ment a client-only handler

[fair sandal]

yeah

[mental ingot]

For that specific event

[fair sandal]

yeah I meant a handler in the mod

[mental ingot]

It is a good idea.....

But I still don't see the reason for that complexity

But let me implement this 😄

And see how I feel like it when we are in code

Because it splits a lot of data

Let me first do the write channel information stuff

[fair sandal]

no rush 😄

[brave rose]

Is the ID really necessary? When I made simple networking for Create Fabric using Fabric's networking API and inspired by SimpleChannel, I was able to avoid having the user pass any sort of ID.

[fair sandal]

it's not strictly necessary, but I don't see a good reason to get rid of it

[mental ingot]

It is needed

[brave rose]

I don't know if that's applicable here, but the way I did it there is automatically assign an int ID to each packet and increment it for each new packet

[mental ingot]

I want to get rid of shit that happens just because people are annoyed about thing for a good name

The ID shows up in the error screen

The ID is used as a descriminator by the packet system

It is needed

[fair sandal]

pepper what you have is just a single id + an int prefix to know what to serialize/deserialize with

[mental ingot]

I am not going to discuss the need, or no need, for a nice human readable and identifyable id

[fair sandal]

we just remove the int prefix and move to one id per packet like vanilla and fabric already have

[mental ingot]

Yeah

[brave rose]

Indeed the int assignment won't work in that case

Ok, I understand now

In case it's important, Create doesn't care about a builder pattern either. It uses an enum to store all the packet type data like serializer and deserializer and then it registers everything by iterating over the enum fields. Currently I see no reason to change this.

[fair sandal]

AE2 also has such an enum

that should still be possible

[mental ingot]

Yeah that is completly still possible

[mental ingot]

@hereThe current API design supports passing a "range" of versions that you support. However, more and more this feels very clunky. I have to create custom payload interfaces and extra methods to deal with reading and writing the packets. All of this feels very clunky to support versioned payloads.

What I am suggesting is the following, you can still pass a version to the system, but this will be the only version you support. So if the version on the client and server don't line up, that is bad news and negotiation will fail. Is that okey?

It removes the need for the custom IO code, the patches to support that, reduces complexity on the modders since they don't have to care for that, reduces maintanance and makes the API easier to use.

Second I am implementing a PayloadBuilder style system as tech shows above. It won't support his precious "split"-registration, my reason for that is that we currently support that no where. However I think it is a good piece of API design to have to future proof it incase we do ever add it.

It would be nice if you guys could leave some feedback thanks.

[fervent token]

imo the important bit is making it so you can require specific versions/that it matches on the server and the client. Personally I don't think there is that many people who actually support more than one remote protocol version of their own mod (yes supporting their mod not being present, but not supporting two versions of their own mod)

so I think that sounds relatively reasonable

[wary solar]

What I am suggesting is the following, you can still pass a version to the system, but this will be the only version you support. So if the version on the client and server don't line up, that is bad news and negotiation will fail. Is that okey?
I'm not opposed to it

[buoyant pagoda]

so Minecraft network protocol version style?

(though less restrictive, since MC bumps that network protocol on each version bump, and mods are unlikely to change their network protocol that often)

[wary solar]

I guess if you want to preserve backwards compat when you update your network apis then you just don't bump your netcode version

[prisma garden]

sounds good to me

[mental ingot]

I will leave this open untill tomorrow morning when I code this

So it can collect some feedback from people around the world

[indigo hazel]

Sounds reasonable to me 👍

[timid fern]

this is what I've done so far

[buoyant pagoda]

wait. you guys bump your netcode version? /jk

[timid fern]

I did, from 1 to 2 because of the new custom particle stuff I did in my mod

[fervent token]

Yes by bumping my modid version and having it use the version from my mod container (though in other smaller mods I do really bump it)

[iron ivy]

I'm very out of the loop on the networking changes, but as long as it lets me have a ByteBuf of data either end and I can do my own thing, I don't really care. It automatically using mod versions as protocol versions is a mistake though.

[fervent token]

That isn’t what Orion said

[delicate dawn]

I actually don't. The majority of my mod channels are still 1.0

[mental ingot]

Pushed another update

All payloads for the configuration phase are now implemented.

The following features are missing:

• Entity Spawn with Data payload
• Open Container payload
• Tool Tier Sorting Networking
• Server Status packet

This push alos removes the range support, and makes the version again a string

[mental ingot]

Also @fair sandal I figured out a way to do the split registration you wanted

It should be possible to implement it

If I refactor the event management a bit

And create the concept of a "PacketRegistrationToken"

It will mean there will be three events fired.

But it should be fine

[fair sandal]

Ok let's see what that looks like

[buoyant pagoda]

draft PR when

[buoyant pagoda]

the custom client entity factory, eh?

i hear it's a screm of a time

[mental ingot]

The custom entity packet shit

Yeah I am reimplementing it completely from scratch

Because the way it was implemented with a dupe of the packet in forge is just terrible AF

I just made it a compound tag for now

And patched it into the actual fucking packet

[buoyant pagoda]

that sounds like a recipe for NF<->vanilla incompatibility

[mental ingot]

Hmm

Shit

You are right

[buoyant pagoda]

after all, it was the tacked-on data to one of the packets which causes NF<->vanilla incompatibility at the moment

[mental ingot]

But in practive the old way is also extremely terrible

Hmm

You are right

That won't work

[buoyant pagoda]

we are practically barred from modifying any of the vanilla packets, in fear of vanilla incompatibility

[mental ingot]

Yeah

[buoyant pagoda]

we get by with the custom payload packet (which is why we duplicated the entity spawn packet, iirc), and also with the flexible JSON data used in the status ping packets iirc

[mental ingot]

But this old system is not going back in

The packet was just completely reimplement

And not even that

It was reimplemented using old code

Which no longer followed the same behaviour

[errant summit]

yeet it and open a regression issue for later (this is too important to be held up by a single packet)

[mental ingot]

Okey

Does anybody know what the Custom OpenContainer packet is for?

Functionality wise?

[buoyant pagoda]

NetworkHooks.openGui, iirc

for passing a FriendlyByteBuf which could have some custom data needed for the container to open

[indigo hazel]

Similar thing to the entity spawn packet: adding additional data to the screen opening process (i.e. a BE's position)

[mental ingot]

Okey so I am going to yeet both

With the new networking style

People should make dedicated packets for that

[buoyant pagoda]

wonder if we can WAIFU-search the amount of uses of the NetworkHooks.openGui overload with the custom data buffer

[dense kite]

that'd be a huge amount (since it is the recommended way of doing it ATM)

[indigo hazel]

Neo provides the infrastructure for both of those to work (IEntityAdditionalSpawnData and IContainerFactory respectively), so it should ideally also provide the packets. Though I agree that they shouldn't hold up the rework itself

For the entity stuff it might be possible to abuse the bundle packet system, which is currently almost (if not entirely) exclusively used by entity spawn packets

[mental ingot]

But realistically, I think that neither of these factories should exist

[mental ingot]

That is actually something I need to research as to how that works with custom payload

[errant summit]

I use it to send a blockpos to get the BE on the client side

[buoyant pagoda]

if so, removing it would mean there's a gap between the rework and some future PR reimplementing it in which mods are unlikely to switch over, due to the missing gap of functionality

if it's as widely used as we think it is, then it is essentially a blocker

[mental ingot]

My problem is that the whole "dynamic way via friendly byte buf data" is just a very bad pattern

[buoyant pagoda]

it likely is, but nevertheless, removing it outright is a huge obstacle for porting over to NF
moreso than the registry rework, which is tending towards "switch out some references, cleanup some uses" (as I understand it) for the common mod

[mental ingot]

I might do some research to use something like Bundle Packets

And something like a raw payload packet

Which just contains that particular subset

So it spawns the entity

[errant summit]

we should not be having a good/bad pattern discussion for this right now... if it is possible implement it if it is not possible yeet it and make a regression issue (the important word here is possible)

[mental ingot]

Up to some degree it is

Up to some degree it is not

It is like in between

I can take a hammer and make it work

Using custom payloads

And processors

And I can make it work

But it won't be pretty

Or very maintainable

[errant summit]

don't we currently use a custom packet too?

[mental ingot]

Yeah but its handler is not very trivial

Well the entity spawn one

THe gui one is actually pretty trivial

But if I change one to work differently

Then I will change both

Cause they use the exact same patterns

[errant summit]

yea implement the gui one and yeet the entity one with regression to fix later

[mental ingot]

I might have an idea

[iron ivy]

I'm confused. Whats so complicated about giving mods a byte buf to read/write data to when opening guis?

[mental ingot]

The gui stuff is not complicated

It is a simple buffer

In reality though it is not a great pattern

They should just have their own record-payload and send that

But I figure out a way around it

[iron ivy]

ew

no

Record payloads are dumb

[indigo hazel]

Sending custom data after screen opening through the standard packet is a recipe for disaster because it means that relevant data for even opening the screen in the first place is missing entirely

[delicate dawn]

there's some argument to be made that if your container menu requires additional data to be constructed, it's probably designed wrongly

[buoyant pagoda]

custom payload packet before the screen opening packet?

[errant summit]

well all payloads are record-able now

[delicate dawn]

but there's so many use cases in modding that assuming everyone can write menus like vanilla does is silly

[iron ivy]

I think Orion was meaning instead of having arbitrary buffer along side the open, you send some record data class along side

[mental ingot]

I thought about that covers

But I made it different

It is a neoforge packet

Specifically for that purpose

[iron ivy]

As long as Forge still just lets me talk to a buffer directly without having to make records for shit..

[buoyant pagoda]

this reminds me: reminder about draft PR

[mental ingot]

It is a compound tag

Not a buffer

The one thing I won't be able to support is the EntityType#customEntityFactory

That actually takes the whole packet

[errant summit]

but you can put arbitrary data into a buffer without overhead (most common usecase is sending a blockpos along)

[mental ingot]

Buffers are terribly for memory management

[iron ivy]

?????

[mental ingot]

Especially when using a local in memory connection that is exposed to LAN

I patched 4 of those read-on-free issues in the past

[errant summit]

but best for packet size management

[mental ingot]

I mean I could make it a byte[]

And then you would need to turn it yourself into a buffer

And free the buffer once done

.....

[iron ivy]

I have no idea what your talking about

[errant summit]

if it is still exposed as a FriendlyByteBuf that is fine

[mental ingot]

No it is not!

That is the point

[iron ivy]

what

[mental ingot]

Exposing any raw ByteBuf in a packet

Is a big fucking no no

Because there is no way to discard that buffer properly

On write

[errant summit]

FriendlyByteBuf is what has the methods to write stuff efficiently tho

[iron ivy]

If your not giving us access to raw packet data then im going to be mixing my own shit in

[mental ingot]

You have access to read it

Just fine

And the buffer you are given

[iron ivy]

it was always an option that mods could register an event based network channel and just deal with raw byes either side

[mental ingot]

Is automatically discarded by vanilla after you read from it

That buffer is not the buffer I am talking baout

I am talking about that intermediary buffer that holds the custom entity and gui data

That is a big no no

[iron ivy]

OH

okay

[mental ingot]

Especially on lan connection

[errant summit]

we need a mechanism to send stuff like the hand enum or a blockpos to the client when opening the container which is passed to the menu constructor

[mental ingot]

Cause those intermediary ones are not freed on send

And just linger around for ever

And cause memory leaks

[iron ivy]

the approach that openGui currently takes with a lambda is the best approach

just give the mod a buffer to write extra data to

done

they can write it as efficiently as they want

and unpooled buffers should absolutely be cleaned up

[mental ingot]

A lambda for sure could work

[iron ivy]

if we don't pass paranid netty detection, then we are doing something wrong

[mental ingot]

Because then I can construct the buffer

Have them write it

Take the byte array

And then destroy the buffer

[iron ivy]

Yes, thats the current approach to openGui

[mental ingot]

It is not for the entity shit

That just takes a raw buffer

And never discards it

As far as I can see

[iron ivy]

entity packets should be a IPacket<?> getSpawnPacket

with some fancy builder to help mods make one

[mental ingot]

It is not for the custom spawn shit

It is PlayMessages.SpawnEntity

[indigo hazel]

The old implementation uses FriendlyByteBuf extraData = new FriendlyByteBuf(Unpooled.buffer()); for the buffer handed to the lambda, which is a heap buffer. If that is not cleaned up by the GC then the world is gonna end

[iron ivy]

It is

[mental ingot]

There are some very funny corner cases with this in LAN exposed worlds with more then one player

[iron ivy]

what custom spawn shit?

[mental ingot]

See the packet I just send you

PlayMessages.SpawnEntity

[indigo hazel]

IEntityAdditionalSpawnData

[mental ingot]

That holds a raw FriendlyByteBuf

[iron ivy]

I dont have a 1.20.2 env anywhere

[mental ingot]

It is 20.1

20.2 does not have that packet

[iron ivy]

then i have an env

[mental ingot]

Cause that is what I am debating on how to implement it

My current idea

Is to create a packet that takes a byte[] or compound tag

[iron ivy]

Okay, that should be a tag

[mental ingot]

And then bundle wrap that

So that they are send and processed to gether

[errant summit]

this is how the extra data is written in the gui one

[iron ivy]

or provide a withCustomData(Consumer<FriendlyByteBuf> cons) functiojn on SpawnEntity

[mental ingot]

Is that in the write?

[iron ivy]

the same as guis

[indigo hazel]

Yes

[mental ingot]

Yeah it is in the write

Yeah that is fine

Since it is constructing a new buffer for each call to write

Which is mightyly important

I think I got this figured out

Allthough I will open a custom RFC for the regression on the spawn data payload to the entity type constructor

[indigo hazel]

The entity crap does almost the exact same thing:

if (msg.entity instanceof IEntityAdditionalSpawnData entityAdditionalSpawnData) {
    final FriendlyByteBuf spawnDataBuffer = new FriendlyByteBuf(Unpooled.buffer());
    entityAdditionalSpawnData.writeSpawnData(spawnDataBuffer);
    buf.writeVarInt(spawnDataBuffer.readableBytes());
    buf.writeBytes(spawnDataBuffer);
    spawnDataBuffer.release();
} else {
    buf.writeVarInt(0);
}

[errant summit]

it's the 1.19 impl in NetworkHooks#openScreen but current one is just formatted differently

[mental ingot]

My copy has this:

Ah it is for fucking readonly!

Jesus mother fucking christ

[indigo hazel]

That field is only used for reading. The encoding snippet I showed happens in encode()

[iron ivy]

Yes, but that buffer is only ever used internally inside encode and handke

[mental ingot]

Nevermind

Ignore my bullshit

[iron ivy]

._.

[mental ingot]

I misread this an hour ago

And never realised

[indigo hazel]

Only decode() (indirectly through a constructor) and handle(), encode() never touches that field

[iron ivy]

yeah, semantics

:p

[indigo hazel]

😄

[mental ingot]

I can reimplement htis

[iron ivy]

Also orion, whilst your touching that packet, can you make a sister pakcet which does not truncate the yaw/pitch and velocity?

:D

@dreamy nexus would like you very much for that

[mental ingot]

That section?

[iron ivy]

DE's arrows currently fire off into narnia when they are going at max speed due to that truncating

[iron ivy]

Yeah,

[mental ingot]

So basically remove the floors and the clamps?

[iron ivy]

we need a variant that mods can opt into using that doesn't try and compact those doubles down, and just sends them raw

[mental ingot]

That should just be the default behaviour

[iron ivy]

pich/yaw being truncated to 1/4th their size is perfectly fine for regular entities, velocity as well

its a lot of extra data, I don't recommend making it the default. Should be a secondary sister packet that mods can opt to using, or it switches to automatically

[indigo hazel]

The field name in ClientboundAddEntityPacket says it all: private static final double MAGICAL_QUANTIZATION = 8000.0;

[iron ivy]

heh

it is magical

it makes arrows go sideways

[mental ingot]

I will think about this

[iron ivy]

So, why is the vanilla packet not sufficient for regular modded entities?

Or it is and that packet is opt in already?

[mental ingot]

That packet "SpawnEntity" is already opt in

[iron ivy]

Oh

[mental ingot]

If you do not need the additional data

Then you use the normal one

Which is why I also hate its current implementation

Because it uses old spawn code

I can't actually see any functional difference in the end result

But the code is really not anymore what it was before

[indigo hazel]

Looking at the call site of Entity#getAddEntityPacket() in ServerEntity#sendPairingData(): we have the entity, we have a consumer to add arbitrary Packets to the bundle packet and we have the target player. This should allow us to do something like if (!player.isVanillaConnection() && entity instanceof IEntityAdditionalSpawnData) { addOurPacket(); } (we probably don't even need to bother with the vanilla connection check, to be honest, since it'd be a custom entity anyway)

[mental ingot]

Probablyy

@indigo hazel Can you do me a favor and make an RFC Ticket for this

I am just straight porting this over

[indigo hazel]

Can do 👍

[mental ingot]

But we for sure as hell need to rethink this whole area of the API

[fair sandal]

The GUI extra data is really essential IMO

Almost every modded GUI uses some form of it

For entities it is a similar situation I think - it's a low impact hook that is very useful; but it is indeed worth researching an implementation that shares as much code as possible with vanilla 🙂

[brave rose]

It is possible to use BundlePackets to send extra data in some cases

[mental ingot]

Yeah

But not with the api design we currently have

So we will need to refactor that

Which is why I asked xfact to open up an rfc ticket

[indigo hazel]

In the entity case that would be possible as I suggested above since that already uses the bundle system. For the menu stuff we'd have to experiment whether vanilla accepts the container packet in a bundle packet (it technically should) but the patch surface would definitely be larger since vanilla doesn't use bundling there

[mental ingot]

Given that these are custom entry points for modders only, there is no patch surface on this

[indigo hazel]

For the entity case you'd need a patch in ServerEntity#sendPairingData(), there's no way around that. For the menu case you could probably do the bundling fully in Neo code, that's true

[mental ingot]

The entity spawn packet we have now is only used on descretion by the modder

It is on to them to send that packet

Instead of the vanilla one

Which is how it should stay

This should not be the default system

But only something for really custom entities

[indigo hazel]

It wouldn't be, the modder opts into the additional spawn data packet by implementing IEntityAdditionalSpawnData on their entity. This is already the case now, with the difference being that you currently need two places to opt into it instead of only one if we add it to the packet bundle

[mental ingot]

You are right

In practice that again indicates that this system is not great

[indigo hazel]

To be fair, this system has existed for ages and packet bundling was only introduced in 1.19.4, IIRC to fix desyncs with delayed entity data packets

[mental ingot]

Yeah

For sure

[indigo hazel]

There ya go: https://github.com/neoforged/NeoForge/issues/275

[errant summit]

orion can we get a draft PR so we can have a look at the changes

[velvet whale]

[mental ingot]

I don't see a reason why? I am not ready, the code is completely untested, the api is completely untested, for those things that I consider important I ask in here for feedback, and it is completely visible in the repo under the feature branch.

There is nothing preventing you from looking at it.

In practice:

• I have 36 compile errors,

• I need to solve the Registry Sync config tasks, with respects to vanilla clients (It requires an ACK at the moment from the client to continue, which won't work if the connection is a client). I am considering two solutions: Either add an indicator to the configuration task colection event that indicates if the connection is to a vanilla client, and skip it, or remove the ACK.

• I really would like to have an ACK, because it makes the whole dealio with threading on the client side eaiser (AKA no loading the registry payload from a network thread, like I have it implemented now) so an indicator as to what kind of connection it needs to be, will likely be needed.

• I need to fix the OpenGUI networking code.

• I need to fix the status packet / split it into 2 more packets that get bounced, to solve the json encoding crap.

• Testing, lots of testing. Especially with registry sync and such.

That is the state

All of that is visible in the branch

I suggest starting here https://github.com/neoforged/NeoForge/blob/feature/1.20.x/networking/src/main/java/net/neoforged/neoforge/network/NetworkInitialization.java#L25 if you want to research the implementations

With respect to a summary of the changes.

• SimpleChannel and EventChannel have been removed.
• Replaced with a single event to which you can register your payload ID, reader, and handler(s). The payload needs to implement CustomPacketPayload, which then gives each payload also an id retrieval method (has to be the same as the one you use to register), and a writer method. Both read and write are FBB based.
• Moved all of forges and FML networking to the configuration phase. Reconfiguration will need to be tested at a later date.
• Created an event that allows Forge and modders to add config tasks. Fired on the mod bus to respect mod dependency order (aka you are guaranteed that your dependency has completed its client configuration tasks before yours begin).
• Added a bunch of Netty Attributes to store connection information straight on the connection object. Techncially they are all internal APIs, but if people need access to underlying raw data of each connection (for example the available channels, or if it is a vanilla connection), when they only have the Netty Connection data then they can use those attributes, without having to do reflection or hacky stuff.
• PacketDistributor has been reimplemented so that you can use it with the new system
• All listeners and PacketDistributor gained methods to simply send a custom payload, without the need to wrap them in to the specific packets, The server side listener will wrap it in a clientbound packet and the client listener in a server bound packet automatically

Note:

• There are still imports in the Patches.
• Stuff is unformatted,
• Stuff is incomplete,
• The API is not documented yet well enough.

So please bare with me while I catch up on that

[mental ingot]

@errant summit and @velvet whale Does that help?

[spark saffron]

why is this a lambda https://github.com/neoforged/NeoForge/blob/feature/1.20.x/networking/src/main/java/net/neoforged/neoforge/network/NetworkInitialization.java#L33

[velvet whale]

I still don't see a reason not to open a draft. it makes it easier to track progress, view the changes, leave notes for future reference or otherwise point possible big problems and that's what draft PRs are meant for. unfinished work in progress

But what if you want to signal that a pull request is just the start of the conversation and your code isn’t in any state to be judged? Perhaps the code is for a hackathon project. You have no intention of ever merging it, but you’d still like people to check it out locally and give you feedback. Or perhaps you’ve opened a pull request without any code at all in order to get the discussion started.

[spark saffron]

cpw also specifically requested things be done through pr when possible

[mental ingot]

I always said that I would open a draft pr when all errors are solved

And I stand by that

When I have everything ported, but before I start finishing and testing

That is when I make my draft prs

[mental ingot]

Because it is a client side only handler, the lecker flow is derived from it.

There is a variant of that method that does not use a handler

But in that case the packet can be send bidirectional

[spark saffron]

why not just have the direction be provided on registration and only have one handler concept

[mental ingot]

Because there are many mods out there, forge included which have bidirectional packets, and that simply does not work, because the is can only be registered once

So there has to be a distinction some where

I had an explicit method on the registrar

Like the version

But people did not like that

So this is what I promised as an alternative

I think it is fine

The lambda to configure the handler stack, and flow control, makes this a very flexible design with little to no overhead

[spark saffron]

I feel like bidirectional packets are ripe for design flaws

[mental ingot]

Yet they are used, and this api is fine. Unless you have something that is fundamentally wrong with it?

[spark saffron]

"they are used" is meaningless, they are used because they exist

[mental ingot]

Does not mean that we need to engineer the api in such a way that prevents then from being used

The api is safe and well designed

[spark saffron]

the problem now is that bidi packets can be maliciously sent to the server and crash it
most consumers are unaware that this is even an issue, because the packet direction is not enforced and defaults to bidi

[mental ingot]

I see no reading why we should remove that ability

[spark saffron]

I guess it doesn't particularly matter if it exists at all, but using it should require that the packet is explicitly marked bidi

What packets are we sending both directions?

[mental ingot]

Splitter mostly

At least it is/was designed to

I am pretty sure it does not happen in practice

[mental ingot]

Simple channel did not have this either

Nor did event for that matter

[spark saffron]

simplechannel's default registration was bidi, unless you set a network direction

[mental ingot]

Yes

This uses the same behaviour

[spark saffron]

thus it was implicitly bidi, which is bad

because people will unintentionally make things bidi

[mental ingot]

Imho we could remove them method from the registrar

That makes the implicit possible

[spark saffron]

like I'd wager more than 50% of mod packets are unintentionally bidi at the moment

[indigo hazel]

Probably more

[spark saffron]

Yeah probably

tutorials glance over it or ignore it

[mental ingot]

Then let's do that

Let's remove the implicit call on the registrar

And add a bidi call on the handler stack builder

That forces everybody to be explicit about what they want

[spark saffron]

should be fine

[mental ingot]

I will do that tomorrow

I am now of to bed

[indigo hazel]

Sounds reasonable. Though, then we should ask ourselfs whether the nested builder is even needed at that point

[mental ingot]

It is, it is the cleanest and simplest approach under the constraint that an id can only be registered once and adding 6 different methods to the registrar which all do something slightly different is confusing

[indigo hazel]

Fair enough

[mental ingot]

Given that there is no real overhead here (it is only called once) I value the cleaner api, the ease of use and the better clarity over the extra memory of the lambda

[iron ivy]

Can we please retain the ability for mods to register raw channels? I do not want to use the packet api design that Forge has historically had, lots of modders don't like it. This is what i was trying to ask to be retained earlier.

[buoyant pagoda]

(a) a draft PR makes the refactor visible to the rest of the community, as opposed to a hidden-in-plain-sight branch
(b) review tools are better in a PR -- PR comments, PR review threads, PR change-requesting/approving reviews
(b.1) the whole PR's changes can be seen all at once, with access to e.g., commenting on the PR
(c) discussion of the PR's refactors are preserved in GH for posterity, rather than the volatile nature of Discord conversations/threads
(d) multiple maintainers have asked you to create a draft PR for the networking API refactor

as long as a draft PR isn't opened yet, I am refraining from reviewing that branch so I can gather all of my comments all at once in a PR review, rather than scattered about here in Discord or perhaps even in commit comments

[fervent token]

adding to b it lets reviewers actually mark files as viewed

[buoyant pagoda]

yea,
(b.2) being able to see which files changed since last review (and seeing those changes in the same interface)

[dim wagon]

Havent been following this too closely

But itd be nice if there was a clean way to do a callback or a boolean check or something to do some stuff if the client/server version of the mod isnt present via the channel

Atm you kinda have to do a hacky check in the revision checker, which isnt very nice or approachable

[pliant stream]

It sounds like the new system amounts to "every packet you register is a raw channel now", unless i'm badly mistaken about some aspect of how it's been described.

[iron ivy]

the new system from how its been described, sounds like simple channel has been made the only implementation.

[mental ingot]

Well currently both are gone. The new one is a hybrid

There are events fired during setup and connecting phases

But the handling of the buffers happens via a direct call back

[iron ivy]

Its very hard to follow along with this, the design, the things that are being changed, etc, without a PR. Once there is a PR i can look at code, i will provide further coments

[mental ingot]

It is a combination of a string and an boolean that indicates optionalness. So the lambda extension callback is gone

[dim wagon]

it's not a matter of optional-ness

It's a matter of being notified when the optional connection is made

so I can do stuff

[mental ingot]

Basically if your channel is marked as optional it is allowed to not be there (for example during a connection with vanilla clients) but if it is there then it's version has to line up

[dim wagon]

yes that's not what I'm asking for

[mental ingot]

I will be opening a pr

Later today

So you can look

[iron ivy]

For an optional channel, you wish to know when the other side exists?

[dim wagon]

yes

precisely

without having to be hacky

[mental ingot]

Why?

[dim wagon]

why not?

[mental ingot]

It either exists or it does not

[dim wagon]

that's like asking why should I win the lottery

you either win or you don't

[iron ivy]

So the client/server side knows when the other component is there and can enable/disable features

[dim wagon]

wtf does that even mean

^^^^^^

[mental ingot]

Ahh oke

[dim wagon]

the whole point of it being optional is things can optionally happen

I'd like to optionally make the optional things happen if the optional channel is there

[mental ingot]

Yeah

That can work

[iron ivy]

super old example with NEI: Client could know when the server exists and use that to spawn items instead of commands

(NEI is dead, but practical use case)

[mental ingot]

Yeah

[dim wagon]

many mods have functions that only work at all if both sides exist

[mental ingot]

I can add that

[dim wagon]

so knowing if it exists is critical

and having that be easy to do is a very useful funcitonality

atm you have to do a stupid hacky workaround

[mental ingot]

Would be an event on the server side only to indicate all available channels on an incoming connection

Be sufficient

[dim wagon]

or just a registerable callback on the channel

when it successfully handshakes

[mental ingot]

Or do you want it fired on both ends

Ehm

[mental ingot]

If I still had my builders for the registrar I would offer that

But people hated that api design early on

[iron ivy]

Both ends

[dim wagon]

like the forge network builders?

yeah it's pretty ick to work with idk

[iron ivy]

I assume you allocate the mod some channel object that they hold onto

it can just be some properties stored on there for easy use

[dim wagon]

callback ideal

but even just a boolean method check would be a good bare minimum

so at client connect I can do a thing

[mental ingot]

You won't get a boolean for sure. At most you can get a consumer of connection

Well maybe

I don't know

[dim wagon]

it can't be that problematic to inject a callback after the channel handshake

[mental ingot]

What I really do not want is people then abusing that callback to do their own versioning checks and disconnecting the client

Or disconnecting from the swrver

[dim wagon]

it's not your job to police mod devs

[iron ivy]

Why are mods not allowed to abort the connection?

[mental ingot]

Up to a certain degree it is. Forge contains specific code to handle the case of an incompatible client or server, and sets standardized properties on the channel connection to handle such cases. Additionally it has its own custom indication screen that is used when a disconnect happens because of channel incompatibility reasons. All of which would be bypassed.

So yes up to a certain degree we have to design a system that works for your requirements, but so that all modders can work nicely together and in such a way that the user and player has a good experience

[dim wagon]

designing a system that works is not the same as designing a system that's hostile to people so they use it your way

[mental ingot]

It is not hostile at all....

[iron ivy]

Mods will abort the connection. Regardless. So embrace it, give them an exception to throw inside the callback to abort the connection. Forge can then roll that into the fancy display: Mod X aborted the connection due to: YZ

[granite mortar]

i'd suggest just catching all exceptions and saying that any exception thrown would abort the connection, but an instance of a specific exception might get you a nicer error message

[mental ingot]

That is an idea is was designing for sure

The point is however that giving a modder the raw connection object, allows them to call disconnect

And bypasses all systems

It just sends the disconnect packet

And then closes the connection

[granite mortar]

Some modder's probably going to do that anyway, just to be different

[dim wagon]

and if they get found doing it in the community they'll get ostracised

like I said, it's not your job to police the users

forge doing that has historically been a major sticking point for mod devs

cus it almost always involves fucking over legitimate devs

cut that shit out

[mental ingot]

I don't agree, there are many mods that might depend on certain operations completing, because forge guarntees it if a mod does not do something stupid

[granite mortar]

I can see wrapping the connection in some sort of interface, but anything more than that is just... not really worth it

[dim wagon]

then they can take it up with the mod that fucks them over

[mental ingot]

In my opinion it is part of forges primary operation to ensure that modders don't fuck each other over

[dim wagon]

in the meantime, you're screwing over every single mod that might have a need for that

which happens all th etime

it's one of the bigger complaints forge gets in general

[mental ingot]

A need for what!?!?! To trigger a random disconnect, instead of throwing the proper exception during a callback?

[dim wagon]

stop thinking so narrowmindedly

[mental ingot]

I don't

[dim wagon]

incorrect

just because they can do that doesn't mean it's the only possible use that channel could have

[mental ingot]

I think we, as in forge, should provide proper APIs to do things

[dim wagon]

I agree

and you, as forge, should make those API's accessible

rather than trying to arbitrarily police what shit devs might do

take the FluidInteractionRegistry, for example

[mental ingot]

And we should guard our and vanillas API endpoints against misuse, which could put it in an invalid state

[dim wagon]

forge made that, then decided that 'no dev should ever need to access it'

which then screws over any dev that might want to check if a fluidtype has an interaction

[mental ingot]

Yeah, but this is not that

[dim wagon]

by making it FUCKING PRIVATE

no, it is this

this is the whole ideology

[mental ingot]

No it is not

[dim wagon]

yes, it is

arbitrarily policing usage

[mental ingot]

Don't insult my intelligence!

I know damn well that such an API is terrible

[dim wagon]

based on a predefined notion you have of what users may or may not want

and yet you continue to use the same logic to make more

[mental ingot]

I don't have a notion what modders might or might not want

[dim wagon]

you are quite literally doing that

[mental ingot]

No

[mental ingot]

Yeah

[mental ingot]

That is the primary operation

Yeah

And that is forges primary operation

You don't seem to be negotiating this for registry events

Or other registration events

[dim wagon]

what?

[mental ingot]

Ot other logic which forge enforces on modders to be used

[dim wagon]

look idk man

[mental ingot]

I understand your argument that you do not want me to artificially enforce limitations on the API

[iron ivy]

I think we should stop this discussion here, its heated enough.

[mental ingot]

And I am not

I am agreeing that a callback needs to be added

If that is an event

[dim wagon]

I'm just so fucking tired of forge pretending to be papa bubble wrap and don't want to see it continued

[mental ingot]

Or a direct callback

[dim wagon]

callback for when a channel connects, both sides, pls

or at the very minimum, a boolean to check the channel's connectivity so it can be checked at playerlogin event or something

I've seen it asked multiple times and the answer always is do something stupidly hacky

for no reason

[mental ingot]

Yeah, and I will get you exactly that

[dim wagon]

thank you

[mental ingot]

I am only saying that if I add it

I need to limit some of its scope

To prevent it from being abused and going around systems which forge has in place.

If you can't agree to that

Then I am sorry but you are talking to the wrong person

[dim wagon]

all I care about is this

and I don't want to see more of this

[mental ingot]

I don't actually understand why that is not at least exposed with a getter.......

That makes 0 sense to me

Simply wrapped in an ImmutableList

And then you have a very nice lookup API.....

Why is this private.....

[delicate dawn]

odds are, at the time it was written the coder didn't think it would be necessary for it to be public and applied the principle of least visibility

[dim wagon]

forge does it all the time

it comes up, all the time

it's one of the more common complaints I've seen about forge historically

[delicate dawn]

in some cases it's by design

[dim wagon]

yeah that's my point

by design is often fucking over actual legitimate devs

[delicate dawn]

well look at it this way

[dim wagon]

there seems to be less thought put into how things might be used

[delicate dawn]

a certain API only works if 100% of modders are using it

if even a single modder bypasses it, then the entire system breaks down

[mental ingot]

^

That is generally my point

[dim wagon]

sure

[delicate dawn]

what do you do?

[mental ingot]

But in all other cases

[delicate dawn]

do you give up? or do you lock it down?

[mental ingot]

It needs to be accessible to modderfs

[dim wagon]

but the modding community is also pretty solid at keeping people in line

when people do stupid shit they get called out on it

I agree that some things need to be protected if theyr'e mission critical

[delicate dawn]

other systems I will agree

we have locked them down when we didn't need to

[dim wagon]

or if it's likely people will use it wrong cus it's just how it appears

[delicate dawn]

a great example is the old packet system

[dim wagon]

but so often this happens on stuff that absolutely should not be locked down

[delicate dawn]

I didn't realize until a few days ago that the network bus is private so it was impossible for a mod to handle the payload events

without going through an event channel

[mental ingot]

Yeah

The whole system

With SimpleImpl

And EventImpl

Is stupid

Why are there two?

Why is there a seperate bus

Why is it private

[delicate dawn]

Eventimpl vs SimpleImpl makes sense

event one is for "raw" payloads

simplechannel is a message registry

[mental ingot]

No, they both do fucking the exact same thing.......

At least in all implementations used by forge

[delicate dawn]

do they? I thought eventimpl didn't have discriminators

[mental ingot]

All forge implementations of event do

[delicate dawn]

and it was a single handler for the whole event channel

[mental ingot]

A fucking int even

[iron ivy]

There is 2 because some people like handling the stream themselves, and simple is implemented atop it.
The bus exist because its unicast

[delicate dawn]

if not then it's stupid

[iron ivy]

Forge uses simple channel

[fair sandal]

If you need something to be exposed just make a PR

Why such aggressivity...

[dim wagon]

yeah cus that's worked well historically too

[mental ingot]

No, it only does in certain cases, like the TierSortingRegistry

[fair sandal]

Well guess what this is NeoForge for a reason

[dim wagon]

is it?

[mental ingot]

^

Yes

[dim wagon]

how's that damagetypes pr going

[mental ingot]

Make the PR

We will merge it

Link?

[dim wagon]

the one I requested like.. 7 months ago

[fair sandal]

Not now

[dim wagon]

and shadows requested review like 4

[granite mortar]

this is getting rather off-topic

[dim wagon]

https://github.com/neoforged/NeoForge/pull/59

they continued it

if you wanna argue PRs are the solution

[fair sandal]

We're busy with higher priority stuff atm, but once the reworks stop using all our time we will have a closer look

[dim wagon]

then at least validate it

[dim wagon]

convenient

[iron ivy]

Well regardless, event channel existed for raw payloads, simple channel existed as a 'simple' descriminator based impl

[granite mortar]

i'd suggest if you want to continue arguing you move elsewhere 😛

[mental ingot]

Yeah both are gone now, you have raw payloads now, with an RL based discriminator

[iron ivy]

That.. is not horrible

I'll allow it

[mental ingot]

It is basically what vanilla has

[iron ivy]

we have to send the RL anyway, may as well merge the descriminator in

[mental ingot]

Yeah

That was my thought

[iron ivy]

Sure

Sounds good

[mental ingot]

And you just get the FBB given

To read and write from

And a seperate handler

[iron ivy]

Amazing

[delicate dawn]

this is a community project, people are not required to review every single PR that happens. literally no one is paying the team, if it hpapens that every maintainer has been busy with something else and no one has looked at that one, well shit happens. it's not going to change no matter how many forks the project has, because that's how a community project works.

[iron ivy]

11/10

[mental ingot]

That gets invoked when you are doing reading

[dim wagon]

don't tell me to stop arguing then continue arguing it

anyway
not sure what other uses there might be for the channel itself orion

but my main thing was just knowing when the channel link was made

since that's what seems to come up most as far as I've seen

[spark saffron]

did we uh, never open a 1.20.1 branch

[iron ivy]

I think it could probably be generalized to an event that gets fired when your counterpart mod id exists actually

[dim wagon]

that's fine too

as long as it's both sides

[spark saffron]

that PR probably needs to be ported to 20.x and kept for 20.1, its fairly important for using the damage system and I'd rather not leave it in total disrepair for 20.1

[delicate dawn]

nope, not yet

[mental ingot]

It will do network stuff only for now ..... So it will fire an event right after it sends the channel information to the client / right after receiving it

[iron ivy]

Sure

[mental ingot]

At that point you can even do the normal disconnect again

Cause from the systems perspective

At that point it is like a normal channel again

Outside of the negotiation logic

It would be stupid for the player

[iron ivy]

Doesn't matter too much, as long as its easily detectable that your counterpart exists.

[iron ivy]

This would be ideal, but compromise for now is fine

[mental ingot]

Funnily enough

The modids are actually not synced

Just the channels for now

I am still trying to figure out the right balance

[iron ivy]

Hmm

May require some more thought, not sure

[mental ingot]

For sure!

[iron ivy]

But that should be fine for now

[mental ingot]

It is part why I don't have a draft yet

There are still like 20 code paths that I need to think through

[iron ivy]

getting proper 'Your mods are out of sync' stuff working is definately priority here

[mental ingot]

I realised this morning that on the server, there is currently no direct way to get the player that send you the packet

[iron ivy]

And making sure its actually readable and understandable by the mod

[mental ingot]

You can reply

But you have no idea to whom you are replying

Is a stupid mistake

But I never thought of it before now

[iron ivy]

You should just pass through the IServerPlayHandler interface

or the impl actually

[mental ingot]

I can't

[iron ivy]

or provide it as optional context

why not?

[mental ingot]

Because there are more then one of those handlers now

[iron ivy]

ugh

okay

[delicate dawn]

semi-related to the conversation above: does the new API have a means to send/receive custom packets that may not have been sent through the new neo system?

[mental ingot]

Like vanilla does stupid switching these days

[iron ivy]

Sending packet should be just Ipacket

[mental ingot]

Yeah as long as the resource location used for the id lines up

And yeah you can still send normal IPackets

[iron ivy]

we should provide a way to take a RL(for your channel) and bytes and turn it into an IPacket

[delicate dawn]

like, if someone wanted to talk to a bukkit plugin

[mental ingot]

I just added methods that allows you to send CustomPacketPayload objects directly

It will wrap them then in the right IPAcket implementation for the direction

[delicate dawn]

👌

[mental ingot]

It is already handled internally

[mental ingot]

In practice giga, buckit uses the same system, so as long as you define an CustomPacketPayload, that writes the same data to the FBB it gets given as the Buckit plugin, you have direct compatibility

I am also like 99% sure it should be compatible with a fabric client

And a forge server

Or the other way around for that matter

[delicate dawn]

yeah

the question is a followup to the network bus being private currently

which means people can send custom payloads

but can't receive them properly

[iron ivy]

Hmm, I have some concerns around sending packets then. But I'll wait till I can actually see code to comment further.

[buoyant pagoda]

i anticipate this quite eagerly

[hasty yew]

Okay, read some of the backlog, now a bit confused. If a mod wants to verify a connection with the server via some of its own logic, and kill the connection if the two are incompatible, what is the intended way to do that?

[mental ingot]

Not

At least not yet

I am still considering the API

I see the usecase

So it will be added

Up to a certain degree

[spark saffron]

as long as its better than the abysmal @NetworkCheckHandler

[hasty yew]

Basically, I've got a bunch of data that should match between the client and server that isn't ensured by registry syncing or anything. So I'd like to send that data during login from, say, server to client, have the client check that it matches, and disconnect if it doesn't, supplying a reason for how it doesn't match. If there's a way to have it use forge's nice disconnect screen and look pretty, all the better

[mental ingot]

You will need to trigger the fancy screen on your own., it is made generic now. But with the new system you can register a configuration phase task that triggers a sync when the client connects, you can do your custom handshake, and then disconnect with that screen.

[hasty yew]

Sounds good to me. I'll probably be hanging out in the help channels when time comes to implement it since I try not to touch this particular sort of stuff if I can avoid it

But as long as I can make sure bad connections are obvious to the player it should be good

[mental ingot]

Yeah

I made that screen generic for that specific reason

I realised early on that during the configuration phase there might be situations where modders perform their own handshakes

And that they should be able to display their errors nicely formatted

[hasty yew]

Better than my current "disconnect with a message and hope it shows up somewhat readable"

[mental ingot]

Yeah that was kind of my aim

I have that same screen

And it is stupid

So 😄

This should work

We might want to iterate on it a bit

With respect to how you trigger it

But that can be added in a PR down the line

https://github.com/neoforged/NeoForge/pull/277

For people that want to review it

[buoyant pagoda]

it begins!

i would like to file a complaint /jk

oof. you did this pre-autoheaders

[mental ingot]

And I will nuke it into orbit!

[buoyant pagoda]

that's gonna be a pain to port patches

[mental ingot]

Comments on formatting

And imports and others

Will get you a ban from reviewing

We are in the fucking draft stage

Behave as such

[buoyant pagoda]

looking through these patches, I remember how much I love auto-headers

[mental ingot]

How do I switch?

Is it just pull?

And then rerun patch generation?

[buoyant pagoda]

for the most part, yes
though you'll have to look through the patches to find lines which were errantly removed by your patch generation (changes that existed in the patches which didn't exist in your workspace when you patch-genned)

[iron ivy]

I'll review it later on tomorrow.

[buoyant pagoda]

actually

another possibility would be to bump your NG to match the one used in 1.20.x latest
then regen patches, and then merge

[mental ingot]

Let me try that

[buoyant pagoda]

since git is smart enough in merging two histories which have the same change in the same place

[mental ingot]

Running now

Lets see

[buoyant pagoda]

neat, -- I see NetworkDirection (or its recent successor, PlayNetworkDirection) got replaced by PacketFlow

I had wondered about doing that same switch some long while ago

[mental ingot]

Generating patches

Lets see

[buoyant pagoda]

genereal note to self: generate an .editorconfig based off the formatter XML, for consistent formatting at least in IntelliJ IDEs (removing much of the need to run the formatter task)

[mental ingot]

Make a ticket, you can do that yourself XD

[buoyant pagoda]

which is now possible since we don't have that "must support Eclipse, else I ain't buying" mentality from the previous management

[mental ingot]

We still do!