[Bug] Delta compression

I found a bug in the delta compression that you might wanna fix real soon ;)

The bug disallows all serialized data on certain observed objects from being read by receivers, that log in after the first non-master client user has joined the game.

Case:
You have a photonView in the scene, which is observing an object. Not all the data of the observed object is updated simultaneously, so some gets updated at one time, and some is updated at some other time.

The first non-MC user logs in (let's call him client A), and the MC starts to publish the data observed by a view X to client A as the data changes. On the MC, the first call to WriteSerializeData for view X sets the view.lastOnSerializeDataSent to non-null, starting the DeltaCompressionWrite for subsequent WriteSerializeData calls.

On client A, updates are received for view X, and everything goes according to plan.

Client B logs in, and the MC starts to publish the data observed by a view X to him. On client B at login, view.lastOnSerializeDataReceived will be null for view X until something has been received. However, nothing will ever be read as DeltaCompressionRead always returns false for view X, as not all of the observed data was changed at the same time, making the following code execute:
private void ReadOnSerialize(Hashtable data, PhotonPlayer sender)
{
...
    if (!DeltaCompressionRead(view, data))
    {
        // Skip this packet as we haven't got received complete-copy of this view yet.                
        DebugReturn(DebugLevel.INFO, "Skipping packet for "+view.name+" ["+view.viewID+"] as we haven't received a full packet for delta compression yet. This is OK if it happens for the first few frames after joining a game.");
        return; 
    }
...
}

Our problem occurred specifically for an observed transform, where only position should be synchronized, as it was the only thing that changed. However, PUN does the following, when writing the updated transform to its data array:
if (view.onSerializeTransformOption == OnSerializeTransform.OnlyRotation
    || view.onSerializeTransformOption == OnSerializeTransform.PositionAndRotation
    || view.onSerializeTransformOption == OnSerializeTransform.All)            
        data.Add(trans.localRotation);            
else            
    data.Add(null);

As null is always equal null, DeltaCompressionWrite() will always populate the compressedFields with at least one value (being null), causing the following to happen:
private bool DeltaCompressionRead(PhotonView view, Hashtable data)
    {        
        if (data.ContainsKey((byte)2))
        {   //Compression was applied as [(byte)2] exists

            if (view.lastOnSerializeDataReceived == null)
                return false; //We dont have a full match yet, we cannot work with missing values: skip this message
...

Thereby, the object will never has its data serialized on client B or any other subsequent client. The problem doesn't just occur for transforms, but for all objects that don't update everything simultaneously, or never updates some values that are observed.

I hope this feedback was helpful.

Comments

  • Be happy that this isn't fully Unity Networking standards. There you need to serialize data of fixed length. Any change in length results in ignorance.

    As for your issue: I'm pretty sure NULL writting is actually a bug on your end.
    You should never write null. If you don't want to write anything, then don't write it but do not write something with undefined length as that breaks any kind of delta algorithm.
    If there is no update then simply write the old data again, that will serve the same purpose while complying to the requirements too.
  • As for your issue: I'm pretty sure NULL writting is actually a bug on your end.

    This piece of code is taken from NetworkingPeer
    else if (type == typeof(Transform))
            {
                Transform trans = (Transform)view.observed;
                List<object> data = new List<object>();
                if (view.onSerializeTransformOption == OnSerializeTransform.OnlyPosition
                    || view.onSerializeTransformOption == OnSerializeTransform.PositionAndRotation
                    || view.onSerializeTransformOption == OnSerializeTransform.All)            
                    data.Add(trans.localPosition);            
                else            
                    data.Add(null);          
    
                if (view.onSerializeTransformOption == OnSerializeTransform.OnlyRotation
                    || view.onSerializeTransformOption == OnSerializeTransform.PositionAndRotation
                    || view.onSerializeTransformOption == OnSerializeTransform.All)            
                    data.Add(trans.localRotation);            
                else            
                    data.Add(null);            
    
                if (view.onSerializeTransformOption == OnSerializeTransform.OnlyScale
                    || view.onSerializeTransformOption == OnSerializeTransform.All)            
                    data.Add( trans.localScale);
                
                evData[(byte)1] = data.ToArray();
            }
    

    So, it's a PUN problem...
  • Oh missed that this is from PUN itself as you mentioned that mentioned that you update stuff at times only which meant that you use OnSerializePhotonView and no longer the observe automagic which would lead to the code path shown in the first block. Should have reread it I again.

    Sorry for causing blubb here
  • Note: While I was typing this post Tobias told me that you figured out that this was a problem for scene views, where we don't properly reset them as I listed under 2. I'll leave this post here to provide some docs on how this stuff all (should) work. It looks like the fix your your bug is an easy one now :)

    Dear Jakob,

    I'm sorry you had to dig trough that code, my fault for it being kinda shady and undocumented.
    What you described is the the cause of the problem as far as I understand:

    1) I tried reproducing this problem by modifying the demoworker to synch the transforms positions-only. This worked.
    Sending 'null' is perfectly fine, the comparisation for null that you quoted is to check whether we have had at least 1 full update:
    "Hashtable lastOnSerializeDataReceived = null; ". It is not checkind for 1 specific value.
    This hashtable will be filled with values (and even if those values are null, the hashtable will still be valid)

    2) When a new player connects, EVERY existing client will wipe it's local "lastOnSerializeDataSent" so that it will send a FULL update to all existing clients and most importantly, the newly connected client. This ensures that that new client will have filled it's "lastOnSerializeDataReceived" hashtable with valid data to enable compression. This is done via "NetworkingPeer.ResetPhotonViewsOnSerialize() Line 418"
  • Meanwhile, we got a confirmation that the fix is working. It will get into the next PUN update (v1.15).
    Until then, you can fix it by simply replacing PhotonNetworkingPeer.ResetPhotonViewsOnSerialize with:
    private void ResetPhotonViewsOnSerialize()
    {
        foreach (PhotonView photonView in this.photonViewList.Values)
        {
            photonView.lastOnSerializeDataSent = null;
        }
    }
    

    Thank you, Jakob, for your help. Much appreciated.
  • EDIT: Tobias was faster than me, damnit (with an even simpler solution) ;-p
    2) When a new player connects, EVERY existing client will wipe it's local "lastOnSerializeDataSent" so that it will send a FULL update to all existing clients and most importantly, the newly connected client. This ensures that that new client will have filled it's "lastOnSerializeDataReceived" hashtable with valid data to enable compression. This is done via "NetworkingPeer.ResetPhotonViewsOnSerialize() Line 418"

    As you said, we caught the bug. In the current PUN, only instantiated views have their lastOnSerializeDataSent reset, when new players join. This explained why we had spawed AIs walking around for the second non-MC player logging in, but had no moving doors or elevators in our scene. ;)

    NetworkingPeer::ResetPhotonViewsOnSerialize can be changed to the following to make the update work for scene views as well.
    private void ResetPhotonViewsOnSerialize()
        {
            foreach (KeyValuePair<int, PhotonView> kvp in this.photonViewList)
            {
                PhotonView view = kvp.Value;
                if (view.owner == this.mLocalActor || (PhotonNetwork.isMasterClient && view.owner == null))
                {
                    view.lastOnSerializeDataSent = null;
                }
            }
        }