ssannandeji / Zenject-2019

Dependency Injection Framework for Unity3D
MIT License
2.53k stars 363 forks source link

DiContainer.InstantiatePrefab offsets new objects by the SceneContext transform (non-Editor builds only) #547

Closed thisisthedave closed 5 years ago

thisisthedave commented 6 years ago

On iOS only, some of our prefabs were being instantiated with an offset applied to the initial position. I was able to debug and find the root of the problem.

For example, constructing an instance like this: var instance = _container.InstantiatePrefab(prefab, location.position, location.rotation, null); ... will yield an instance that has been offset by the transform of the SceneContext object. Here's why.

InstantiatePrefab eventually drills into DiContainer.CreateAndParentPrefab, which calculates the parent for the new instance like this:

            Transform initialParent;
#if !UNITY_EDITOR
            if(wasActive)
            {
                prefabAsGameObject.SetActive(false);
            }
#else
            if(wasActive)
            {
                initialParent = ZenUtilInternal.GetOrCreateInactivePrefabParent();
            }
            else
#endif
            if(parent != null)
            {
                initialParent = parent;
            }
            else
            {
                // This ensures it gets added to the right scene instead of just the active scene
                initialParent = ContextTransform;
            }

If UNITY_EDITOR is unset (on iOS, for example) and a null parent transform is passed to InstantiatePrefab, the instance is parented under the scene context. A few lines down, the new instance is unparented:

            if(gameObj.transform.parent != parent)
            {
                gameObj.transform.SetParent(parent, false);
            }

However, the position/rotation of the new gameObj are not updated. Consequently, if the scene context is not positioned at the origin, the new instance is unintentionally offset by the scene context transform.

I believe this shouldn't happen because you passing false for the worldPositionStays parameter of SetParent, so perhaps this is a bug with Unity? I have reproduced the error on Unity versions 2018.2.0f2 and 2018.2.7f1.

svermeulen commented 5 years ago

Yeah it does sound like a unity bug especially since it only exists on iOS. That must have been pretty confusing behaviour until you figured it out. I'm not really sure what the fix should be however. I suppose to could apply a workaround to ensure the offset doesn't happen until unity fixes it. Can you reproduce the problem when you copy that logic of reparenting it after instantiating it? If so, can you report this as a bug to unity?

thisisthedave commented 5 years ago

I'll try to create a minimal repro case and submit it to Unity. FWIW, this is the workaround I applied to DiContainer.cs (patch attached):

image

dicontainer.patch.txt

On Fri, Sep 21, 2018 at 11:40 PM Steve Vermeulen notifications@github.com wrote:

Yeah it does sound like a unity bug especially since it only exists on iOS. That must have been pretty confusing behaviour until you figured it out. I'm not really sure what the fix should be however. I suppose to could apply a workaround to ensure the offset doesn't happen until unity fixes it. Can you reproduce the problem when you copy that logic of reparenting it after instantiating it? If so, can you report this as a bug to unity?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/svermeulen/Zenject/issues/547#issuecomment-423721435, or mute the thread https://github.com/notifications/unsubscribe-auth/AE7GgbRSnjaXPrFhWldiRskCg3_Sw9xHks5uddtjgaJpZM4WmXCD .

svermeulen commented 5 years ago

Ok I applied your fix, thanks

svermeulen commented 5 years ago

I took a deeper look into this, and I think I found the issue. I think that when an explicit position and rotation are given, then it should really be passing true to the SetParent methods, so that it retains the absolute position and rotation that were passed to GameObject.Instantiate. I've removed the patch that you suggested in favour of fe60e94. Let me know if you're able to confirm whether that works for your case