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

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

Closed
wooolly opened this issue Jul 25, 2024 · 21 comments
Closed

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

wooolly opened this issue Jul 25, 2024 · 21 comments
Labels
Bug Something isn't working Resolved Pending Release

Comments

@wooolly
Copy link

wooolly commented Jul 25, 2024

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
Copy link
Owner

Could not replicate.

Steps:

  • Open SyncVarCallbackHost scene.
  • Hit play (will start as clientHost).
  • See callback firing in console asServer true and false as expected.
  • Select Player object in hierarchy and move 10+ units away.
  • See callback firing asServer true only, as expected.

@FirstGearGames
Copy link
Owner

FirstGearGames commented Jul 25, 2024

SyncVar Callback Host.zip
Example attached.

@FirstGearGames FirstGearGames added Waiting For Information Not enough information has been supplied to resolve this issue. Acknowledged Your issue has been acknowledged and will be investigated. labels Jul 25, 2024
@wooolly
Copy link
Author

wooolly commented Jul 25, 2024

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
Copy link
Owner

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
Copy link
Owner

FirstGearGames commented Jul 25, 2024

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
Copy link
Author

wooolly commented Jul 25, 2024

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.

@FirstGearGames FirstGearGames added Bug Something isn't working Resolved Pending Release and removed Waiting For Information Not enough information has been supplied to resolve this issue. Acknowledged Your issue has been acknowledged and will be investigated. labels Jul 25, 2024
@wooolly
Copy link
Author

wooolly commented Jul 26, 2024

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
Copy link
Author

wooolly commented Jul 28, 2024

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
Copy link
Owner

I'll test again before releasing 4.4.0

@FirstGearGames
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

Once again, really resolved in 4.4.0

@wooolly
Copy link
Author

wooolly commented Aug 10, 2024

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
Copy link
Author

wooolly commented Aug 12, 2024

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
Copy link
Author

wooolly commented Aug 13, 2024

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
Copy link
Owner

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
Copy link
Author

wooolly commented Aug 15, 2024

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
Copy link
Owner

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
Copy link
Author

wooolly commented Aug 15, 2024

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

@FirstGearGames
Copy link
Owner

FirstGearGames commented Aug 15, 2024 via email

FirstGearGames pushed a commit that referenced this issue Aug 20, 2024
Changes in 4.4.2
- Fixed stackoverflow error on an editor script.
- Improved removed redundant explicit type casts.
- Changed refactor on spawning; new code, same behavior.
- Fixed package.json not specifying all dependencies (#719).
- Fixed NetworkTransform potentially applying eventual consistency to relayed client-authoritative datas.
- Fixed TimeManager.LastPacketTick not resetting on network stop (#748).
- Changed removed all MethodImplOptions.AggressiveInlining.
- Added TickNetworkBehaviour.OnUpdate/OnLateUpdate overrides.
- Added Benchmarks folder.
- Added NetworkTransform benchmark.
- Removed Network Level of Detail; it's being replaced with a better system.
- Improved reduced CPU usage for writing spawn messages.
- Improved reduced bandwidth for writing spawn messages.
- Fixed NetworkTransform _cachedTransform not being set when object is disabled.
- Improved NetworkObject automatically adds EmptyNetworkBehaviour is no NetworkBehaviours are present at runtime.
- Improved NetworkObject performance by removing Update loop.
- Improved NetworkTransform performance by removing Update loop.
- Improved NetworkAnimator performance by removing Update loop.

Changes in 4.4.1
- Fixed modified collection/stackOverflow caused during observer rebuild.

Changes in 4.4.0
- Added delta serializers for prediction. This is a beta feature, you may switch to Stable using the Fish-Networking menu.
- Fixed SyncVar not sending latest values when the networkObject is spawned and despawned on the same tick (#692).
- Improved SyncVar no longer resets values prior to client deinitializing the object.
- Improved NetworkTransform performance by caching transform (#742).
- Fixed OnStopNetwork invoking on clientHost when client lost observation, while server was still active (#733).
- Fixed PredictedSpawn.OnTrySpawnServer not calling (#725).
- Fixed failed PredictedSpawns not skipping remaining data on the spawn, potentially corrupting the packet remainder.
- Added Writer.InsertUInt16Unpacked.
- Fixed ColliderRollback assertion error when rolling back before 2 snapshots were created (#743).
- Improved removed garbage collection from ColliderRollback fields and types.
- Changed several exposed internal API renamed on NetworkObject.
- Added boolean includeNested for PredictedOwner as well NetworkObject Give/RemoveOwnership.
- Obsoleted several methods in favor of includeNested for ownership.
- Fixed prediction graphicalObject smoothing for spectators when state forwarding was disabled (#734).
- Fixed replicate being run on spectators when state forwarding was disabled; related to (#734).
- Improved TimeManager.TickToLocalTick and LocalTickToTick can now return future tick values (#736).
- Added NetworkObject.HasAuthority.
- Fixed sceneIds sometimes not generating until after script compile or using regenerate sceneId menu.
- Fixed nested NetworkObjects spawning at the wrong coordinates for clients (#738).
- Fixed networkObject.SetParent(null) causing NullreferenceException.
- Fixed OnStopNetwork invoking on clientHost when client lost observation, while server was still active.
@wooolly
Copy link
Author

wooolly commented Aug 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Resolved Pending Release
Projects
None yet
Development

No branches or pull requests

2 participants