-
Notifications
You must be signed in to change notification settings - Fork 499
Allow registering large payloads with deferred max packet size #5119
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
base: 26.1
Are you sure you want to change the base?
Allow registering large payloads with deferred max packet size #5119
Conversation
| * @param maxPacketSize the maximum size of payload packet | ||
| * @param <T> the payload type | ||
| */ | ||
| <T extends CustomPacketPayload> void setMaxPacketSize(CustomPacketPayload.Type<T> type, int maxPacketSize); |
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'm just thinking of alternatives to this API, it does feel a little weird to have a setter that has to be called after registering with a possibly unknown inital max size.
I was wondering if taking a ToIntFunction (T being the payload) in a registerLarge overload would be a better API. I then realised that will only work for encoding 🤔.
My only other ideas are taking an IntFunction or maven even a AtomicInteger, neither of which I love.
(Im just thinking out loud so let me know what you think).
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.
Ya it does feel a bit weird, but personally I still prefer the setter. The other approaches would mean the max size could change arbitrarily, so the value would need to be revalidated each time. The max size could also be decreased from a previously set value which the current API doesn't allow (though I suppose we could just allow it? not sure if that would cause any problems).
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.
Again thinking out loud:
It does seem wrong to allow changing this during gameplay/after packets have been sent, could it take a function to return the max size once per connection/server instance?
E.g In the constructor of FabricPacketMerger/Splitter it could compute the max packet sizes for all registered types?
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.
Perhaps a registerLarge overload that takes an IntSupplier that is called once right before the first packet of a payload type is sent/received? Should be sufficient for the data attachment API assuming no large attachments are registered after mod init.
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've made the above change. I also removed getMaxPacketSize() as I feel the new design doesn't really need it.
...orking-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/PayloadTypeRegistryImpl.java
Outdated
Show resolved
Hide resolved
modmuss50
left a comment
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 this is good now 👍
Adds to thePayloadTypeRegistryAPI to allow modifying and querying the max packet size for a payload type.Adds a
registerLargeoverload toPayloadTypeRegistrythat takes anIntSupplier, allowing mods to defer setting the max packet size.Partially supersedes #4996