-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cookie Api - Fabric Networking Api #3547
base: 1.20.5
Are you sure you want to change the base?
Conversation
...etworking-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerTransferable.java
Outdated
Show resolved
Hide resolved
Hmmm, The injected interfaces don't seem to be working without a cast (testing in an external dev environment). Does any one see anything obviously wrong? (Could also be my dev env lol) |
Try run genSource on the networking project. |
Yeah doesn't seem to fix it. Issue happens both in the test mod & in a full external environment |
...etworking-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerTransferable.java
Outdated
Show resolved
Hide resolved
...etworking-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerTransferable.java
Outdated
Show resolved
Hide resolved
...working-api-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ClientConnectionMixin.java
Outdated
Show resolved
Hide resolved
...networking-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerCookieStore.java
Outdated
Show resolved
Hide resolved
...i-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ServerCommonNetworkHandlerMixin.java
Outdated
Show resolved
Hide resolved
@@ -117,6 +139,13 @@ public void onInitialize() { | |||
} | |||
} | |||
}); | |||
|
|||
ServerLoginConnectionEvents.INIT.register((handler, server) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the same test for configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to ensure that configuration can wait until the cookies have been returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test. I'm not sure if we need to? A mod that wants that behaviour would probably just implement that in the rest of its configuration stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Login also needs testing.
I think its imporant that login and/or configuation can be bocked until the cookie has been returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the best way to hold the network stage be?
...i-v1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java
Outdated
Show resolved
Hide resolved
...i-v1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java
Outdated
Show resolved
Hide resolved
public void fabric_invokeCookieCallback(Identifier cookieId, byte[] cookie) { | ||
CompletableFuture<byte[]> future = pendingCookieRequests.remove(cookieId); | ||
if (future == null) return; | ||
future.complete(cookie); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What thread does this complete on? I would expect the network thread during config, and the main thread during play
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it always runs the network thread, as its triggered by the packet response (unless packets process in the main thread in play? I didn't think they did)
...pi-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerConfigurationNetworking.java
Outdated
Show resolved
Hide resolved
...orking-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerLoginNetworking.java
Show resolved
Hide resolved
...working-api-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerPlayNetworking.java
Outdated
Show resolved
Hide resolved
...working-api-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ClientConnectionMixin.java
Outdated
Show resolved
Hide resolved
@@ -172,4 +178,25 @@ public final void handleDisconnect() { | |||
* @return whether the channel is reserved | |||
*/ | |||
protected abstract boolean isReservedChannel(Identifier channelName); | |||
|
|||
public boolean triggerCookieFuture(Identifier cookieId, byte[] cookie) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a prime target for some unit tests :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you prime the underlying map with data so you can test this function?
...i-v1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java
Outdated
Show resolved
Hide resolved
...i-v1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java
Outdated
Show resolved
Hide resolved
@@ -117,6 +139,13 @@ public void onInitialize() { | |||
} | |||
} | |||
}); | |||
|
|||
ServerLoginConnectionEvents.INIT.register((handler, server) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Login also needs testing.
I think its imporant that login and/or configuation can be bocked until the cookie has been returned.
fabric-networking-api-v1/src/main/resources/fabric-networking-api-v1.accesswidener
Outdated
Show resolved
Hide resolved
...n/java/net/fabricmc/fabric/mixin/networking/accessor/ServerCommonNetworkHandlerAccessor.java
Outdated
Show resolved
Hide resolved
Co-authored-by: modmuss <modmuss50@gmail.com>
@modmuss50 Just wanted to put it out there, would a pre transfer event be useful? In case mods want to set cookies when another mod triggers a transfer? This may also work nicely with the new /transfer command |
...pi-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerConfigurationNetworking.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerConfigurationNetworking.java
Outdated
Show resolved
Hide resolved
...etworking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/AbstractNetworkAddon.java
Show resolved
Hide resolved
...c-networking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/ServerCookieStore.java
Outdated
Show resolved
Hide resolved
...i-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ServerCommonNetworkHandlerMixin.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ServerLoginNetworkHandlerMixin.java
Outdated
Show resolved
Hide resolved
Hey, sorry for being so slow to get back on this. Once my exams are done this week I should hopefully be able to finish this up. Thanks for your help! |
Co-authored-by: modmuss <modmuss50@gmail.com>
This Pr adds an api for the new 1.20.5 transfer packets and cookie system. Currently the api can be accessed through any of the network handlers (except handshake).
Transfer api:
Cookie Api: