Supplementaries

Supplementaries

82M Downloads

[🐞]: [1.20.1] Mixin injecting SynchedEntityData into entities not owned by Supplementaries is dangerous

TelepathicGrunt opened this issue Β· 6 comments

commented

Version-Loader

1.20.1-forge

Issue Detail

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.

@Unique
private static final EntityDataAccessor<Boolean> HAS_QUIVER =
SynchedEntityData.defineId(Skeleton.class, EntityDataSerializers.BOOLEAN);
protected SkeletonMixin(EntityType<? extends AbstractSkeleton> entityType, Level level) {
super(entityType, level);
}
@Inject(method = "defineSynchedData", at = @At("TAIL"))
protected void defineSynchedData(CallbackInfo ci) {
this.getEntityData().define(HAS_QUIVER, false);
}

@Unique
private static final EntityDataAccessor<Boolean> HAS_QUIVER =
SynchedEntityData.defineId(Stray.class, EntityDataSerializers.BOOLEAN);
protected StrayMixin(EntityType<? extends AbstractSkeleton> entityType, Level level) {
super(entityType, level);
}
@Override
protected void defineSynchedData() {
super.defineSynchedData();
this.getEntityData().define(HAS_QUIVER, false);
}

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

yeah was aware of this...
tho never had reports of issues happening
Keep telling that forge should make this safe with some extra syncing. Anyways guess ill do some networking

commented

So in your case, you need the data on client. Do what I do for my horse owner uuid syncing workaround. Mixin to client. When entity spawns on client, send packet to server asking for data. Server then gets the needed data and sends it back to client. Client then holds the data

https://github.com/TelepathicGrunt/Bumblezone/blob/c20cfc70e51f47ccbb8b5dfc04603c9354ef1abe/common/src/main/java/com/telepathicgrunt/the_bumblezone/mixin/client/ClientPacketListenerMixin.java#L30

https://github.com/TelepathicGrunt/Bumblezone/blob/c20cfc70e51f47ccbb8b5dfc04603c9354ef1abe/common/src/main/java/com/telepathicgrunt/the_bumblezone/packets/SyncHorseOwnerUUIDPacketToServer.java#L51

https://github.com/TelepathicGrunt/Bumblezone/blob/c20cfc70e51f47ccbb8b5dfc04603c9354ef1abe/common/src/main/java/com/telepathicgrunt/the_bumblezone/packets/SyncHorseOwnerUUIDPacketFromServer.java#L51

Only 1 mixin and whatever system you want for attaching that data to the entity. Whether that be cap or to the entity’s nbt directly or to a normal field mixin added to entity that is not an EntityDataAccessor

commented

and synced entity data isnt even used here to store data here, that is done with the save methdods which are arguably way easier to use than capabilities. Capabilities still dont offer any automatic syncing behaviors either for entities.
Reason synced data is so useful is that it automatically gets sent in the spawn packet. Here i'm setting some data after an entitty has spawned in finalize spawn. by there the entity isnt even available on client so i cant even send a packet there. I'm also pretty sure the same would apply tothe spawn entity event and if thats the case i dont really know how to replace this other than some huge mixin mess

commented

im checking them now. Data is just visial so not critical so such a delay introduced by client asking for data shouldnt be a problem

commented

I have not checked out the entity spawn events but if those fire on client, it could replace the mixin I suggested

commented

behold, very ugly code that syncs that with just 1 S2C packet and fabric/forge entity spawn events. Works both with newly added entities and existing ones

image