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

Persistent sceneid #563

Merged
merged 16 commits into from Mar 8, 2019

Conversation

Projects
None yet
3 participants
@vis2k
Copy link
Owner

commented Mar 6, 2019

Fix sceneId bugs once and for all, hopefully.

This version generates a random sceneId of form 0x00FFFFFF in OnValidate and sets the scene dirty to force the user to save it.

OnPostProcessScene then copies a buildIndex byte into it, so 0x00FFFFFF becomes 0x12FFFFFF or similar. This makes sure that sceneIds are unique across scenes.

This works perfectly fine with uMMORPG and NetworkZones.

Note: when building a scene that wasn't opened with this Mirror version yet (where the sceneIds weren't set yet), the generated sceneIds won't be saved but an error message will be shown at runtime in OnPostProcessScene that tells the user to resave that scene.

@vis2k vis2k requested a review from paulpach Mar 6, 2019

@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 6, 2019

Note: Debug.Log messages need to be removed before merging. But should help us test it.

@MrGadget1024

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Sorry @vis2k - same as last time. All the networked objects in the sub-scene with ProximityCheckers appear immediately when the sub-scene is loaded instead of staying disabled until I get within range.

@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 7, 2019

@MrGadget1024 can you share your test scene?

Interrupt build if there is an unopened scene that still needs sceneI…
…d assignments. This makes it 100% fail safe.
@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 7, 2019

Update: latest commit makes the unopened scenes - sceneIds issue 100% fail safe because a build is canceled if an unopened scene contains empty sceneIds.

@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 7, 2019

@MrGadget1024 when you get a chance to test, please try to remove the '.Where(InActiveScene)' part in NetworkScenePostProcess and see if that gives you the same result. That part is somewhat optional, the only goal was to ignore recently destroyed objects.

// => ONLY USE THIS FROM POSTPROCESSSCENE!
public void SetSceneIdSceneIndexByteInternal()
{
byte buildIndex = (byte)gameObject.scene.buildIndex;

This comment has been minimized.

Copy link
@paulpach

paulpach Mar 7, 2019

Collaborator

What happens if you reorder your scenes? you would have saved your sceneid already no? then you will end up with the wrong byte in m_SceneId.

What happens if you duplicate the scene? The first time you create the scene, it will set a m_SceneId, then, when you duplicate it, all the objects will preserve their m_SceneID and will conflict with the objects in the original.

A better approach, is that the post processor don't worry about the scene index or hash at all. Just make a unique m_SceneID within that scene and persist it.
Then at runtime, we append the scene name hash into the top 4 bytes and make an ulong.

This comment has been minimized.

Copy link
@vis2k

vis2k Mar 7, 2019

Author Owner

build index byte is never saved.

if you reorder, it doesn't matter because this is called when building / when pressing play after all reordering is done.

duplicating scene is fine: if you duplicate sceneA with objectA with assetId 0x00AAAAAA then you get sceneB with objectA with assetId 0x00AAAAAA. then when building or when pressing player, OnPostProcessScene copies '0x00' for sceneA into objectA from sceneA, and '0x01' for sceneB into objectA from sceneB.

This comment has been minimized.

Copy link
@paulpach

paulpach Mar 7, 2019

Collaborator

How do you prevent it from being saved?

You may want to clean up the scene id before assigning it here then. how about:

m_SceneId &= 0x00FFFFFF;

before you add a buildIndex?

This comment has been minimized.

Copy link
@vis2k

vis2k Mar 7, 2019

Author Owner

OnPostProcessScene is only called when building (no sceneid is set when building, exception is thrown instead if it still needs to be set before building) or when pressing play (changes during play mode are never saved)

// => ONLY USE THIS FROM POSTPROCESSSCENE!
public void SetSceneIdSceneIndexByteInternal()
{
byte buildIndex = (byte)gameObject.scene.buildIndex;

This comment has been minimized.

Copy link
@paulpach

paulpach Mar 7, 2019

Collaborator

How do you prevent it from being saved?

You may want to clean up the scene id before assigning it here then. how about:

m_SceneId &= 0x00FFFFFF;

before you add a buildIndex?

@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 8, 2019

merging this now.
it's been 3 years with this bug. many of my uMMORPG customers need it for network zones.
it even gets rid of 30 LOC and I can't find a way to break it.

@vis2k vis2k merged commit 98a0e23 into master Mar 8, 2019

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 8, 2019

Posting this here too for future reference:

Update: very happy to announce that the Persistent SceneId fix is live [ https://github.com//pull/563 ].

This bug caused me tremendous amounts of pain during the last three years. In fact, I was the one who originally reported it to the UNET team back then.

Original UNET used FindObjectsOfType to assign sceneIds with a simple counter. The problem was that the FindObjectsOfType order was not deterministic. In other words, the result was often scrambled. So when building a game and connecting to it with the Editor, everything worked fine until we restarted the Editor and got a completely different FindObjectsOfTypeOrder, at which point the Editor and the Build would get out of sync. This resulted in situations where sometimes a player's move on the server would actually move a monster on the client, which is a complete nightmare to deal with.

Later on when starting HLAPI Community Edition, I found a workaround where FindObjectsOfType was sorted by the actual order in the Hierarchy (by the sibling path to be exact). This worked perfectly fine for one scene, and in fact the UNET team even ported my fix to UNET. Instead of using the sibling path, they used only the root index though, which would still result in the same type of errors if the scene object was a child object of something else.

Eventually people started switching scenes at runtime, which exposed a flaw in my original fix. When loading a scene for the second time, it's possible (in fact, likely) that objects like the NetworkManager were moved into the DontDestroyOnLoad scene, hence offseting the sibling path by -1 and getting the client and the server out of sync again.

The next approach was to use Unity's fileID. Unity stores a fileID value for each object and each component in the Scene. It's persistent across restarts, and can be found in the Scene.unity files. The fileID itself is not directly available in Unity. There is however a workaround where a script is used to set the Inspector to debug view, at which point the LocalIdentifierInFile could be read. This was the perfect solution, except that it didn't work all the time. the LocalIdentifierInFile is sometimes 0 if Play wasn't pressed yet, and it even changes at runtime occasionally. There is no documentation on this field either, but it seems like this is not the true fileID at all times, only sometimes.

My final solution was to generate a random sceneID of the format 0x00FFFFFF in Edit time, whenever OnValidate is called. Generating the sceneID sets the scene as dirty, which adds a '*' behind the scene name to indicate the user that it needs to be resaved. In other words, each NetworkIdentity gets a random sceneID that is stored on disk and never saved again afterwards.

This alone was not enough yet though. If a user duplicates SceneA.unity to SceneB.unity, then ObjectA from SceneA will have the same sceneID as ObjectA in SceneB. This second problem was solved by shifting the scene's build index into the SceneID when the scene is first loaded. The previous sceneId of 0x00FFFFFF would then change to 0x01FFFFFF for the scene at index = 1. As result, we get a unique sceneId that even works if the user duplicates a scene.

The third critical problem were unopened scenes. A lot of people will upgrade to the latest Mirror version for the fix, and there is a small chance that a user with SceneA and SceneB might only open SceneA, get all sceneIDs for SceneA assigned and then build the project without ever actually opening SceneB again, leaving all the SceneB sceneIDs still empty. In that case, Unity would call OnValidate for each object in SceneB, Mirror would generate a sceneID, but the scene would never get saved because the modifications happened at build time. This issue would not affect new Mirror users (because they always opened each of their new scenes with the latest Mirror version), but even if we were to add a huge "PLEASE RESAVE ALL SCENES ONCE" warning, someone would still miss it and run into strange missing sceneID errors.

This third problem was solved by a simple isBuilding check to never assign sceneIDs in OnValidate. Instead, users with yet unopened scenes will receive an Error message that even cancels the build process, notifying them to resave the unopened scene once.

In the end, we get perfectly persistent sceneIds that work across multiple scenes in (so far) all possible scenarios. This fix will allow you to implemented advanced features like zones, instanced dungeons, player housing and (possibly) additive scene loading.

I believe this was the last critical bug in Mirror. Enjoy!

@paulpach paulpach deleted the persistent_sceneid branch Mar 9, 2019

@vis2k

This comment has been minimized.

Copy link
Owner Author

commented Mar 19, 2019

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vis2k vis2k added the released label Mar 19, 2019

tomkrikorian pushed a commit to tomkrikorian/Mirror that referenced this pull request Jun 4, 2019

Persistent sceneid (vis2k#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.