miwarnec / DOTSNET

Public DOTSNET issue tracker
20 stars 0 forks source link

Sync-ing transform and component in certain direction throws deserialization error #49

Open Ro4m3r opened 3 years ago

Ro4m3r commented 3 years ago

A NetworkIdentity with transformDirection = SyncDirection.CLIENT_TO_SERVER and a NetworkComponent with SyncDirection = SyncDirection.SERVER_TO_CLIENT will throw a deserialization error along the lines of:

Deserialization: reading key failed for NetworkComponent key=4153 with reader BitPosition=0 RemainingBits=8 in NetworkComponentSerializer.cs:217

If there are troubles reproducing I can add a reproduction project.

Ro4m3r commented 3 years ago

Tagging @vis2k

Ro4m3r commented 3 years ago

Alright, I believe I figured out the issue.

The deserialization.reader is being filled with 8 bits because the block of code below rounds up to 8 bits even if the payload has no data. This happens if the client syncs only the transform, and nothing else, and the server runs the deserialization system. Even a single NetworkComponent synced by the client will correctly fill the buffer and let the deserialization system peek the key (data > 16 bits).

unsafe
{
    int sizeInBytes = Utils.RoundBitsToFullBytes(message.payloadBitSize);
    deserialization.reader = new BitReader128(message.payload, sizeInBytes);
}

Utils.RoundBitsToFullBytes will round 0 bits to 1 byte, causing the 8 bits that are mentioned in the error message.

The fix is to check if the payload is 0, and only then create the BitReader128.

if (message.payloadBitSize > 0)
{
    unsafe
    {
        int sizeInBytes = Utils.RoundBitsToFullBytes(message.payloadBitSize);
        deserialization.reader = new BitReader128(message.payload, sizeInBytes);

    }
}

I haven't checked if SnapshotClientMessageSystem has the same issue, but it would probably be better to check if the payload actually has data there too, just to be safe.

miwarnec commented 3 years ago

looking into this today. 'Utils.RoundBitsToFullBytes will round 0 bits to 1 byte' - sounds like you found a bug, will add a unit test for that first.

miwarnec commented 3 years ago

trying to reproduce your issue before the fix now so we can have a test

miwarnec commented 3 years ago

still can't reproduce.

miwarnec commented 3 years ago

I've added two tests for sync without networkcomponents now. they both pass.

Ro4m3r commented 3 years ago

To illustrate what the issue is:

Payload is 0. This is because the client is only sync-ing its transform and not the NetworkComponent, meaning the snapshot only contains the transform data and no (payload=0) serialization data.

sizeInBytes is incorrectly set to 1 because Utils.RoundBitsToFullBytes rounds 0 bits to 1 byte.

deserialization.reader is incorrectly given 8 remaining bits. This is what causes the peeking of the key (which is 16 bits long) to fail on the server.

Ro4m3r commented 3 years ago

Some further reproduction steps in case anybody wants to try and reproduce this:

Add the following script Health.cs

using DOTSNET;
using System.Collections;
using System.Collections.Generic;
using Unity.Entities;
using UnityEngine;

[GenerateAuthoringComponent]
public struct Health : NetworkComponent
{
    public int Value;

    public bool Deserialize(ref BitReader128 reader)
        => reader.ReadInt(out Value);

    public SyncDirection GetSyncDirection() => SyncDirection.SERVER_TO_CLIENT;

    public bool Serialize(ref BitWriter128 writer)
        => writer.WriteInt(Value);
}

public class HealthSerializer : NetworkComponentSerializer<Health> { }

And add the Health component to the Player prefab in the Benchmark scene.

Then start the server and connect the client.