FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.3k stars 136 forks source link

SyncVar on change events not fired for host when observer #733

Closed wooolly closed 3 weeks ago

wooolly commented 1 month ago

General Unity version: 2022.3.38f1 Fish-Networking version: 4.3.8R PRO Discord link: https://discord.com/channels/424284635074134018/1034477094731784302/1265734923075981363

Description Since upgrading from 4.1.0R PRO, my game has exhibited game-breaking behaviour, and after debugging, I have identified the issue to be with the host (server + client) and SyncVars. I use a GridCondition and SceneCondition in my observer manager, but the host has always been exempt from this in the past since the server needs to know everyone's state. Now, however, it seems as though when a client goes far away from the host player (outside the host's HashGrid observer distance) the host no longer receives SyncVar updates for that far away client, which is leading to big issues. Even tried disabling the "Update Host Visibility" options in the observer manager and network observer components, with no success.

Replication Steps to reproduce the behavior:

  1. Setup scene by... creating a simple plater prefab with a simple NetworkBehaviour script containing a SyncVar (Vector3 in my case) and an OnChanged listener which logs debug messages. Add GridCondition and SceneCondition to the observer manager.
  2. Start server and client (running as host) in Unity.
  3. Start a separate built player and connect to same server as client.
  4. Make client run far enough away from the host player.
  5. Notice the logs will stop generating for the client when they are far away from our host.

Expected behavior In 4.1.0R PRO, the server/host would always receive SyncVar OnChanged events for all players - this is how it should be, the server needs to be able to correctly maintain state for all players. Since 4.3.8R PRO (and likely earlier versions, I only update every so often) this has not been the case.

Screenshots N/A.

FirstGearGames commented 1 month ago

Could not replicate.

Steps:

FirstGearGames commented 1 month ago

SyncVar Callback Host.zip Example attached.

wooolly commented 1 month ago

Firstly, thank you for the example, it was useful for testing further, appreciated.


I have got to the bottom of it. Apologies, some more information has come to light.

I debugged the OnChange code inside the fishnet SyncVar script, and noticed that the OnChange was null when out of observable range. The change which has caused me issues isn't with SyncVar's or network observers, but with the OnStartNetwork/OnStopNetwork methods! This is where I'm registering/unregistering the OnChange event. If you change the example code to do the same, you should see the same behaviour. OnStartNetwork/OnStopNetwork is not called based on observer status, even for the host, which can't have been the case for 4.1.0 and before.

So, I was unregistering from the event without knowing, however, I would still constitute this as a breaking change in Fishnet. Something has changed between 4.1.0 and 4.3.8 regarding OnStopNetwork/OnStartNetwork, because the OnChange event did not register/unregister based on observer status before.

Going forward, I'm not sure of the resolution. Clients and the server alike rely on the same OnChange handler, hence why I had it in OnStartNetwork (so that it wasn't registered twice for host). I guess Awake/OnDestroy will produce expected results, since the server doesn't destroy them, and will still only be called once - is this the recommended approach for handling OnChange? Thanks for your assistance Mr FirstGearGames :)

FirstGearGames commented 1 month ago

The bug is OnStopNetworking is calling when it should not be, because the server is still active.

This is a bug not related so SyncVars, but none the less a bug of great importance. This will be resolved and an update pushed ASAP.

FirstGearGames commented 1 month ago

Here's the resolution. Look for these methods and replace with provided.

        private void InvokeStartCallbacks(bool asServer, bool invokeSyncTypeCallbacks)
        {
            /* Note: When invoking OnOwnership here previous owner will
             * always be an empty connection, since the object is just
             * now initializing. */

            //Invoke OnStartNetwork.
            bool invokeOnNetwork = (asServer || IsServerOnlyStarted || IsClientOnlyInitialized);
            if (invokeOnNetwork)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].InvokeOnNetwork(start: true);
            }

            //As server.
            if (asServer)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStartServer_Internal();
                _onStartServerCalled = true;
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnOwnershipServer_Internal(FishNet.Managing.NetworkManager.EmptyConnection);
            }
            //As client.
            else
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStartClient_Internal();
                _onStartClientCalled = true;
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnOwnershipClient_Internal(FishNet.Managing.NetworkManager.EmptyConnection);
            }

            if (invokeSyncTypeCallbacks)
                InvokeOnStartSyncTypeCallbacks(true);

            InvokeStartCallbacks_Prediction(asServer);
        }
        internal void InvokeStopCallbacks(bool asServer, bool invokeSyncTypeCallbacks)
        {
            InvokeStopCallbacks_Prediction(asServer);

            if (invokeSyncTypeCallbacks)
                InvokeOnStopSyncTypeCallbacks(asServer);

            bool clientStartCalled = _onStartClientCalled;

            if (asServer && _onStartServerCalled)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStopServer_Internal();
                _onStartServerCalled = false;
            }
            else if (!asServer && _onStartClientCalled)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStopClient_Internal();
                _onStartClientCalled = false;
            }

            bool invokeOnNetwork = ((!asServer && clientStartCalled) || IsServerOnlyStarted);
            if (invokeOnNetwork)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].InvokeOnNetwork(false);
            }
        }
wooolly commented 1 month ago

Hi FirstGearGames, I'm glad we could identify and fix a bug of importance! Thanks for the fix, I will try tomorrow, unfortunately it is late for me now.

wooolly commented 1 month ago

Apologies, but I tried the resolution you provided, and the bug seems to persists. I'm fairly confident I placed it in the right location, I even did it twice, once for my own project and once for the example replicable project you provided, but in both instances the OnChange event unregistered due to OnStopNetwork being fired.

wooolly commented 1 month ago

Hey @FirstGearGames did you modify the example project to use OnStartNetwork and OnStopNetwork to register/unregister the events, did the resolution work for you? :) As I say, I'm pretty confident I put the code in correctly, but haven't seen any difference yet sadly.

FirstGearGames commented 1 month ago

I'll test again before releasing 4.4.0

FirstGearGames commented 1 month ago

Hey @FirstGearGames did you modify the example project to use OnStartNetwork and OnStopNetwork to register/unregister the events, did the resolution work for you? :) As I say, I'm pretty confident I put the code in correctly, but haven't seen any difference yet sadly.

Yeah, it's still being weird. Taking another look!

FirstGearGames commented 1 month ago

Looks like the 'InvokeStopCallbacks' changes I provided had a flipped xor check. Here's the resolved code.

        internal void InvokeStopCallbacks(bool asServer, bool invokeSyncTypeCallbacks)
        {
            InvokeStopCallbacks_Prediction(asServer);

            if (invokeSyncTypeCallbacks)
                InvokeOnStopSyncTypeCallbacks(asServer);

            bool clientStartCalled = _onStartClientCalled;

            if (asServer && _onStartServerCalled)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStopServer_Internal();
                _onStartServerCalled = false;
            }
            else if (!asServer && _onStartClientCalled)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].OnStopClient_Internal();
                _onStartClientCalled = false;
            }

            bool invokeOnNetwork = asServer || (clientStartCalled && IsClientOnlyStarted);
            if (invokeOnNetwork)
            {
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].InvokeOnNetwork(false);
            }
        }
FirstGearGames commented 1 month ago

Once again, really resolved in 4.4.0

wooolly commented 4 weeks ago

Hi FirstGearGames, thanks for reporting back and updating Fishnet.

I've tried v4.4.1 pro, but I must confess that I'm still experiencing weird issues so the behaviour seems different to before.

For my game, I have NPCs, and I have my host player (in production, it'll be just a dedicated server without a host client, but for easy testing I want to run the server as a client too). My game registers to syncvar change events which then creates/updates UI over said networked NPC, but now they don't have UI because the syncvar change event is only called when NPC is first spawned, but my host hasn't connected at that point so the UI isn't created then. Once host connects, syncvar isn't fired when going into the observerable range. The events are registered as expected thanks to your previous fix (OnStop/OnStartNetwork behaves as before I believe).

Before, syncvar change events were always fired when we became an observer of them, even as host. This is ideal behaviour in my opinion, as it means that you can replicate the same sort of behaviour a client would experience, even though you're running as host and therefore technically know about their state even when they're outside of the observable range.

I hope this makes sense, sorry for being a constant pain.

wooolly commented 3 weeks ago

I've debugged a little more and this seems to be the behaviour I'm experiencing:

  1. Start server
  2. NPCs spawn and fire SyncVar change events (asServer true only)
  3. Connect as client/host
  4. Player spawns and fires SyncVar change events (asServer true and false)
  5. Player doesn't receive any SyncVar change events for existing NPCs, even when getting within observer range.
  6. A new NPC spawns far away and fires SyncVar change events (asServer true only)
  7. Player runs within observer range of new NPC, and receives SyncVar change events for NPC (asServer false)

This is different to how it used to work. The problem seems to be at point number 5, existing networked objects that spawn before the host don't seem to send asServer=false when the host gets close to them. I have not tested separate client/server yet.

In FishNet 4.1.0 (which I used for my game's early stages of development), this was not an issue. NPCs still sent asServer=false SyncVar change events when the host joined and got near to them.

wooolly commented 3 weeks ago

Hello @FirstGearGames

I've taken some time to produce a repro project to demonstrate the issue, please can you take a look when you get some free time. https://1drv.ms/u/c/ef45518805503c08/EQos0qBKPelOgUSX_ZN-XAkBlc4FxUHLkX-8jLiLVFVW_w?e=zehZtX - shared via OneDrive

The example demonstrates what I've tried to describe above, how it fails to network syncvars properly. This is using 4.4.1R PRO.

Steps

  1. Launch game
  2. Start server, notice that an NPC spawns
  3. A syncvar is assigned a random int in the NPC. On change is fired asServer=true only, as expected.
  4. Wait 20 seconds and another random int will be assigned, note that prev is still 0!!! (bug also!?)
  5. Connect as client, notice that player syncvar is assigned random int. On change is fired both asServer=true and asServer=false as expected.
  6. Note, no NPC syncvar fired when we connected!
  7. Wait for NPC to set syncvar again, note that it'll still be prev: 0, but at least asServer=false will fire now too.

What I think is happening:

The prev:0 remains zero until we connect as host, so I believe that the server is struggling to properly update syncvars when not within some sort of observable range to host? As soon as we connect, it begins to correct itself but only once syncvar is set again.

FirstGearGames commented 3 weeks ago

Previous will and should be 0 when the object spawns and gets it's first callback. The client is not aware of the previous value until they can observe the object.

I ran a bunch of tests on my live stream today and they all passed. It's worth noting I did just finish refactoring the spawn message and syncvars though, so it's possible that resolved the issue.

If you have access to the Pro repo you can pull a7e556f under the dev branch to tests. Note the notes before pulling.

wooolly commented 3 weeks ago

Yeah when it first spawns, but not every time until the host connects? Right? Bearing in mind this is for asServer=true so the server should always know what the latest syncvar value is.

Oh that's nice, where can I watch this livestream? (edit: found it now)

Thanks for letting me know of the latest changes. Sadly I can't find the Pro repo, I'm only on the updates tier right now (I'm poor) so perhaps I don't have access to it. I'm looking forward to testing the latest release to see whether the latest refactorings have fixed the issues I'm experiencing, any ETA on release :)? Thanks!

FirstGearGames commented 3 weeks ago

If the object is despawned, the value is reset.

If it's despawned for client, it's reset to 0 for client. If it's despawned on server, then 0 for server.

wooolly commented 3 weeks ago

It was never despawned on server, yet the server previous value was always 0.

FirstGearGames commented 3 weeks ago

Okay. I've not been seeing that in the current dev branch so let's see what happens after release.

On Thu, Aug 15, 2024, 9:37 AM Dominic Edhouse @.***> wrote:

It was never despawned on server, yet the server previous value was always 0.

— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishNet/issues/733#issuecomment-2291282776, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3QFV2UUTS4CDXGLXOTZRSVJTAVCNFSM6AAAAABLN4KDR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJRGI4DENZXGY . You are receiving this because you modified the open/close state.Message ID: @.***>

wooolly commented 1 week ago

All seems to be fixed, behaviour is consistent again, thank you for fixing.