Merge pull request #391 from CesiumGS/subscene-use-parent-coordinates

Modify CesiumSubScene to use parent CesiumGeoreference's coordinates when added in-editor.

CHANGES.md

@@ -4,14 +4,15 @@
 
 ##### Fixes :wrench:
 
-- Removed Universal Additional Camera Data script from DynamicCamera prefab, as it shows up as a missing script in other render pipelines.
+- Removed the "Universal Additional Camera Data" script from DynamicCamera, as it shows up as a missing script in other render pipelines.
+- Fixed a bug where adding a `CesiumSubScene` as the child of an existing `CesiumGeoreference` in editor would cause the parent `CesiumGeoreference` to have its coordinates reset to the default.
+- Fixed the "DynamicCamera is not nested inside a game object with a CesiumGeoreference" warning when adding a new DynamicCamera in the editor.
 
 ### v1.7.1 - 2023-12-14
 
 ##### Fixes :wrench:
 
 - Fixed a bug that prevented the default `CesiumIonServer` asset from remembering its token in a clean project.
-- Fixed "DynamicCamera is not nested inside a game object with a CesiumGeoreference" warning when adding a new DynamicCamera in the editor.
 
 ### v1.7.0 - 2023-12-14
 

EditorTests/CesiumEditorTests.asmdef

@@ -0,0 +1,25 @@
+{
+    "name": "CesiumEditorTests",
+    "rootNamespace": "",
+    "references": [
+        "UnityEngine.TestRunner",
+        "UnityEditor.TestRunner",
+        "CesiumRuntime",
+        "Unity.Mathematics"
+    ],
+    "includePlatforms": [
+        "Editor"
+    ],
+    "excludePlatforms": [],
+    "allowUnsafeCode": false,
+    "overrideReferences": true,
+    "precompiledReferences": [
+        "nunit.framework.dll"
+    ],
+    "autoReferenced": false,
+    "defineConstraints": [
+        "UNITY_INCLUDE_TESTS"
+    ],
+    "versionDefines": [],
+    "noEngineReferences": false
+}

EditorTests/TestCesiumSubScene.cs

@@ -0,0 +1,55 @@
+using CesiumForUnity;
+using NUnit.Framework;
+using System.Collections;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+public class TestCesiumSubScene
+{
+    [UnityTest]
+    public IEnumerator AddingSubSceneCopiesGeoreferenceCoordinates()
+    {
+        GameObject goGeoreference = new GameObject("Georeference");
+        CesiumGeoreference georeference = goGeoreference.AddComponent<CesiumGeoreference>();
+        georeference.SetOriginLongitudeLatitudeHeight(-55.0, 55.0, 1000.0);
+
+        GameObject goSubScene = new GameObject("SubScene");
+        goSubScene.transform.parent = goGeoreference.transform;
+        CesiumSubScene subScene = goSubScene.AddComponent<CesiumSubScene>();
+
+        yield return null;
+
+        Assert.AreEqual(-55.0, georeference.longitude);
+        Assert.AreEqual(55.0, georeference.latitude);
+        Assert.AreEqual(1000.0, georeference.height);
+        Assert.AreEqual(georeference.longitude, subScene.longitude);
+        Assert.AreEqual(georeference.latitude, subScene.latitude);
+        Assert.AreEqual(georeference.height, subScene.height);
+    }
+
+    [UnityTest]
+    public IEnumerator ModifyingSubsceneModifiesParentGeoreference()
+    {
+        GameObject goGeoreference = new GameObject("Georeference");
+        CesiumGeoreference georeference = goGeoreference.AddComponent<CesiumGeoreference>();
+        georeference.SetOriginLongitudeLatitudeHeight(-55.0, 55.0, 1000.0);
+
+        GameObject goSubScene = new GameObject("SubScene");
+        goSubScene.transform.parent = goGeoreference.transform;
+        CesiumSubScene subScene = goSubScene.AddComponent<CesiumSubScene>();
+
+        yield return null;
+
+        subScene.SetOriginLongitudeLatitudeHeight(-10.0, 10.0, 100.0);
+
+        yield return null;
+
+        Assert.AreEqual(subScene.longitude, -10.0);
+        Assert.AreEqual(subScene.latitude, 10.0);
+        Assert.AreEqual(subScene.height, 100.0);
+
+        Assert.AreEqual(georeference.longitude, -10.0);
+        Assert.AreEqual(georeference.latitude, 10.0);
+        Assert.AreEqual(georeference.height, 100.0);
+    }
+}

Runtime/CesiumSubScene.cs

@@ -191,6 +191,21 @@ public double ecefZ
         [NonSerialized]
         internal CesiumGeoreference _parentGeoreference;
 
+#if UNITY_EDITOR
+        // The coordinates of the parent CesiumGeoreference before being changed by `UpdateOrigin`.
+        // This is used when adding a CesiumSubScene component to a GameObject. Since `OnEnable`
+        // runs before `Reset`, it will force the georeference to move to the default coordinates
+        // of CesiumSubScene, which is undesired. But there's no way to modify `OnEnable` to
+        // distinguish whether the subscene was just added or already existed in the scene.
+        // Instead, we need to store the georeference's previous values during `OnEnable`,
+        // so that we can move it back during `Reset`.
+        [NonSerialized]
+        private double3 _oldParentCoordinates = double3.zero;
+
+        [NonSerialized]
+        private CesiumGeoreferenceOriginAuthority _oldParentOriginAuthority;
+#endif
+
         /// <summary>
         /// Sets the origin of the coordinate system to particular <see cref="ecefX"/>, <see cref="ecefY"/>,
         /// <see cref="ecefZ"/> coordinates in the Earth-Centered, Earth-Fixed (ECEF) frame.
@@ -231,6 +246,17 @@ public void SetOriginLongitudeLatitudeHeight(double longitude, double latitude,
             this.originAuthority = CesiumGeoreferenceOriginAuthority.LongitudeLatitudeHeight;
         }
 
+        private void CopyParentCoordinates()
+        {
+            this._longitude = this._parentGeoreference.longitude;
+            this._latitude = this._parentGeoreference.latitude;
+            this._height = this._parentGeoreference.height;
+
+            this._ecefX = this._parentGeoreference.ecefX;
+            this._ecefY = this._parentGeoreference.ecefY;
+            this._ecefZ = this._parentGeoreference.ecefZ;
+        }
+
         private void DetachFromParentIfNeeded()
         {
             if (this._parentGeoreference != null)
@@ -263,6 +289,7 @@ private void OnValidate()
             this.UpdateOrigin();
         }
 
+#if UNITY_EDITOR
         /// <summary>
         /// Called by the Editor when the user chooses to "reset" the component.
         /// The implementation here makes sure the newly-reset values for the serialized
@@ -271,7 +298,24 @@ private void OnValidate()
         private void Reset()
         {
             this.UpdateParentReference();
+
+            // The default coordinates for the CesiumSubScene component should be the coordinates of its parent, if possible.
+            // This means adding the component as the child of an existing CesiumGeoreference won't reset the parent's coordinates.
+            if (this._parentGeoreference != null)
+            {
+                if (this._oldParentOriginAuthority == CesiumGeoreferenceOriginAuthority.EarthCenteredEarthFixed)
+                {
+                    this._parentGeoreference.SetOriginEarthCenteredEarthFixed(this._oldParentCoordinates.x, this._oldParentCoordinates.y, this._oldParentCoordinates.z);
+                }
+                else
+                {
+                    this._parentGeoreference.SetOriginLongitudeLatitudeHeight(this._oldParentCoordinates.x, this._oldParentCoordinates.y, this._oldParentCoordinates.z);
+                }
+
+                this.CopyParentCoordinates();
+            }
         }
+#endif
 
         private void OnEnable()
         {
@@ -291,6 +335,19 @@ private void OnEnable()
                 scene.gameObject.SetActive(false);
             }
 
+#if UNITY_EDITOR
+            // The parent's previous coordinates are saved here in case they have to be reverted in the editor during `Reset`
+            this._oldParentOriginAuthority = this._parentGeoreference.originAuthority;
+            if (this._oldParentOriginAuthority == CesiumGeoreferenceOriginAuthority.EarthCenteredEarthFixed)
+            {
+                this._oldParentCoordinates = new double3(this._parentGeoreference.ecefX, this._parentGeoreference.ecefY, this._parentGeoreference.ecefZ);
+            }
+            else
+            {
+                this._oldParentCoordinates = new double3(this._parentGeoreference.longitude, this._parentGeoreference.latitude, this._parentGeoreference.height);
+            }
+#endif
+
             this.UpdateOrigin();
         }
 
@@ -310,13 +367,7 @@ private void OnParentChanged()
 
             // Update our origin to our parent georef, maintain our origin authority,
             // and copy both sets of reference coordinates. No need to calculate any of this again
-            this._longitude = this._parentGeoreference.longitude;
-            this._latitude = this._parentGeoreference.latitude;
-            this._height = this._parentGeoreference.height;
-
-            this._ecefX = this._parentGeoreference.ecefX;
-            this._ecefY = this._parentGeoreference.ecefY;
-            this._ecefZ = this._parentGeoreference.ecefZ;
+            this.CopyParentCoordinates();
         }
 
         private void OnDisable()
@@ -386,7 +437,7 @@ public void UpdateOrigin()
             }
         }
 
-        #if UNITY_EDITOR
+#if UNITY_EDITOR
         private void OnDrawGizmos()
         {
             if (this._showActivationRadius)
@@ -396,6 +447,6 @@ private void OnDrawGizmos()
                 Gizmos.DrawWireSphere(this.transform.position, (float)this._activationRadius);
             }
         }
-        #endif
+#endif
     }
 }