More Mob Variants

More Mob Variants

13M Downloads

[1.20.1] Mixin injecting SynchedEntityData into entities not owned by MobVariants is dangerous

TelepathicGrunt opened this issue · 5 comments

commented

Entity Data Accessors should only be ever set by the creator of the entity. Never added to by other mods.

The reason for this is because Entity Data Accessors are order dependent so if multiple mods tries to add these entity data accessors to mobs and on client, they run in a different order than the server did (because mod load order is not guaranteed), the client will get forcibly disconnected from server when the entity spawns.

Instead, this mixin part needs to be removed and migrated properly to a Forge Capability because capabilities were designed specifically for this use case of attaching data to arbitrary entities. It is safer for mods as the vanilla system is designed specifically only for vanilla and unsafe for mods. Then you would use your own packet to send the data to client when needed.

private static final TrackedData<String> VARIANT_ID =

private static final TrackedData<String> VARIANT_ID =

private static final TrackedData<String> VARIANT_ID =

and other files as well.

This is especially a problem in large modpacks where many mods are doing this. Example is TNPLimitless 7 where there's many mods injecting SynchedEntityData onto entities and are getting out of order due to mod load order. Which is causes a lot of reports of people getting disconnected from server due to:
image

commented

I know you mention replacing that mixin part with Forge Capabilities, does that only apply to the Forge version of this mod or is there an equivalent for the Fabric version? I'm assuming that the issue would occur on both modloaders.

commented

Fabric can use Cardinal components as the equivalent of forge caps.

Another option is to change this field to be a normal field (not a data accessor at all) and send your own packet to client to set the client values. Supplementaries dev did a similar fix MehVahdJukaar/Supplementaries#851 (comment)

With the mixin to the read and write nbt methods for the entity, you can make the normal fields still persistent. Whatever option you want to do, totally valid. Just can’t be mixin adding data accessors fields. Anything else should be ok

commented

Got it, thanks for the tip. I'll start switching everything over, haven't received any reports of this actually occurring yet but I'm already refactoring everything with this update so might as well fix it now lol

commented

Reached out to you on discord to review some changes before I mark this as resolved.

commented

Fixed in the upcoming 1.3.0 update, data syncing is now done manually between the server and client.