neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.14k stars 165 forks source link

Synced data attachments #335

Open Technici4n opened 9 months ago

Technici4n commented 9 months ago

Now that the cap rework is out, I would like to add a system for opt-in automated syncing of block entity, chunk, and entity data attachments from the server to the client. For this, I need the input of modders that would use such a system. To help us implement this, please comment here by explaining your use case, and try to answer the following questions if possible.

Thanks!

Yes this is literally a "request for comments".

TelepathicGrunt commented 9 months ago

Use cases I seen where people’s used SyncedEntityData (that actually needed client sync and wasn’t just an error on misuse of wrong tool) was to set server controlled data on entity that would change looks or render something on the entity on client side.

So entity spawns on server, data is set, then entity spawns on client side and gets data set from the server. If this can be done on client entity spawn packet, that would be best so entity doesn’t spawn with default looks while waiting for separate packet from server for the synced data.

Caltinor commented 9 months ago

I am going to speak to Chunk data, since that's what I primarily use. I use chunk data for location-specific things like protection zones, block placement history, etc. I have been syncing this data for things like rendering protection borders in the world and overlays when looking at blocks you placed. That said, I don't necessarily need everything I store in a chunk on the client. Therefore, I would favor a manual and selective syncing method to minimize network traffic to only when i change things and need it on the client.

TelepathicGrunt commented 9 months ago

More entity use cases I am seeing by modders:

Manual sync probably would be fine. Most of these are sync once or so. Though stuff like variants needs to have data set initially for the mob and auto synced when the spawn packet is sent to client

MehVahdJukaar commented 9 months ago

So my use case for this would be having a simple way to add some data which is relevant to the client and is needed as soon as an entity spawn. More specifically I had some skeleton spawn with a quiver item which would also be shown on their model. This of course needs to be synced to the client and the fact that the quiver attribute is set when the entity spawns makes it so that one can't just send a packet to the client right there as the entity might not be created yet on client. Another use case I had was related to block entity capabilities. I this case I have some extra data associated with certain vanilla and or modded tile entities. This data would need to be sent to the client right after the tile entity is loaded and just like the after mentioned usecase sending a packet straight away might not work as the tile might not be created yet on client

justliliandev commented 9 months ago

it would also be nice to be able to define to which client an attachment should sync to. attachments needed for data hud stuff only has to be send to the player the attachment is attached to. Syncing should, imo not be automatically. Just like the markdirty methods for saving, the attachment user should, imo notify the sycing system that data has changed

MercuriusXeno commented 9 months ago

My use case is I'm saving a typed resource to the player. The capability is attached on entity creation and cloned/persisted on death (deliberately lossless at the time of writing). Think 2x HashMap<SomeEnum, Long>, one for the capacities of each type of resource being stored, and another for the amount the player is holding.

Q1 Should syncing happen automatically, or should modders notify the system that they want their data attachment to be synced?: I can't think of a reason I wouldn't want this synced back to client automatically.

Q2 Should it be possible to send incremental updates as well? (If only part of the data changes, only send that part): My networking setup consists of syncing the smallest unit of work possible for changes to these values, so a packet would consist of: SomeEnum and a long, sync to client player to let them know either their capacity or their amount has changed (two different packets). Ideally I'd keep my network impact as low as humanly possible, so I'd prefer the option to sync only a specific change - to wit this is probably an argument in favor of manual syncs, or complicates thing.

Q3 Does the data need to arrive at the same time as vanilla's data? (Potentially problematic for vanilla clients): In my case, the data could sync separately from the vanilla data. I could deal with a delay; I'd consider it my responsibility to defer any handling until I could guarantee the data was synced.

Q4 Do we need a graceful fallback for vanilla clients?: I may not understand the question; I think this is N/A for my use case. Mine isn't going to be a capability my mod can allow a player to not have. This would be a throw for me I think?

Worth noting at the time of writing, I only need to sync this data to the client player the stats belong to and not other nearby players.

TL;DR: I'm very weakly opinionated that the automatic sync feels like it would be really nice/sugar. Having said that, my networking setup was not cumbersome and is some very low-friction netcode. I reckon newcomers to the system aren't going to be totally repelled (this was my first capability and I was not repelled). I think syncing manually is not an unreasonable expectation, but automatic sure sounds nice.

Speiger commented 9 months ago

Q1 Should syncing happen automatically, or should modders notify the system that they want their data attachment to be synced?: I can't think of a reason I wouldn't want this synced back to client automatically.

To answer that one. Some Capabilities may only be for data storage and others be for sync, but it could also be for mixed data. And we do have to account for bad connections too. Or for the fact that also some live on data limits. So having the option to opt/in out on specific entries might be a good idea.

To give a bit of insight how IC2C for example does it. IC2C has a Annotation you can hook on to fields, and whenever you want to sync a field you simply push a request with the field name you want to sync in "updateData" function. Via reflection the field is then obtained, packed into a byte buffer with all the other fields and then send to the client. Upon BlockEntity creation there is a list keeping track of fields that should be automatically initialized, so that these fields are auto synced. If a Class type isn't supported, then you can simply create a class extending an interface that has a PacketBuffer accessible to it (read/write). But you do lose "null" support on these type of fields.

This system effectively works similar to the Entity sync system, but doesn't rely on wrappers for everything you want to sync. Because wrappers for everything SUCKS IMO.

It is more flexible, does support custom data types without actually having to register anything. Most common objects could be simply added default support for. And the owner of the capability gets the control "when and how much" should be synced.

Only downside, it heavily relies on reflection.

lukebemish commented 9 months ago

I'd say sync should definitely be opt-in - there might be, say, data on another player that you'd rather not expose to an untrusted client to prevent cheating or whatnot. Beyond that, yeah, incremental would be good - but let's not assume anything about the structure of the data folks have! I'd say the best way to implement this would simply be a separate method you override for writing data to sync, so you can control exactly what goes to the client - maybe with some context added to help make incremental syncing possible? I'm not entirely sure, mostly spitballing here - I've got a use case for incremental syncing in one of my own mods but frankly I'm not sure I'd use such a system in neoforge, I might just stick with my existing system for manually syncing stuff only when I think it's relevant.

Whatever we do, let's not use any weirdness with annotations for this, please - we don't want a repeat of the nonsense that is the config system. Let modders declare the structure of their own data, and which bits need to be synced or not, instead of having that be something you try to imply.

MercuriusXeno commented 9 months ago

To answer that one. Some Capabilities may only be for data storage and others be for sync, but it could also be for mixed data. And we do have to account for bad connections too. Or for the fact that also some live on data limits. So having the option to opt/in out on specific entries might be a good idea.

Q1 Should syncing happen automatically, or should modders notify the system that they want their data attachment to be synced?: I can't think of a reason I wouldn't want this synced back to client automatically.

Read this again but more carefully. I didn't say I can't think of a reason anyone wouldn't want automatic syncing. I said >I<, specifically, for my use case.

Speiger commented 9 months ago

Read this again but more carefully. I didn't say I can't think of a reason anyone wouldn't want automatic syncing. I said >I<, specifically, for my use case.

That is totally fine @MercuriusXeno , i just pointed these things out because they get missed by a lot of people ^^" Still, i am sorry!

TT432 commented 2 months ago

I believe the DataAttachment API only needs to have a StreamCodec and send updates when changes occur. I think Java's expressiveness is insufficient to clearly convey incremental data synchronization without writing a lot of boilerplate code. Both Codec and StreamCodec still require a lot of boilerplate code, but I believe using the vanilla rules helps achieve consistency in usage.

Or similar to Config, where each entry is separated and internally encoded numerically. This way, incremental synchronization can be achieved, and using a builder would not cause the same discomfort for user as using Codec.

lukebemish commented 2 months ago

As long as custom syncing logic is somewhat easy to implement (that is, especially on the "recieved data from server" end to do stuff with existing data instead of tossing it out and replacing it with newly deserialized stuff), incremental updates are not too hard to do -- heck, I've got a system that does incremental updates with StreamCodecs. I don't think neo needs to provide a dedicated system for this as the few folks who want it probably have their own ways they want to do it -- so long as the appropriate hooks for syncing are present it should be all fine.

I do not believe using a system like config stuff does is a good idea here. It seems, frankly, rather terrible to work with.

TT432 commented 2 months ago

As long as custom syncing logic is somewhat easy to implement (that is, especially on the "recieved data from server" end to do stuff with existing data instead of tossing it out and replacing it with newly deserialized stuff), incremental updates are not too hard to do -- heck, I've got a system that does incremental updates with StreamCodecs. I don't think neo needs to provide a dedicated system for this as the few folks who want it probably have their own ways they want to do it -- so long as the appropriate hooks for syncing are present it should be all fine.只要自定义同步逻辑有点容易实现(也就是说,特别是在“从服务器接收的数据”端对现有数据进行处理,而不是将其扔掉并用新的反序列化的数据替换),增量更新就不太容易很难做到——哎呀,我有一个使用 StreamCodecs 进行增量更新的系统。我不认为 N​​eo 需要为此提供一个专用系统,因为少数想要它的人可能有他们自己想要的方式——只要存在适当的同步钩子就应该没问题。

I do not believe using a system like config stuff does is a good idea here. It seems, frankly, rather terrible to work with.我不认为使用像配置这样的系统是一个好主意。坦率地说,合作似乎相当糟糕。

I hope to have an easy-to-use API where users only need to define their data structures, and then neo handles everything internally like magic. Users can implement broader customizations on their own.

In my experience, most data in mod development can actually be represented as a tree composed of records, which is very similar to a config. At the same time, I don't think there's any way to cover all possible cases. Based on this, I believe that a structure representation system similar to config is very useful.

ApexModder commented 2 months ago

This change would help out greatly for my modded flag system (#1322), as I currently have to sync the flag changes to clients manually as and when they change. With this change I wouldnt need todo so anymore, as I update the overworlds level data when ever a flags state changes, which would then auto-sync changes to the clients.

I would however need some way to tell the attachment type to notify ALL clients, not just clients within the level assoicated with my data attachment. This is due to me storing the data in the overworlds level data but flags being server wide data.