[1.20.2] Support common registration packets. Add configuration task API

[modmuss50]

There is more! The original port was very rough, this is hopefully a better longer term solution!

Common channel registration packets

Previously we sent the play packet channels in a custom packet called fabric-networking-api-v1:early_registration this worked OK, but had a few issues.

• Proxy servers didnt always forward this packet onto the client.
• This packet was specific to Fabric making it harder to create cross platform mods and plugins.

After some discussion in the fabric discord about a solution to this problem I raised our ideas to the folk over at @neoforged , @PaperMC and @SpongePowered after a quick chat we came up with the idea of also sending a version packet to allow for future expansion. This is still very much open for public discussion and improvments, and still will be after this PR has been merged. The end goal of this is to allow mod/plugin loaders to negotiate with each other allowing mod developers greater flexibility when developing cross platform mods.

The above diagram details the current packet format, and the expected behaviour of the packets during the new configuration phase added in 23w31a. This is still very open for input and change, I look forward to seeing what everyone can do with this! Thanks to @Patbox for the great suggestion of using the ping packet to contiune to support vanilla clients!

Configuration task APIs

FabricServerConfigurationNetworkHandler is now interface injected onto ServerConfigurationNetworkHandler exposing the vanilla configuration task API. ServerConfigurationConnectionEvents now has a PRE_CONFIGURE that allows tasks to be ran before any vanilla configuration takes place.

Registry sync and custom ingredients have been moved over to use this API.

Fabric Packets

Fabric packets are now written to the PacketByteBuf on the network thread by default in production. In development the previous behaviour is retained to help modders test their packets. The system property -Dfabric-api.networking.write-fabric-packet-calling-thread=false has been added to allow overriding this behaviour.

Disabled for now, as its causing issues in single player. I will resolve this in its own PR as its a larger than I expected.

[Technici4n]

Do these events fire on the main or network thread? I guess network thread, but it would be good to clarify that.

[modmuss50]

Network, I do need to improve the docs. 👍 I think adding an example will be helpful as well. Its quite easy to get wrong and stall the login, so a good example will be helpful.

[modmuss50]

I have added a test mod and expanded the docs to cover this. Please do let me know if you think they need expanding a bit more.

[LambdAurora]

I'm not sure to understand the usage of c:register, isn't minecraft:register supposed to already be able to register the supported channels?

[modmuss50]

I'm not sure to understand the usage of c:register, isn't minecraft:register supposed to already be able to register the supported channels?

We use c:register to to send the channels for the play phase during configuration before play has started. Previously we sent fabric-networking-api-v1:early_registration during login.

We cannot use minecraft:register as it does not contain a way to specify the intended phase, we decided against changing anything within the minecraft namespace.

[LambdAurora]

Mmh, and I guess sending the channels during the actual play phase isn't an option?

[modmuss50]

Mmh, and I guess sending the channels during the actual play phase isn't an option?

It technically would be if we delayed the play packet untill the 2 way handshake has completed. It still supports the minecraft:register / unregister packets during play.

However configuration is the ideal time for this as we need to know the channels before we fire the ServerPlayConnectionEvents.INIT. I imagine this is why we previously sent them during login. I couldnt find much info on the old login packet to be sure of this. Configuration makes things easier for proxies as well.

[LambdAurora]

Mmh, ok, I guess.

My only last question is why include the version field in the register packet if it's already handshaked earlier?

New question: are the registry synchronization channels also registered through the same minecraft:register packets as c:version and c:register? It sounds obvious to me but I'd rather have confirmation since not present on the diagram.

Also, I have to voice some grievances over this spec having been built in Discord spaces, this is awful for discoverability of said discussions, and make it hard to understand the reasons behind the spec itself, this should have been discussed somewhere publicly like GitHub.

[modmuss50]

My only last question is why include the version field in the register packet if it's already handshaked earlier?

I think its still worth having in the packet, even if its just to validate it. This could maybe be removed as it is kinda redunant.

Also, I have to voice some grievances over this spec having been built in Discord spaces, this is awful for discoverability of said discussions, and make it hard to understand the reasons behind the spec itself, this should have been discussed somewhere publicly like GitHub.

I do understand the problems with doing this in a private space as everyone likes to be involved. Its not the first time things have been decided in private and wont be the last. It was in a space that I am guest of so did not have any control over who has access to it. However I believe having a small group of people who understand the problem space well leads to a very productive discussion.

What is shown in this PR is not final and very much still open for discussion and change. Either in this PR, or in future PRs on this proejct or other projects. Quite a lot of this design was initally created in a public channel.

[modmuss50]

New question: are the registry synchronization channels also registered through the same minecraft:register packets as c:version and c:register? It sounds obvious to me but I'd rather have confirmation since not present on the diagram.

Registry sync and any other configuration channels will be in the inital minecraft:register packets.

[LambdAurora]

I do understand the problems with doing this in a private space as everyone likes to be involved. Its not the first time things have been decided in private and wont be the last. It was in a space that I am guest of so did not have any control over who has access to it. However I believe having a small group of people who understand the problem space well leads to a very productive discussion.

While the discussion may be productive, it makes accessing the rationale behind each choices much harder to understand when a PR such like this appears. I also think discussing such subjects on spaces like GitHub can yield similar levels of productivity as talks on GitHub has a tendency to be more fleshed out and discourage unproductive talks.
It also makes anyone backtracking the choices that are made today much harder down the line, I'd very much like to not have a repeat of me hunting down ancient Fabric contributors to ask about a detail of the registry synchronization code which turned out to only be a side effect of how MC worked in 1.11 and was made useless (as I initially guessed) since 1.13.

I'm hoping future decisive discussions of this spec will be held mainly on GitHub for the sake of the general modding community and future modders and maintainers.

[Patbox]

I would change specification a bit.

• First part of running some verification, through I would use there c:handshake/c:version there instead of legacy minecraft:register. Since it would become a standard, it doesn't matter to keep old one there (it should be kept for play only, for mods that need legacy compat)
• Registry sync should come after initial protocol, ideally speaking server-client would first negotiate what they can understand (ofc with server waiting for answer). This would allow for a platform to support alternative registry sync protocol, as it would know what client supports.
• Then you could run core (non-data/static) registry sync to make vanilla registries in line. Could also include other modded sync that could be useful for mods. Ofc only for client supporting it. If it's required and client doesn't support it, it would be disconnected instead
• At this stage vanilla confuguration stuff could be executed.
• Extra modded configuration (if supported) for things that need to be synced after vanilla.
• Transition to play.

[LambdAurora]

• First part of running some verification, through I would use there c:handshake/c:version there instead of legacy minecraft:register. Since it would become a standard, it doesn't matter to keep old one there (it should be kept for play only, for mods that need legacy compat)

Does Mojang already use minecraft:register in the configuration phase?
I feel like the advantage of that packet is that most MC clients will understand it. Even if they don't support this protocol.

• Registry sync should come after initial protocol, ideally speaking server-client would first negotiate what they can understand (ofc with server waiting for answer). This would allow for a platform to support alternative registry sync protocol, as it would know what client supports.

Given what was said earlier, about those packets being advertised through the initial minecraft:register, this is a non-issue since both sides would already be aware of the supported synchronization protocols.

• Extra modded configuration (if supported) for things that need to be synced after vanilla.

I feel like the current spec doesn't prevent that, it just doesn't define the order of that.

[Patbox]

Nope, minecraft:register is legacy supported channel list packet (originally designed by Dinnerbone for Minecraft 1.1). Not used in Minecraft by itself

[LambdAurora]

Nope, minecraft:register is legacy supported channel list packet (originally designed by Dinnerbone for Minecraft 1.1). Not used in Minecraft by itself

Oh, after a quick look it indeed changed quite a lot, there was many more custom pay load packets before...
Strange.

This is definitely puzzling because it means a vanilla would log not understanding minecraft:register in the logs...
I guess using it specifically might not make as much sense anymore? Though I do question the usefulness of a c:handshake.

[SquidDev]

I realise the configuration changes are the main focus of this PR, but felt it was worth commenting on the FabricPacket changes as I requested them. ❤️ for those!

Are FabricPacketPayloads actually handled anywhere right now - I couldn't see any obvious changes in ClientCommonNetworkHandler/ServerCommonNetworkHandler. It's a bit of a shame that the Map<Identifier, PacketByteBuf.Reader> maps in the custom packet classes are immutable. Ideally registering a new packet would just extend those maps with the PacketType's reader.

[modmuss50]

Are FabricPacketPayloads actually handled anywhere right now

FabricPacketPayloads are currently only used when sending a packet, as they extend the vanilla CustomPayload class they can be passed stright into the custom payload packet. See Server/ClientNetworkingImpl. Thus the PacketByteBuf (PBB) is written to on the network thread.

It's a bit of a shame that the Map<Identifier, PacketByteBuf.Reader> maps in the custom packet classes are immutable. Ideally registering a new packet would just extend those maps with the PacketType's reader.

This isnt really an issue, right now we hook into readPayload and return a the copied PBB payload as we need to handle the PBB on the network thread. The FabricPacket API is currently implemented ontop of this. I think in a future PR I will look into removing this abstraction and using FabricPacketPayload directly for incoming packets, removing the need to copy the PBB.

That should all be impl detail, and not affect mods. As this PR has grown quite large I think it makes sense to do that later.

I hope this makes sense :D

[SquidDev]

Ahhh, gotcha. The lack of symmetry confused me, but that makes sense. Thanks!

[modmuss50]

I would change specification a bit.

• First part of running some verification, through I would use there c:handshake/c:version there instead of legacy minecraft:register. Since it would become a standard, it doesn't matter to keep old one there (it should be kept for play only, for mods that need legacy compat)

Continuing to use the legacy packets to dermine the current phases channels seems fine to me, the main reason we need a custom packet is to allow sending the play packets while not being in the play phase. If either the client or server doesnt understand the custom stuff, but does handle the old packets chances are things will mostly work out of the box. At least to the current status-quo.

• Registry sync should come after initial protocol, ideally speaking server-client would first negotiate what they can understand (ofc with server waiting for answer). This would allow for a platform to support alternative registry sync protocol, as it would know what client supports.

Yes, thats exactally what this allows for. The inital minecraft:register will contain the supported channels, thus allowing the server to decide how to handle reg sync. Or skip it if the client doesnt support it.

• Then you could run core (non-data/static) registry sync to make vanilla registries in line. Could also include other modded sync that could be useful for mods. Ofc only for client supporting it. If it's required and client doesn't support it, it would be disconnected instead

Exactally, the rest of this PR adds an API for mods to do just this, see the new testmod. The server can decide to proceeded or disconnect if the client cannot recieve on a given channel.

• At this stage vanilla confuguration stuff could be executed.
• Extra modded configuration (if supported) for things that need to be synced after vanilla.
• Transition to play.

Yep, this PR handles all of that that by having a BEFORE_CONFIGURE and CONFIGURE event, vanilla goes in the middle, transiition to play happens once all tasks have been completed.

I hope that answers your questions, I did miss the mark a little on how I worded how this spec was agreed upon (I updated the PR), it was very much the other parties saying this is a good/intresting idea and not commiting to using it. The worst case is we have something that behavaes the same as it does in previous versions (not great, not terrible).

I should have also explined in detail what problem was being solved, its a low level system that I expect 99% of mod developers dont even know exists let alone have a good understanding of what it does. I certainly didnt untill I was forced into this last week 😆

A lot of it was discussed/designed in public before I asked for input elsewhere. The main addion was the version packet. Its still very much open for changes (even past this PR being merged), if anyone did feel left out of the converstation I'm sorry it wasnt my intent. I think its good that this discussion did happen as it shows what can possible if we all work togeather, just need to figure out a good (and fun!) way to go about this next time.

I do plan to merge this tomorrow and release alongside the next snapshot (assuming there is going to be one).

[liach]

Should check the 0xFAB71C parameter for extra safeguard.

[modmuss50]

I did orignally do this, but decided against it in the case that another platfrom decides to use this. Maybe I should pick something more generic.

[liach]

Suggested change

	public static <T extends FabricPacket> Packet<ClientCommonPacketListener> createS2CPacket(T packet) {
	public static <T extends FabricPacket> Packet<ClientConfigurationPacketListener> createS2CPacket(T packet) {

a better signature?

[modmuss50]

CustomPayloadS2CPacket extends Packet<ClientCommonPacketListener> so this is correct.

[liach]

The events seems to be missing on the in-play reconfiguration code path, which never calls sendConfigurations (used by login to configuration only)

[modmuss50]

Ill check this, im fairly sure it does work when reconfiguring.

[modmuss50]

Oh yeah, you are correct. I tottaly missed that queueJoinWorldTask method 👍 As its only called from the debug command right now I think its ok to leave/handle at a later date. Its a little unclear to me how this is intended to be used.

[liach]

I peeked through the new configuration state logic in enigma and this PR and have a few remarks:

1. The old login-query packet registration was an implementation detail, a workaround for packet registry that isn't immediately available upon Game join packet. As long as the packet channel list is still accessible at the beginning of play phase event, this mechanism can be nuked.
2. Vanilla's configuration stage has a "task" mechanism which is sequential; it waits for one task to complete before moving into the next thing in the queue. This is inefficient if we wish to allow parallel processing of data and dependency declaration instead.
3. Mojang now adds EnterConfigurationC2SPacket for C2S login->configuration state transition (previously, C2S login->play simply "happened" when the client receives a GameJoinS2CPacket). Clients can totally send minecraft:register and unsolicited pongs immediately after sending a EnterConfiguration for login, but our implementation doesn't seem to be ready for such scenarios. On a side note, vanilla simply ignores them, and its task system prevents unsolicited ReadyC2SPacket (configuration->play transition).

[modmuss50]

I peeked through the new configuration state logic in enigma and this PR and have a few remarks:

1. The old login-query packet registration was an implementation detail, a workaround for packet registry that isn't immediately available upon Game join packet. As long as the packet channel list is still accessible at the beginning of play phase event, this mechanism can be nuked.

Yes, we are discussing if we can get rid of the login related APIs, it seems like we can. Wont be done as part of this PR though.

2. Vanilla's configuration stage has a "task" mechanism which is sequential; it waits for one task to complete before moving into the next thing in the queue. This is inefficient if we wish to allow parallel processing of data and dependency declaration instead.

Yes, this is the API that we have exposed, in theory it would be possible for Fabric to change the behaviour of how the tasks are ran to support running them in parallel. I think for now keeping it as close to vanilla is fine.

3. Mojang now adds EnterConfigurationC2SPacket for C2S login->configuration state transition (previously, C2S login->play simply "happened" when the client receives a GameJoinS2CPacket). Clients can totally send minecraft:register and unsolicited pongs immediately after sending a EnterConfiguration for login, but our implementation doesn't seem to be ready for such scenarios. On a side note, vanilla simply ignores them, and its task system prevents unsolicited ReadyC2SPacket (configuration->play transition).

Yeah... I think it should handle this case ok, we check to make sure that the inital registry/ping packets were sent before doing anything about their response. Do you have a better solution?

[modmuss50]

Merging before porting to the new snapshot, please open an issue for further changes. (I will likely have a follow up PR as well)