From 84f3a12d27c59ffb19769ddede4447e8ecc88921 Mon Sep 17 00:00:00 2001 From: Ashley Rogers Date: Wed, 31 Jan 2024 14:18:56 -0500 Subject: [PATCH] Update based on review --- Editor/CesiumOriginShiftEditor.cs | 54 -------------------------- Editor/CesiumOriginShiftEditor.cs.meta | 11 ------ Runtime/CesiumOriginShift.cs | 26 +++---------- Tests/Comparers.cs | 27 +++++++++++-- Tests/TestCesiumOriginShift.cs | 26 +++++++------ 5 files changed, 42 insertions(+), 102 deletions(-) delete mode 100644 Editor/CesiumOriginShiftEditor.cs delete mode 100644 Editor/CesiumOriginShiftEditor.cs.meta diff --git a/Editor/CesiumOriginShiftEditor.cs b/Editor/CesiumOriginShiftEditor.cs deleted file mode 100644 index 0c2c5526..00000000 --- a/Editor/CesiumOriginShiftEditor.cs +++ /dev/null @@ -1,54 +0,0 @@ -using UnityEditor; -using UnityEngine; - -namespace CesiumForUnity -{ - [CustomEditor(typeof(CesiumOriginShift))] - public class CesiumOriginShiftEditor : Editor - { - private SerializedProperty _activationDistance; - private SerializedProperty _useActivationDistance; - - private void OnEnable() - { - _activationDistance = this.serializedObject.FindProperty("_activationDistance"); - _useActivationDistance = this.serializedObject.FindProperty("_useActivationDistance"); - } - - public override void OnInspectorGUI() - { - this.serializedObject.Update(); - this.DrawProperties(); - this.serializedObject.ApplyModifiedProperties(); - } - - private void DrawProperties() - { - // Specify a label width so that both properties' values are aligned - // This is the same value as in CesiumCameraControllerEditor so that everything lines up - int labelWidth = 215; - - GUILayout.BeginHorizontal(); - GUIContent useActivationDistanceContent = new GUIContent( - "Use Activation Distance", - "Whether the Activation Distance property will be used. " + - "If unchecked, the origin will be shifted on every frame."); - GUILayout.Label(useActivationDistanceContent, GUILayout.Width(labelWidth)); - EditorGUILayout.PropertyField(this._useActivationDistance, GUIContent.none); - GUILayout.EndHorizontal(); - - EditorGUI.BeginDisabledGroup(!this._useActivationDistance.boolValue); - - GUILayout.BeginHorizontal(); - GUIContent activationDistanceContent = new GUIContent( - "Activation Distance", - "When Use Activation Distance is checked, the origin will only be shifted " + - "when the distance in meters from the parent georeference is greater than this value."); - GUILayout.Label(activationDistanceContent, GUILayout.Width(labelWidth)); - EditorGUILayout.PropertyField(this._activationDistance, GUIContent.none); - GUILayout.EndHorizontal(); - - EditorGUI.EndDisabledGroup(); - } - } -} diff --git a/Editor/CesiumOriginShiftEditor.cs.meta b/Editor/CesiumOriginShiftEditor.cs.meta deleted file mode 100644 index 1f2c9ef8..00000000 --- a/Editor/CesiumOriginShiftEditor.cs.meta +++ /dev/null @@ -1,11 +0,0 @@ -fileFormatVersion: 2 -guid: e43f7266749bdfc4cb5172fe0a574604 -MonoImporter: - externalObjects: {} - serializedVersion: 2 - defaultReferences: [] - executionOrder: 0 - icon: {instanceID: 0} - userData: - assetBundleName: - assetBundleVariant: diff --git a/Runtime/CesiumOriginShift.cs b/Runtime/CesiumOriginShift.cs index 75b81099..2f1b50c5 100644 --- a/Runtime/CesiumOriginShift.cs +++ b/Runtime/CesiumOriginShift.cs @@ -29,20 +29,6 @@ namespace CesiumForUnity [IconAttribute("Packages/com.cesium.unity/Editor/Resources/Cesium-24x24.png")] public class CesiumOriginShift : MonoBehaviour { - /// - /// When false, the origin will be shifted every frame. - /// When true, will be used to specify the distance from the old - /// origin after which the origin will be shifted. - /// - public bool useActivationDistance - { - get => _useActivationDistance; - set => _useActivationDistance = value; - } - - [SerializeField] - private bool _useActivationDistance = false; - /// /// Specifies the minimum distance in meters from the old origin to the current origin before the /// origin of the parent will be shifted. @@ -54,7 +40,7 @@ public double activationDistance } [SerializeField] - private double _activationDistance = 1.0; + private double _activationDistance = 0.0; void LateUpdate() { @@ -69,15 +55,13 @@ void LateUpdate() return; } - this.UpdateFromEcef(georeference, anchor); + this.UpdateFromEcef(georeference, anchor.positionGlobeFixed); } private List _sublevelsScratch = new List(); - private void UpdateFromEcef(CesiumGeoreference georeference, CesiumGlobeAnchor anchor) + private void UpdateFromEcef(CesiumGeoreference georeference, double3 ecef) { - double3 ecef = anchor.positionGlobeFixed; - CesiumSubScene closestLevel = null; double distanceSquaredToClosest = double.MaxValue; @@ -128,9 +112,9 @@ private void UpdateFromEcef(CesiumGeoreference georeference, CesiumGlobeAnchor a double distance = math.length(new double3(georeference.ecefX, georeference.ecefY, georeference.ecefZ) - ecef); - if (!this.useActivationDistance || distance >= this._activationDistance) + if (distance >= this._activationDistance) { - // Update the origin continuously. + // Update the origin if we've surpassed the distance threshold. georeference.SetOriginEarthCenteredEarthFixed(ecef.x, ecef.y, ecef.z); // Make sure the physics system is informed that things have moved Physics.SyncTransforms(); diff --git a/Tests/Comparers.cs b/Tests/Comparers.cs index 7408de1c..31e0e54e 100644 --- a/Tests/Comparers.cs +++ b/Tests/Comparers.cs @@ -5,26 +5,45 @@ internal class Comparers { private class DoubleComparer : IEqualityComparer { - private readonly double _epsilon; + private readonly double _absoluteEpsilon; + private readonly double _relativeEpsilon; - public DoubleComparer(double epsilon) + public DoubleComparer(double absoluteEpsilon) { - this._epsilon = epsilon; + this._absoluteEpsilon = absoluteEpsilon; + this._relativeEpsilon = 0.0; + } + + public DoubleComparer(double absoluteEpsilon, double relativeEpsilon) : this(absoluteEpsilon) + { + this._relativeEpsilon = relativeEpsilon; } public bool Equals(double x, double y) { - return Math.Abs(x - y) <= this._epsilon; + double diff = Math.Abs(x - y); + return diff <= this._absoluteEpsilon || + diff <= RelativeToAbsoluteEpsilon(x, y, this._relativeEpsilon); } public int GetHashCode(double obj) { throw new NotImplementedException(); } + + private double RelativeToAbsoluteEpsilon(double left, double right, double relativeEpsilon) + { + return relativeEpsilon * Math.Max(Math.Abs(left), Math.Abs(right)); + } } public static IEqualityComparer Double(double absoluteEpsilon) { return new DoubleComparer(absoluteEpsilon); } + + public static IEqualityComparer Double(double absoluteEpsilon, double relativeEpsilon) + { + return new DoubleComparer(absoluteEpsilon, relativeEpsilon); + } } diff --git a/Tests/TestCesiumOriginShift.cs b/Tests/TestCesiumOriginShift.cs index 7bc1203c..47b34e9b 100644 --- a/Tests/TestCesiumOriginShift.cs +++ b/Tests/TestCesiumOriginShift.cs @@ -1,6 +1,7 @@ using CesiumForUnity; using NUnit.Framework; using System.Collections; +using System.Collections.Generic; using Unity.Mathematics; using UnityEngine; using UnityEngine.TestTools; @@ -114,7 +115,6 @@ public IEnumerator UsesActivationDistanceProperty() CesiumGlobeAnchor globeAnchor = goOriginShift.AddComponent(); CesiumOriginShift originShift = goOriginShift.AddComponent(); - originShift.useActivationDistance = true; originShift.activationDistance = 5000; yield return null; @@ -123,19 +123,23 @@ public IEnumerator UsesActivationDistanceProperty() yield return null; - Assert.AreEqual(baseEcef.x + 4999.0, globeAnchor.positionGlobeFixed.x); - Assert.AreEqual(baseEcef.x + 0, georeference.ecefX); + IEqualityComparer epsilon6 = Comparers.Double(1e-6); + + // The anchor is still within the distance threshold, so the georeference should remain unchanged. + Assert.That(baseEcef.x + 4999.0, Is.EqualTo(globeAnchor.positionGlobeFixed.x).Using(epsilon6)); + Assert.That(baseEcef.x, Is.EqualTo(georeference.ecefX).Using(epsilon6)); globeAnchor.positionGlobeFixed = baseEcef + new double3(5010.0, 0, 0); yield return null; - Assert.AreEqual(baseEcef.x + 5010.0, globeAnchor.positionGlobeFixed.x); - Assert.AreEqual(baseEcef.x + 5010.0, georeference.ecefX); + // The anchor has surpassed the distance threshold, so the georeference should shift to the anchor's ECEF coordinates. + Assert.That(baseEcef.x + 5010.0, Is.EqualTo(globeAnchor.positionGlobeFixed.x).Using(epsilon6)); + Assert.That(globeAnchor.positionGlobeFixed.x, Is.EqualTo(georeference.ecefX).Using(epsilon6)); } // Testing a bug where the character controller would cause a jump ahead when the activation distance is hit. [UnityTest] - public IEnumerator ShiftingOriginWithCharacterController() + public IEnumerator ShiftsOriginWithCharacterController() { double3 baseEcef = new double3(-2694020, -4297355, 3854720); @@ -150,7 +154,6 @@ public IEnumerator ShiftingOriginWithCharacterController() CesiumGlobeAnchor globeAnchor = goOriginShift.AddComponent(); CesiumOriginShift originShift = goOriginShift.AddComponent(); - originShift.useActivationDistance = true; originShift.activationDistance = 5000; CharacterController controller = goOriginShift.AddComponent(); @@ -161,7 +164,9 @@ public IEnumerator ShiftingOriginWithCharacterController() yield return new WaitForEndOfFrame(); - Assert.AreEqual((float)baseEcef.x, (float)globeAnchor.positionGlobeFixed.x); + IEqualityComparer epsilon6 = Comparers.Double(1e-6, 1e-6); + + Assert.That(baseEcef.x, Is.EqualTo(globeAnchor.positionGlobeFixed.x).Using(epsilon6)); // speed per second double speed = 1000.0; @@ -182,11 +187,8 @@ public IEnumerator ShiftingOriginWithCharacterController() yield return new WaitForEndOfFrame(); globeAnchor.Sync(); - Assert.AreEqual((float)(previousPositionEcef.x + unitsEcef), (float)globeAnchor.positionGlobeFixed.x); + Assert.That(previousPositionEcef.x + unitsEcef, Is.EqualTo(globeAnchor.positionGlobeFixed.x).Using(epsilon6)); Assert.Less(georeference.ecefX - globeAnchor.positionGlobeFixed.x, 5000.0); - - Debug.Log($"georeference d: ({georeference.ecefX - baseEcef.x}, {georeference.ecefY - baseEcef.y}, {georeference.ecefZ - baseEcef.z})"); - Debug.Log($"anchor d: {globeAnchor.positionGlobeFixed - baseEcef}"); } } }