NOTABLE BUG FIX (1.19.3) : Client-Server Init Sync Bug.

[DashieDev]

Currently, at this point, there is a notable sync problem in DogEntity.java that cause the client failed to fetch and init data from the server, hence upon joining the world, players are unable to view dog talents, accessories, and level, and the dog appeared as if he is freshly trained, even though on serverside he is already equipped. Currently, the solution seems to be to always create new objects data and set it via the entitydata.set() function in and only need to be in DogEntity::readAdditionalSavedData().

[percivalalb]

Thanks, looking into this now. So the reason is due to the way values are compared when synced. The data sync system was designed with only immutable objects in mind.

net.minecraft.network.syncher.SynchedEntityData#L121 uses org.apache.commons.lang3.ObjectUtils.notEqual which is equivalne to:

return a == b || a != null && a.equals(b);

So in the case of of the ArrayList<TalentInstance> and DogLevel objects since they are the same object with the the internals changed so they always hit that first case a == b. Therefore any change to the internal values of the object are not recongised as changing the value that needs syncing. Which is why this works as it actually changes the object

The sync data needs a small tidy up anyway,

[DashieDev]

Hmm, I cannot confirm if Minecraft totally 100% treats synced objects immutably...... But I can totally confirm that it does in the in the SynchedEntityData::set function like how ReduxJS treats the UI state. It may 100% in design due to we are supposed to set the entityData via that function only, but we have chosen to break the rule so .....

//net.minecraft.network.syncher.SynchedEntityData.class

// :119
public <T> void set(EntityDataAccessor<T> p_135382_, T p_135383_) {
      //.......
      // (1/1 reference to ObjectUtils in this file)
      if (ObjectUtils.notEqual(p_135383_, dataitem.getValue())) {
      //.......
}

// org.apache.commons.lang3.ObjectUtils.class

// :629
@Deprecated
public static boolean equals(final Object object1, final Object object2) {
    if (object1 == object2) {
        return true;
    }
    //.....
}

//.....

// :1184
public static boolean notEqual(final Object object1, final Object object2) {
    return !equals(object1, object2);
}

But the thing is, after forcefully set the data entry to this.isDirty = true, which is what DoggyTalents currently do (breaking the rule), the data is going get send to the client no matter what.

//net.minecraft.network.syncher.SynchedEntityData.class

// :134
@Nullable
 public List<SynchedEntityData.DataValue<?>> packDirty() {
    //.....
       for(SynchedEntityData.DataItem<?> dataitem : this.itemsById.values()) {
          if (dataitem.isDirty()) {
             dataitem.setDirty(false);
             if (list == null) {
                list = new ArrayList<>();
             }

             list.add(dataitem.value());
          }
       }
   //....
 }

And the client did not use SynchedEntityData::set to set the received payload (at least on multiplayer, on singleplayer i don't know if it even send a packet or not, i have set a breakpoint but get no response in singleplayer, but do in multiplayer).

// net.minecraft.client.multiplayer.ClientPacketListener.class

// :482
public void handleSetEntityData(ClientboundSetEntityDataPacket p_105088_) {
      //......
         entity.getEntityData().assignValues(p_105088_.packedItems());
      //......
}

So i kinda don't think that Minecraft entity data sync treats object as fully immutable, at least when we chose to break the rule. As a proof of concept i implemented the exact fix at DoggyTalentsNext, and all of the rest of the feature that involves setting entity data worked good. For example, let's look at when an immortal dog dies,

//DoggyTalentsNext
//doggytalents.common.event.IncapacitatedEventHandler.java

// :21
@SubscribeEvent
public void onWolfOrDogDeath(LivingDeathEvent ev) {
 //.........
      AccessoryInstance hurtLayer;
      if (ev.getSource().isFire()) {
          hurtLayer = DoggyAccessories.INCAPACITATED_BURN.get().getDefault();
      } else if (ev.getSource() == DamageSource.MAGIC) {
          hurtLayer = DoggyAccessories.INCAPACITATED_POISON.get().getDefault();
      } else {
          hurtLayer = DoggyAccessories.INCAPACITATED_BLOOD.get().getDefault();
      }

      if (hurtLayer != null) d.addAccessory(hurtLayer);
      if (d.isPassenger()) d.stopRiding();
      if (d.isVehicle()) {
          for (var x : d.getPassengers()) {
              x.stopRiding();
          }
      }
//..... 
}

As you can see the serverside-only function do a serverside calls to Dog(DogEntity)::addAccessoryadd the injured accessory layer on the dog, which don't immutably deals with the object.

// doggytalents.common.entity.Dog(DogEntity)

// : 2086
@Override
public boolean addAccessory(@Nonnull AccessoryInstance accessoryInst) {
    //.....
    accessories.add(accessoryInst);

    this.markDataParameterDirty(ACCESSORIES.get());
    //.....
}

And the dog still update and show injured texture clientside upon being defeated both in singleplayer and multiplayer as i have been rigorously testing.

[percivalalb]

In hindsight creating the markDataParameterDirty was not the best move as it makes assumptions about the MC base code. In a subsequent update it could cause unforeseen behavour. I've removed that for now.

[DashieDev]

Yeah i agreed, it is better to follow the rules when neccessary, there is no significant cost of creating new objects anyways. And doing that (markDataParameterDirty) is Undefined Behaviour in MC's point of view, so there could be bugs that we havent discovered.. But still the bugs is kinda weird as it affect only at the init phrase. Immutability is not the cause because as i mention, we broke the rule so ...yeah.... And by the way, about the situation when a subsequent update something related to that changes, i think that system is the least that we can worry about, we would already have to change a lot which make that look like a cherry on the top of an ice cream.....