Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NetworkClient): Defer ApplySpawnPayload until OnObjectSpawnFinished #3968

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MrGadget1024
Copy link
Collaborator

@MrGadget1024 MrGadget1024 commented Jan 12, 2025

This PR works when there are less than 20 objects, but fails when there are more than 20.

  • Benchmark example fails with OnDeserialize warnings and disconnects with the error below.
  • I checked the PR with several other examples and they're all fine, including Multiple Matches which spawns about 16 objects to client.
  • subsequent spawning and unspawning via interest management is fine (Scene / Distance / Spatial - all fine).
  • repeated StopClient - > StartClient in other examples is also fine.
  • The warnings and errors clearly point to some sort of reader position pointer problem.
  • SpawnMessage contains public ArraySegment<byte> payload; and that seems to be where the reader is getting lost, but SpawnMessage is a struct, and thus a value type, so I can't really make sense of why the payload isn't fully intact, unless it's not being written correctly.
if (message.payload.Count > 0)
{
    using (NetworkReaderPooled payloadReader = NetworkReaderPool.Get(message.payload))
    {
        identity.DeserializeClient(payloadReader, true);
    }
}

image

Reason: System.IO.EndOfStreamException: ReadBlittable<System.Byte> not enough data in buffer to read 1 bytes: [3F-10-03-0C-8E-04-63-41-00-00-00-00-8E-04-63 @ 18/15]
  at Mirror.NetworkReader.ReadBlittable[T] () [0x00047] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkReader.cs:142 
  at Mirror.NetworkReader.ReadByte () [0x00000] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkReader.cs:182 
  at Mirror.NetworkBehaviour.Deserialize (Mirror.NetworkReader reader, System.Boolean initialState) [0x00003] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkBehaviour.cs:1306 
  at Mirror.NetworkIdentity.DeserializeClient (Mirror.NetworkReader reader, System.Boolean initialState) [0x0002c] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkIdentity.cs:1128 
  at Mirror.NetworkClient.ApplySpawnPayload (Mirror.NetworkIdentity identity, Mirror.SpawnMessage message) [0x000f9] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkClient.cs:1150 
  at Mirror.NetworkClient.OnObjectSpawnFinished (Mirror.ObjectSpawnFinishedMessage _) [0x0005f] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkClient.cs:1328 
  at (wrapper delegate-invoke) System.Action`1[Mirror.ObjectSpawnFinishedMessage].invoke_void_T(Mirror.ObjectSpawnFinishedMessage)
  at Mirror.NetworkClient+<>c__DisplayClass61_0`1[T].<RegisterHandler>g__HandlerWrapped|0 (Mirror.NetworkConnection _, T value) [0x00000] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkClient.cs:543 
  at Mirror.NetworkMessages+<>c__DisplayClass9_0`2[T,C].<WrapHandler>g__Wrapped|0 (C conn, T msg, System.Int32 _) [0x00000] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkMessages.cs:206 
  at Mirror.NetworkMessages+<>c__DisplayClass8_0`2[T,C].<WrapHandler>b__0 (Mirror.NetworkConnection conn, Mirror.NetworkReader reader, System.Int32 channelId) [0x000cf] in C:\Unity\Mirror2023\Assets\Mirror\Core\NetworkMessages.cs:179

Original PR Description

==================
Virtually eliminates null refs with GO/NI/NB SyncVars by getting all objects into spawned dictionary before applying payloads and invoking hooks.

  • No change to normal spawning, only initial spawn
  • Uses and clears a static dictionary, so no garbage allocation

See ticket #3097

Test with PickupsDropsChilds example:

  • Added SyncList to PickupsDropsChilds on player for everything player drops
  • Added SyncVar to SceneObject for owner player object
  • Started with built host client, dropped 3 things, then joined client from editor
  • Depending on spawn order, either the SyncVar hook or the SyncList OnAdd handler would've thrown NRE's
public readonly SyncList<GameObject> drops = new SyncList<GameObject>();

public override void OnStartClient()
{
    drops.OnAdd += OnAdd;
    for (int i = 0; i < drops.Count; i++)
        drops.OnAdd?.Invoke(i);
}

void OnAdd(int index)
{
    Debug.Log($"OnAdd {index} {drops[index].name}");
}

[Command]
void CmdDropItem()
{
    . . .
    // set owner object
    sceneObject.owner = gameObject;
    . . .
    // Spawn the scene object on the network for all to see
    NetworkServer.Spawn(newSceneObject);

    drops.Add(newSceneObject);
}
[ReadOnly, SyncVar(hook = nameof(OnOwnerChanged))]
public GameObject owner;

void OnOwnerChanged(GameObject _, GameObject newOwner)
{
    Debug.Log($"SyncVar hook OnOwnerChanged {owner.name}", owner);
}

Before this PR

NullReferenceException: Object reference not set to an instance of an object
Mirror.Examples.PickupsDropsChilds.PickupsDropsChilds.OnAdd (System.Int32 index) (at Assets/Mirror/Examples/PickupsDropsChilds/Scripts/PickupsDropsChilds.cs:40)
Mirror.Examples.PickupsDropsChilds.PickupsDropsChilds.OnStartClient () (at Assets/Mirror/Examples/PickupsDropsChilds/Scripts/PickupsDropsChilds.cs:35)
Mirror.NetworkIdentity.OnStartClient () (at Assets/Mirror/Core/NetworkIdentity.cs:757)
UnityEngine.Debug:LogException(Exception, Object)
Mirror.NetworkIdentity:OnStartClient() (at Assets/Mirror/Core/NetworkIdentity.cs:761)
Mirror.NetworkClient:InvokeIdentityCallbacks(NetworkIdentity) (at Assets/Mirror/Core/NetworkClient.cs:1398)
Mirror.NetworkClient:BootstrapIdentity(NetworkIdentity) (at Assets/Mirror/Core/NetworkClient.cs:1371)

After the PR, all refs are valid.

image

MrGadget1024 and others added 3 commits December 29, 2024 14:04
Virtually eliminates null refs with GO/NI/NB SyncVars by getting all objects into spawned dictionary before applying payloads and invoking hooks.
- No change to normal spawning, only initial spawn
- Uses and clears a static dictionary, so no garbage allocation

See ticket #3097
@MrGadget1024 MrGadget1024 added enhancement New feature or request work in progress Need more time to decide. Nothing to do here for now. labels Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request work in progress Need more time to decide. Nothing to do here for now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants