FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.24k stars 138 forks source link

Destroy / Disable nob bug #649

Open SnaiperoG3D opened 3 months ago

SnaiperoG3D commented 3 months ago

General Unity version: 2021.3.30f1 Fish-Networking version: 4.2.0 Pro Discord link: https://discord.com/channels/424284635074134018/1034477094731784302/1228664103296303166

Description After update my main project from 3.x version to 4.2.0, i start to have weird thing. I have items and enemies in game. Enemies nob started to not despawn. I faced situation when ResetState for SyncVars called, but gameobject nob not disabled (enemies nobs pooled, but also true for simple destroy). Ive found that it happens only at Host. And idk how to replicate it, because items nobs working fine. I did expose some values from ManagedObjects.cs and log them to console. just added 2 vars for tracking NetworkManager.ServerManager.Objects.AddToPending(nob); and NetworkManager.ServerManager.Objects.RemoveFromPending(nob.ObjectId); code: Debug.Log($"destroy: {destroy}, nested: {nob.IsNested}, asServer: {asServer}, IsSceneObject: {nob.IsSceneObject}, AddToPending: {addToPending}, RemoveFromPending: {removeFromPending}"); here is a log of this issue: destroy: False, nested: False, asServer: False, IsSceneObject: False, AddToPending: False, RemoveFromPending: False This log calling once (edited) per each nob.

I checked how this log working in normal case, and it should have asServer true, or AddToPending true, and RemoveFromPending true on next call, but it never happens in Host for some reason

Expected behavior nobs disabling or destroying depends of pool or derstroy setting

Screenshots Video

SnaiperoG3D commented 2 months ago

still happens in 4.2.2, which prevents it from working properly in single player

SnaiperoG3D commented 2 months ago

new issue in 4.2.2. i believe both somehow connected to each other NRE in NetworkObject.cs:842 appears as previos one, when object should be disabled or destroyed

UPD: ClientManager for somereason in this row is null I have a feeling that because Despawn method calling twice as client on host, and as a result, resetstate method calling twice, and when 842 line called, ClientManager was reseted via resetstate method, and we got NRE

UPD 2: for some reason this issue appears not for all nobs. ive placed some logs to know asServer is true or not, and ClientManager is null or not. For some reason for enemies nobs, i have asServer as false called twice for each enemy, but ClientManager not a null, but theses nobs never despawn via first issue, so i cant get objective information about it

FirstGearGames commented 2 months ago

A recent callback fix was applied for this https://github.com/FirstGearGames/FishNet/issues/652

I'll send you an update via DM to try. Let me know if it fixes.

SnaiperoG3D commented 1 month ago

Duplicate from Discord. No update not resolve the issue, also i have a little more info about it. I believe that it related. There are 2 errors happens when despawn should occur: NullReferenceException: Object reference not set to an instance of an object FishNet.Object.NetworkObject.Deinitialize (System.Boolean asServer) (at Assets/FishNet/Runtime/Object/NetworkObject.cs:858) FishNet.Managing.Object.ManagedObjects.Despawn (FishNet.Object.NetworkObject nob, FishNet.Object.DespawnType despawnType, System.Boolean asServer) (at Assets/FishNet/Runtime/Managing/Object/ManagedObjects.cs:162) FishNet.Managing.Client.ClientObjectCache.IterateDespawn (FishNet.Managing.Client.CachedNetworkObject cnob) (at Assets/FishNet/Runtime/Managing/Client/Object/ObjectCaching.cs:495) FishNet.Managing.Client.ClientObjectCache.<Iterate>g__ProcessObject|14_0 (FishNet.Managing.Client.CachedNetworkObject cnob, System.Boolean spawn, System.Int32 index, FishNet.Managing.Client.ClientObjectCache+<>c__DisplayClass14_0& ) (at Assets/FishNet/Runtime/Managing/Client/Object/ObjectCaching.cs:411) FishNet.Managing.Client.ClientObjectCache.Iterate () (at Assets/FishNet/Runtime/Managing/Client/Object/ObjectCaching.cs:279) FishNet.Managing.Client.ClientObjects.IterateObjectCache () (at Assets/FishNet/Runtime/Managing/Client/Object/ClientObjects.cs:495) FishNet.Managing.Client.ClientManager.ParseReader (FishNet.Serializing.PooledReader reader, FishNet.Transporting.Channel channel, System.Boolean print) (at Assets/FishNet/Runtime/Managing/Client/ClientManager.cs:539) FishNet.Managing.Client.ClientManager.ParseReceived (FishNet.Transporting.ClientReceivedDataArgs args) (at Assets/FishNet/Runtime/Managing/Client/ClientManager.cs:384) FishNet.Managing.Client.ClientManager.Transport_OnClientReceivedData (FishNet.Transporting.ClientReceivedDataArgs args) (at Assets/FishNet/Runtime/Managing/Client/ClientManager.cs:342) FishNet.Transporting.Tugboat.Tugboat.HandleClientReceivedDataArgs (FishNet.Transporting.ClientReceivedDataArgs receivedDataArgs) (at Assets/FishNet/Runtime/Transporting/Transports/Tugboat/Tugboat.cs:238) FishNet.Transporting.Tugboat.Client.ClientSocket.IterateIncoming () (at Assets/FishNet/Runtime/Transporting/Transports/Tugboat/Core/ClientSocket.cs:295) FishNet.Transporting.Tugboat.Tugboat.IterateIncoming (System.Boolean server) (at Assets/FishNet/Runtime/Transporting/Transports/Tugboat/Tugboat.cs:211) FishNet.Managing.Transporting.TransportManager.IterateIncoming (System.Boolean server) (at Assets/FishNet/Runtime/Managing/Transporting/TransportManager.cs:705) FishNet.Managing.Timing.TimeManager.TryIterateData (System.Boolean incoming) (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:1017) FishNet.Managing.Timing.TimeManager.IncreaseTick () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:670) FishNet.Managing.Timing.TimeManager.<TickUpdate>g__MethodLogic|98_0 () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:342) FishNet.Managing.Timing.TimeManager.TickUpdate () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:332) FishNet.Transporting.NetworkReaderLoop.Update () (at Assets/FishNet/Runtime/Transporting/NetworkReaderLoop.cs:28)

NullReferenceException: Object reference not set to an instance of an object FishNet.Component.Transforming.NetworkTransform.TimeManager_OnPostTick () (at Assets/FishNet/Runtime/Generated/Component/NetworkTransform/NetworkTransform.cs:947) FishNet.Managing.Timing.TimeManager.IncreaseTick () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:688) FishNet.Managing.Timing.TimeManager.<TickUpdate>g__MethodLogic|98_0 () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:342) FishNet.Managing.Timing.TimeManager.TickUpdate () (at Assets/FishNet/Runtime/Managing/Timing/TimeManager.cs:332) FishNet.Transporting.NetworkReaderLoop.Update () (at Assets/FishNet/Runtime/Transporting/NetworkReaderLoop.cs:28)

SnaiperoG3D commented 1 month ago

I guess i know where happens last issue. I have spawning system, when player goes too far away, it despawn all spawned nob by this system, and its worked in v3 and wokring in v4 on server, but not on host. So basically i guess nre appears on host in moment, when nob was despawned via distance condition, then spawning system detect that player far away and do its own despawn, and as a result despawn nob twice, and i believe that there is no protect from twice despawn call in fish v4, cuz ClientManager is null, and null setted in ResetState method. Gonna try to make example project, but still cant understand why nobs not disabling at topic start description bug ps testing on 4.3.3Pro now

SnaiperoG3D commented 1 month ago

ok, well, i didnt find the point of the problem, but i believe i did find a temp fix. I did a wrap for ClientManager for null check in NetworkObject.cs script at 858 line

                if(ClientManager != null)
                {
                    Dictionary<NetworkObject, NetworkConnection.LevelOfDetailData> currentLods = ClientManager.Connection.LevelOfDetails;
                    if (currentLods.TryGetValue(this, out NetworkConnection.LevelOfDetailData lodData))
                        ObjectCaches<NetworkConnection.LevelOfDetailData>.Store(lodData);
                    ClientManager.Connection.LevelOfDetails.Remove(this);

                    //Client only.
                    if (!NetworkManager.IsServerStarted)
                        IsDeinitializing = true;

                    RemoveClientRpcLinkIndexes();
                }

Not despawn / destroy fix still in process, cuz idk the internal fish logic

SnaiperoG3D commented 1 month ago

Well. I have some moe info about it. I did inverstigate into fish code and found something interesting. Need to say about bug, whats happening again: some nobs go not disabling, but syncvars reseted. I thought why its happening not for all nobs, cuz i have items and enemies, with items everything is ok, but enemies go are not disabling. After temp bug fix i found that it happens only with items, because of despawning system, and i did compare with enemy despawn system, it works in different way. If items despawning almost at same time as distance condition despawn it, enemies despawning in another way. This logic has 10 sec cooldown. I did compare last fish NetworkObject.cs revision with some older versions in my VC, and ive found next: new one:

        /// <summary>
        /// Called to prepare this object to be destroyed or disabled.
        /// </summary>
        internal void Deinitialize(bool asServer)
        {
#if !PREDICTION_1
            Prediction_Deinitialize(asServer);
#endif
            InvokeStopCallbacks(asServer, true);
            for (int i = 0; i < NetworkBehaviours.Length; i++)
                NetworkBehaviours[i].Deinitialize(asServer);

            if (asServer)
            {
                NetworkObserver?.Deinitialize(false);
                IsDeinitializing = true;
            }
            else
            {
                Dictionary<NetworkObject, NetworkConnection.LevelOfDetailData> currentLods = ClientManager.Connection.LevelOfDetails;
                if (currentLods.TryGetValue(this, out NetworkConnection.LevelOfDetailData lodData))
                    ObjectCaches<NetworkConnection.LevelOfDetailData>.Store(lodData);
                ClientManager.Connection.LevelOfDetails.Remove(this);
                //Client only.
                if (!NetworkManager.IsServerStarted)
                    IsDeinitializing = true;

                RemoveClientRpcLinkIndexes();
            }

            SetInitializedStatus(false, asServer);

            if (asServer)
                Observers.Clear();
        }

old one:

        /// <summary>
        /// Called to prepare this object to be destroyed or disabled.
        /// </summary>
        internal void Deinitialize(bool asServer)
        {
            InvokeStopCallbacks(asServer);
            if (asServer)
            {
                IsDeinitializing = true;
            }
            else
            {
                //Client only.
                if (!NetworkManager.IsServer)
                    IsDeinitializing = true;

                RemoveClientRpcLinkIndexes();
            }

            SetActiveStatus(false, asServer);
            if (asServer)
                Observers.Clear();
        }

We interested in this lines:

 for (int i = 0; i < NetworkBehaviours.Length; i++)
                NetworkBehaviours[i].Deinitialize(asServer);

They do reset SyncVars inside their logic.

So i believe whats happening: enemies despawning with DistanceCondition, but only for "client" (remember we talking about host, cuz bug only here), on "server" side enemies still exist, BUT this lines do reset for SyncVars, and when my spawning system want to despawn enemies, it catch NRE cuz it using SyncVar to identify enemy, but SyncVar already reseted. Thats why it happening only with enemies, cuz i dont use syncvars in items logic and go not setted to false, cuz enemies still exist on "server" side.

So as a result i believe that bug with Reset SyncVars on host for client, when distance condition worked, what do reset for server side, cuz it host.

SnaiperoG3D commented 1 month ago

Well, i guess ive found a temp fix for this bug too. I did wrap for these lines with checks for !asServer and for host. Idk is it break something or not, but now my games start to work in singleplayer atleast.

            //  exclude client on host
            if(!(!asServer && (NetworkManager == null || NetworkManager.IsHostStarted)))
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].Deinitialize(asServer);

UPD: These bugs are clearly interrelated, I decided to check both at the same time, now NRE occurs in the check for the host for items despawn, NetworkManager is null . For a temporary fix, I will also check for null NetworkManager.

            //  exclude client on host. NetworkManager == null appears only on host so it will == as NetworkManager.IsHostStarted
            if(!(!asServer && (NetworkManager == null || NetworkManager.IsHostStarted)))
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].Deinitialize(asServer);

UPD2: Edited code, previous check was wrong

FirstGearGames commented 1 month ago

Going off what you read I'm understanding the end point of your issue is that the SyncVar is resetting on the Deinitialize server logic, and by that clearing the value it's affecting the clientHost.

If this is correct can you explain or show me a little more how you are using the SyncVar in relation to what is causing your error?

PS: if I may make a recommendation of something to try as a temp fix instead of the route you took ... go into the SyncVar.cs file and change the ResetState method to this...


        internal protected override void ResetState(bool asServer)
        {
            base.ResetState(asServer);

            //Let clientHost reset if the object containing this syncvar is initialized for them.
            if (asServer && base.IsNetworkInitialized && NetworkBehaviour.IsClientInitialized)
                return;

            _value = _initialValue;
            _previousClientValue = _initialValue;
        }

Even if this fix works I'd like to see your usage specifically where the error is happening just to be sure nothing is getting missed.

SnaiperoG3D commented 1 month ago

No, this code doesnt help, plus SyncVar never reset now

FirstGearGames commented 2 weeks ago

This seems to be a very unique use-case issue. I'm having no luck seeing any obvious problem at this time and am not able to replicate. If you have this problem on 4.3.5+ can you please DM me on Discord so we can resolve it in real-time.

SnaiperoG3D commented 1 week ago

Got some info about it. Im developing another game, where i have same bug i guess and this bug releated to pool nobs (i believe). I have in this game simple logic, player can eat food, food have random value which ads to player score. There are lot of food on the map so i did prewarm food nob. So as a result after despawn, and respawn, food have 0 int syncvar instead of random value. my fix above fix the issue. https://pastebin.com/iBfGeKwA there are 3 scipts, food spawner, food, and player eater. maybe pool is not the point, but i have this bug with this simple logic. i have nothing custom in this project fish 4.3.5 Pro

SnaiperoG3D commented 1 week ago

when i done with this project i can try to replicate this bug in simple new project. cuz now i exaclty know that it happens with this simple logic

SnaiperoG3D commented 4 days ago

UPD: there is a final version of my fix. I reread this thread and my snippets may be not clear to understand


internal void Deinitialize(bool asServer)
        {
#if !PREDICTION_1
            Prediction_Deinitialize(asServer);
#endif
            InvokeStopCallbacks(asServer, true);
            if (!(!asServer && (NetworkManager == null || NetworkManager.IsHostStarted)))
                for (int i = 0; i < NetworkBehaviours.Length; i++)
                    NetworkBehaviours[i].Deinitialize(asServer);

            if (asServer)
            {
                NetworkObserver?.Deinitialize(false);
                IsDeinitializing = true;
            }
            else
            {
                if (ClientManager != null)
                {
                    Dictionary<NetworkObject, NetworkConnection.LevelOfDetailData> currentLods = ClientManager.Connection.LevelOfDetails;
                    if (currentLods.TryGetValue(this, out NetworkConnection.LevelOfDetailData lodData))
                        ObjectCaches<NetworkConnection.LevelOfDetailData>.Store(lodData);
                    ClientManager.Connection.LevelOfDetails.Remove(this);
                    //Client only.
                    if (!NetworkManager.IsServerStarted)
                        IsDeinitializing = true;

                    RemoveClientRpcLinkIndexes();
                }
            }

            SetInitializedStatus(false, asServer);

            if (asServer)
                Observers.Clear();
        }
FirstGearGames commented 1 day ago

What did you change to resolve your issue? You're welcome to DM me as well so we can troubleshoot this.