From 98a0e234136d7bc1671e3eb802145d63ba21e60c Mon Sep 17 00:00:00 2001 From: vis2k Date: Fri, 8 Mar 2019 20:24:18 +0100 Subject: [PATCH] Persistent sceneid (#563) * add empty line * persistent scene ids * never assign scene ids at runtime * fix warning * fix 'sceneid is never assigned to' warning in play mode * simplify bitwise operation * set dirty even if only scene index byte changed * improve log message * assign build index byte in OnPostProcessScene to also work with unopened scenes and build index changes * add comment * improve log * Add sceneid not set check * Only PostProcess the objects from this scene. * Interrupt build if there is an unopened scene that still needs sceneId assignments. This makes it 100% fail safe. * improve comment and error message * clear build index byte before or-ing into it --- .../Editor/NetworkInformationPreview.cs | 2 +- .../Mirror/Editor/NetworkScenePostProcess.cs | 190 +++--------------- Assets/Mirror/Runtime/NetworkIdentity.cs | 121 ++++++++++- 3 files changed, 139 insertions(+), 174 deletions(-) diff --git a/Assets/Mirror/Editor/NetworkInformationPreview.cs b/Assets/Mirror/Editor/NetworkInformationPreview.cs index 579e01e3b..997d6347f 100644 --- a/Assets/Mirror/Editor/NetworkInformationPreview.cs +++ b/Assets/Mirror/Editor/NetworkInformationPreview.cs @@ -215,7 +215,7 @@ void GetNetworkInformation(GameObject gameObject) m_Info = new List { GetAssetId(), - GetString("Scene ID", m_Identity.sceneId.ToString()) + GetString("Scene ID", m_Identity.sceneId.ToString("X")) }; if (!Application.isPlaying) diff --git a/Assets/Mirror/Editor/NetworkScenePostProcess.cs b/Assets/Mirror/Editor/NetworkScenePostProcess.cs index 6e0edfdff..6e77bb6b2 100644 --- a/Assets/Mirror/Editor/NetworkScenePostProcess.cs +++ b/Assets/Mirror/Editor/NetworkScenePostProcess.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.Linq; using UnityEditor; using UnityEditor.Callbacks; @@ -9,168 +8,20 @@ namespace Mirror { public class NetworkScenePostProcess : MonoBehaviour { - // persistent sceneId assignment to fix readstring bug that occurs when restarting the editor and - // connecting to a build again. sceneids were then different because FindObjectsOfType's order - // is not guranteed to be the same. - // -> we need something unique and persistent, aka always the same when pressing play/building the first time - // -> Unity has no built in unique id for GameObjects in the scene - - // helper function to figure out a unique, persistent scene id for a GameObject in the hierarchy - // -> Unity's instanceId is unique but not persistent - // -> hashing the whole GameObject is not enough either since a duplicate would have the same hash - // -> we definitely need the transform sibling index in the hierarchy - // -> so we might as well use just that - // -> transforms have children too so we need a list of sibling indices like 0->3->5 - public static List SiblingPathFor(Transform t) - { - List result = new List(); - while (t != null) - { - result.Add(t.GetSiblingIndex()); - t = t.parent; - } - - result.Reverse(); // parent to child instead of child to parent order - return result; - } - - // we need to compare by using the whole sibling list - // comparing the string won't work work because: - // "1->2" - // "20->2" - // would compare '1' vs '2', then '-' vs '0' - // - // tests: - // CompareSiblingPaths(new List(){0}, new List(){0}) => 0 - // CompareSiblingPaths(new List(){0}, new List(){1}) => -1 - // CompareSiblingPaths(new List(){1}, new List(){0}) => 1 - // CompareSiblingPaths(new List(){0,1}, new List(){0,2}) => -1 - // CompareSiblingPaths(new List(){0,2}, new List(){0,1}) => 1 - // CompareSiblingPaths(new List(){1}, new List(){0,1}) => 1 - // CompareSiblingPaths(new List(){1}, new List(){2,1}) => -1 - public static int CompareSiblingPaths(List left, List right) - { - // compare [0], remove it, compare next, etc. - while (left.Count > 0 && right.Count > 0) - { - if (left[0] < right[0]) - { - return -1; - } - else if (left[0] > right[0]) - { - return 1; - } - else - { - // equal, so they are both children of the same transform - // -> which also means that they both must have one more - // entry, so we can remove both without checking size - left.RemoveAt(0); - right.RemoveAt(0); - } - } - - // equal if both were empty or both had the same entry without any - // more children (should never happen in practice) - return 0; - } - - public static int CompareNetworkIdentitySiblingPaths(NetworkIdentity left, NetworkIdentity right) - { - return CompareSiblingPaths(SiblingPathFor(left.transform), SiblingPathFor(right.transform)); - } - - // we might have inactive scenes in the Editor's build settings, which - // aren't actually included in builds. - // so we have to only count the active ones when in Editor, otherwise - // editor and build sceneIds might get out of sync. - public static int GetSceneCount() - { -#if UNITY_EDITOR - return EditorBuildSettings.scenes.Count(scene => scene.enabled); -#else - return SceneManager.sceneCountInBuildSettings; -#endif - } + // helper function to check if a NetworkIdentity is in the active scene + static bool InActiveScene(NetworkIdentity identity) => + identity.gameObject.scene == SceneManager.GetActiveScene(); [PostProcessScene] public static void OnPostProcessScene() { - // vis2k: MISMATCHING SCENEID BUG FIX - // problem: - // * FindObjectsOfType order is not guaranteed. restarting the - // editor results in a different order - // * connecting to a build again would cause Mirror to deserialize - // the wrong objects, causing all kinds of weird errors like - // 'ReadString out of range' - // - // solution: - // sort by sibling-index path, e.g. [0,1,2] vs [1] - // this is the only deterministic way to sort a list of objects in - // the scene. - // -> it's the same result every single time, even after restarts - // - // note: there is a reason why we 'sort by' sibling path instead of - // using it as sceneId directly. networkmanager etc. use Dont- - // DestroyOnLoad, which changes the hierarchy: - // - // World: - // NetworkManager - // Player - // - // ..becomes.. - // - // World: - // Player - // DontDestroyOnLoad: - // NetworkManager - // - // so the player's siblingindex would be decreased by one. - // -> this is a problem because when building, OnPostProcessScene - // is called before any dontdestroyonload happens, but when - // entering play mode, it's called after - // -> hence sceneids would differ by one - // - // => but if we only SORT it, then it doesn't matter if one - // inbetween disappeared. as long as no NetworkIdentity used - // DontDestroyOnLoad. - // - // note: assigning a GUID in NetworkIdentity.OnValidate would be way - // cooler, but OnValidate isn't called for other unopened scenes - // when building or pressing play, so the bug would still happen - // there. - // - // note: this can still fail if DontDestroyOnLoad is called for a - // NetworkIdentity - but no one should ever do that anyway. - List identities = FindObjectsOfType().ToList(); - identities.Sort(CompareNetworkIdentitySiblingPaths); - - // sceneId assignments need to work with additive scene loading, so - // it can't always start at 1,2,3,4,..., otherwise there will be - // sceneId duplicates. - // -> we need an offset to start at 1000+1,+2,+3, etc. - // -> the most robust way is to split uint value range by sceneCount - // -> only if more than one scene. otherwise use offset 0 to avoid - // DivisionByZero if no scene in build settings, and to avoid - // different offsets in editor/build if scene wasn't added to - // build settings. - uint offsetPerScene = 0; - if (SceneManager.sceneCountInBuildSettings > 1) - { - offsetPerScene = uint.MaxValue / (uint)GetSceneCount(); - - // make sure that there aren't more sceneIds than offsetPerScene - // -> only if we have multiple scenes. otherwise offset is 0, in - // which case it doesn't matter. - if (identities.Count >= offsetPerScene) - { - Debug.LogWarning(">=" + offsetPerScene + " NetworkIdentities in scene. Additive scene loading will cause duplicate ids."); - } - } - - uint nextSceneId = 1; - foreach (NetworkIdentity identity in identities) + // find all NetworkIdentities in this scene + // => but really only from this scene. this avoids weird situations + // like in NetworkZones when we destroy the local player and + // load another scene afterwards, yet the local player is still + // in the FindObjectsOfType result with scene=DontDestroyOnLoad + // for some reason + foreach (NetworkIdentity identity in FindObjectsOfType().Where(InActiveScene)) { // if we had a [ConflictComponent] attribute that would be better than this check. // also there is no context about which scene this is in. @@ -181,13 +32,22 @@ public static void OnPostProcessScene() if (identity.isClient || identity.isServer) continue; - uint offset = (uint)identity.gameObject.scene.buildIndex * offsetPerScene; - identity.ForceSceneId(offset + nextSceneId++); - if (LogFilter.Debug) Debug.Log("PostProcess sceneid assigned: name=" + identity.name + " scene=" + identity.gameObject.scene.name + " sceneid=" + identity.sceneId); + // valid scene id? then set build index byte + // otherwise it might be an unopened scene that still has null + // sceneIds. builds are interrupted if they contain 0 sceneIds, + // but it's still possible that we call LoadScene in Editor + // for a previously unopened scene. + // => throwing an exception would only show it for one object + // because this function would return afterwards. + if (identity.sceneId != 0) + { + identity.SetSceneIdSceneIndexByteInternal(); + } + else Debug.LogError("Scene " + identity.gameObject.scene.path + " needs to be opened and resaved, because the scene object " + identity.name + " has no valid sceneId yet."); - // disable it AFTER assigning the sceneId. - // -> this way NetworkIdentity.OnDisable adds itself to the - // spawnableObjects dictionary (only if sceneId != 0) + // disable it + // note: NetworkIdentity.OnDisable adds itself to the + // spawnableObjects dictionary (only if sceneId != 0) identity.gameObject.SetActive(false); // safety check for prefabs with more than one NetworkIdentity diff --git a/Assets/Mirror/Runtime/NetworkIdentity.cs b/Assets/Mirror/Runtime/NetworkIdentity.cs index 1527d2215..6a287df9f 100644 --- a/Assets/Mirror/Runtime/NetworkIdentity.cs +++ b/Assets/Mirror/Runtime/NetworkIdentity.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.Linq; using UnityEngine; - #if UNITY_EDITOR using UnityEditor; #if UNITY_2018_3_OR_NEWER @@ -19,7 +18,6 @@ namespace Mirror public sealed class NetworkIdentity : MonoBehaviour { // configuration - [SerializeField] uint m_SceneId; [SerializeField] bool m_ServerOnly; [SerializeField] bool m_LocalPlayerAuthority; bool m_IsServer; @@ -90,6 +88,12 @@ internal set } } + // persistent scene id + [SerializeField] uint m_SceneId = 0; + + // keep track of all sceneIds to detect scene duplicates + static Dictionary sceneIds = new Dictionary(); + // used when adding players internal void SetClientOwner(NetworkConnection conn) { @@ -125,9 +129,6 @@ internal void ForceAuthority(bool authority) public delegate void ClientAuthorityCallback(NetworkConnection conn, NetworkIdentity identity, bool authorityState); public static ClientAuthorityCallback clientAuthorityCallback; - // only used when fixing duplicate scene IDs during post-processing - public void ForceSceneId(uint newSceneId) => m_SceneId = newSceneId; - // used when the player object for a connection changes internal void SetNotLocalPlayer() { @@ -184,30 +185,134 @@ bool ThisIsASceneObjectWithPrefabParent(out GameObject prefab) return true; } + // persistent sceneId assignment + // (because scene objects have no persistent unique ID in Unity) + // + // original UNET used OnPostProcessScene to assign an index based on + // FindObjectOfType order. + // -> this didn't work because FindObjectOfType order isn't deterministic. + // -> one workaround is to sort them by sibling paths, but it can still + // get out of sync when we open scene2 in editor and we have + // DontDestroyOnLoad objects that messed with the sibling index. + // + // we absolutely need a persistent id. challenges: + // * it needs to be 0 for prefabs + // => we set it to 0 in SetupIDs() if prefab! + // * it needs to be only assigned in edit time, not at runtime because + // only the objects that were in the scene since beginning should have + // a scene id. + // => Application.isPlaying check solves that + // * it needs to detect duplicated sceneIds after duplicating scene + // objects + // => sceneIds dict takes care of that + // * duplicating the whole scene file shouldn't result in duplicate + // scene objects + // => buildIndex is shifted into sceneId for that. + // => if we have no scenes in build index then it doesn't matter + // because by definition a build can't switch to other scenes + // => if we do have scenes in build index then it will be != -1 + // note: the duplicated scene still needs to be opened once for it to + // be set properly + // * scene objects need the correct scene index byte even if the scene's + // build index was changed or a duplicated scene wasn't opened yet. + // => OnPostProcessScene is the only function that gets called for + // each scene before runtime, so this is where we set the scene + // byte. + // * disabled scenes in build settings should result in same scene index + // in editor and in build + // => .gameObject.scene.buildIndex filters out disabled scenes by + // default + // * generated sceneIds absolutely need to set scene dirty and force the + // user to resave. + // => Undo.RecordObject does that perfectly. + // * sceneIds should never be generated temporarily for unopened scenes + // when building, otherwise editor and build get out of sync + // => BuildPipeline.isBuildingPlayer check solves that + void AssignSceneID() + { + // we only ever assign sceneIds at edit time, never at runtime. + // by definition, only the original scene objects should get one. + // -> if we assign at runtime then server and client would generate + // different random numbers! + if (Application.isPlaying) + return; + + // no valid sceneId yet, or duplicate? + NetworkIdentity existing; + bool duplicate = sceneIds.TryGetValue(m_SceneId, out existing) && existing != null && existing != this; + if (m_SceneId == 0 || duplicate) + { + // if a scene was never opened and we are building it, then a + // sceneId would be assigned to build but not saved in editor, + // resulting in them getting out of sync. + // => don't ever assign temporary ids. they always need to be + // permanent + // => throw an exception to cancel the build and let the user + // know how to fix it! + if (BuildPipeline.isBuildingPlayer) + throw new Exception("Scene " + gameObject.scene.path + " needs to be opened and resaved before building, because the scene object " + name + " has no valid sceneId yet."); + + // if we generate the sceneId then we MUST be sure to set dirty + // in order to save the scene object properly. otherwise it + // would be regenerated every time we reopen the scene, and + // upgrading would be very difficult. + // -> Undo.RecordObject is the new EditorUtility.SetDirty! + // -> we need to call it before changing. + Undo.RecordObject(this, "Generated SceneId"); + + // generate random sceneId + // range: 3 bytes to fill 0x00FFFFFF + m_SceneId = (uint)UnityEngine.Random.Range(0, 0xFFFFFF); + Debug.Log(name + " in scene=" + gameObject.scene.name + " sceneId assigned to: " + m_SceneId.ToString("X") + (duplicate ? " because duplicated" : "")); + } + + + // add to sceneIds dict no matter what + // -> even if we didn't generate anything new, because we still need + // existing sceneIds in there to check duplicates + sceneIds[m_SceneId] = this; + } + + // copy scene build index into sceneId for scene objects. + // this is the only way for scene file duplication to not contain + // duplicate sceneIds as it seems. + // -> sceneId before: 0x00AABBCCDD + // -> then we clear the left most byte, so that our 'OR' uses 0x00 + // -> then we OR buildIndex into the 0x00 part + // => ONLY USE THIS FROM POSTPROCESSSCENE! + public void SetSceneIdSceneIndexByteInternal() + { + byte buildIndex = (byte)gameObject.scene.buildIndex; + m_SceneId = (m_SceneId & 0x00FFFFFF) | (uint)(buildIndex << 24); + Debug.Log(name + " in scene=" + gameObject.scene.name + " scene index byte(" + buildIndex.ToString("X") + ") copied into sceneId: " + m_SceneId.ToString("X")); + } + void SetupIDs() { if (ThisIsAPrefab()) { - ForceSceneId(0); + m_SceneId = 0; // force 0 for prefabs AssignAssetID(gameObject); } else if (ThisIsASceneObjectWithPrefabParent(out GameObject prefab)) { + AssignSceneID(); AssignAssetID(prefab); } else if (PrefabStageUtility.GetCurrentPrefabStage() != null) { - ForceSceneId(0); + m_SceneId = 0; // force 0 for prefabs string path = PrefabStageUtility.GetCurrentPrefabStage().prefabAssetPath; AssignAssetID(path); } else { + AssignSceneID(); m_AssetId = ""; } } - #endif + void OnDestroy() { if (m_IsServer && NetworkServer.active)