-
Notifications
You must be signed in to change notification settings - Fork 416
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
Data Attachment Sync API #4049
base: 1.21.1
Are you sure you want to change the base?
Data Attachment Sync API #4049
Conversation
Defines 3 payload/packet types based on targets: Chunks + block entities, Entities, Worlds. Each of these require being sent at out-of-sync times, and so need to be handled separately.
* Added more payload types, renamed a bunch * Wrote logic for sending each type of packet, client and serverside * Fixed interface injection (pointed to ServerWorld instead of World) * Began work on testmod TODO: a lot of the syncing code is very unoptimized
* Added API for common ways of syncing * Optimized the packet creation code by caching and bookkeeping ahead of time
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 good start, however there is a lot going on here. We need to be especially careful with C2S packets. Im especially worried about the refreshAttachments
, in an ideal world I dont think it should be needed at all.
...-api-v1/src/client/java/net/fabricmc/fabric/impl/attachment/client/AttachmentSyncClient.java
Outdated
Show resolved
Hide resolved
...-api-v1/src/client/java/net/fabricmc/fabric/impl/attachment/client/AttachmentSyncClient.java
Outdated
Show resolved
Hide resolved
...-api-v1/src/client/java/net/fabricmc/fabric/impl/attachment/client/AttachmentSyncClient.java
Outdated
Show resolved
Hide resolved
...ttachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentRegistry.java
Outdated
Show resolved
Hide resolved
...chment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSyncImpl.java
Outdated
Show resolved
Hide resolved
...ent-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSyncPayload.java
Outdated
Show resolved
Hide resolved
...v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AcceptedAttachmentsPayloadC2S.java
Outdated
Show resolved
Hide resolved
...chment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSyncImpl.java
Outdated
Show resolved
Hide resolved
...chment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSyncImpl.java
Outdated
Show resolved
Hide resolved
...chment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSyncImpl.java
Outdated
Show resolved
Hide resolved
This reverts commit bbd6096.
* write AttachmentChange data to bytes before sending, and use that to check * reorganize folders * unify the builder methods by introducing AttachmentSyncPredicate
* Now stored in the ClientConnection * Will log a warning on configuration if there are syncable server attachments that aren't supported on a client * Fixed forgetting to store packet codec
...ttachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentRegistry.java
Outdated
Show resolved
Hide resolved
...ttachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentRegistry.java
Outdated
Show resolved
Hide resolved
...ment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentSyncPredicate.java
Outdated
Show resolved
Hide resolved
...ta-attachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentType.java
Outdated
Show resolved
Hide resolved
...ta-attachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentType.java
Outdated
Show resolved
Hide resolved
...attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSync.java
Outdated
Show resolved
Hide resolved
...attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSync.java
Outdated
Show resolved
Hide resolved
...attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSync.java
Outdated
Show resolved
Hide resolved
...attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentSync.java
Outdated
Show resolved
Hide resolved
...-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/SyncType.java
Outdated
Show resolved
Hide resolved
...tachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/AttachmentChange.java
Outdated
Show resolved
Hide resolved
...a-attachment-api-v1/src/main/java/net/fabricmc/fabric/mixin/attachment/BlockEntityMixin.java
Outdated
Show resolved
Hide resolved
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.
Some code changes are still required, before trying the testmod
...tachment-api-v1/src/main/java/net/fabricmc/fabric/mixin/attachment/ChunkDataSenderMixin.java
Outdated
Show resolved
Hide resolved
...-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/BlockEntityAttachmentReceiver.java
Outdated
Show resolved
Hide resolved
...achment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/AttachmentRegistryImpl.java
Outdated
Show resolved
Hide resolved
...ta-attachment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentType.java
Show resolved
Hide resolved
...api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/sync/s2c/AttachmentSyncPayload.java
Outdated
Show resolved
Hide resolved
At long last, I found it within me to properly test the testmod on a dedicated server, and squashed all relevant issues! In any case, I would recommend #4109 be merged before this one. |
This is looking good, I havent done a full review but nothing stands out to me from a quick look so I dont expect many changes. I have re-ran the server test. |
# Conflicts: # fabric-data-attachment-api-v1/src/main/java/net/fabricmc/fabric/impl/attachment/AttachmentRegistryImpl.java
...ment-api-v1/src/main/java/net/fabricmc/fabric/api/attachment/v1/AttachmentSyncPredicate.java
Outdated
Show resolved
Hide resolved
Added to last call, sorry it took me so long (Factorio is partly to blame). The API looks solid, we could likely go back and forth on the impl details for a year so I think its best to get something out and if there are problems fix them as and when. |
Completes the data attachment API with client-server syncing capabilities.
Motivation
The existing API works great for attaching data to game objects, be they serverside or clientside, but lacks any ability to synchronize between the two.
A mod that wants to add a "thirst" mechanic can easily do so on the server side by attaching an integer to every player. However, the mod may also want to use this information to render additional HUD elements on the client. Currently, informing the client of this data can only be done manually, which is cumbersome, error-prone, and is much better-suited as an API feature.
API Changes
The API doesn't change a lot (at least compared to the implementation), with a few new methods and one new class.
One new method has been added to
AttachmentRegistry.Builder
, namelysyncWith
. It declares that an attachment type may be synchronized with some clients, and takes aPacketCodec
to encode attachment data over the network, as well as an element of the newAttachmentSyncPredicate
interface.This interface extends
BiPredicate<AttachmentTarget, ServerPlayerEntity>
to allow for user-defined predicates, and provides some common presets:all()
: attachment data will be synchronized with all clients (that track the target).targetOnly()
: attachment data will only be synchronized with the target it is attached to, when it is a player. If the target is not a player, it won't be synchronized with any client.allButTarget()
: reverse of the above. For non-player targets, attachment data will be synchronized with all clients.NOTE: for a user-defined condition, whether attachment data is synchronized with a client can change at runtime (e.g. "sync only with operators" when a player changes operator status). A specialized method to "refresh" data was considered, but in the end discarded due to complexity. It falls to individual mods to handle these edge cases.
AttachmentType also gets one new
isSynced
method to know whether a given attachment type can be synced.Usage
Here is how one could register an attachment for a "thirst" meter like mentioned above.
To-do: