Lots of comments, plus unit tests for 3 CLLocation extensions (#228)

* Add swiftdoc comments to a couple of internal functions.

* Add a large collection of unit tests for coordinateWithBearing(bearing:distanceMeters).

* Punctuation nit.

* Rename ARCLTests.swift to CLLocationExtensionsTests.swift.

* Rename the class, not just the file.

* Simplify the CLLocationExtensionsTests structure.

* Add tests for bearing-to-point computation.

* Tweaks and comments.

* Remove signing.

* Better error messages.

* Correct comment on CLLocation.bearing(between:).

* A few more comments, and some minor edits.

* Whitespace changes to make the linter happy.

* Comment the test-runners.

* More comments.

* Add unit test for CLLocation.translation(toLocation:).

* Add comments on node's `pivot` property and the impact on label Y axis offset.

* Minor comment tweaks.

* Comments on the test runner.

* Minor comment change.

* Enable code coverage tracking.

* More comments.

* Add some testing for CLLocataion.translation(toLocation:).

* Minor edit.

* Add a note on a mysterious property.

* update Travis CI to use Xcode 11.1

* Remove duplicate LNTouchDelegate compliance.

* disable failing unit tests

.travis.yml

@@ -1,5 +1,5 @@
 language: objective-c
-osx_image: xcode10.2 
+osx_image: xcode11.1 
 
 script:
     - bundle install

ARKit+CoreLocation.xcodeproj/project.pbxproj

@@ -21,7 +21,7 @@
 		49A3FBE51F09988100AA59C8 /* Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 49A3FBE41F09988100AA59C8 /* Assets.xcassets */; };
 		49A3FBE81F09988100AA59C8 /* LaunchScreen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 49A3FBE61F09988100AA59C8 /* LaunchScreen.storyboard */; };
 		580A603392EEF4589D68F428 /* Pods_ARKit_CoreLocation_ARCLTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = AE4378FAA43D764F4F6D0513 /* Pods_ARKit_CoreLocation_ARCLTests.framework */; };
-		932EF23220BE7E6E0052210C /* ARCLTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 932EF23120BE7E6E0052210C /* ARCLTests.swift */; };
+		932EF23220BE7E6E0052210C /* CLLocationExtensionsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 932EF23120BE7E6E0052210C /* CLLocationExtensionsTests.swift */; };
 /* End PBXBuildFile section */
 
 /* Begin PBXContainerItemProxy section */
@@ -55,7 +55,7 @@
 		6C10523EF85AA6B7CDF962AD /* Pods_ARKit_CoreLocation.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_ARKit_CoreLocation.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		8C31ACA439BC114F28F96BDD /* Pods-ARKit+CoreLocation.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-ARKit+CoreLocation.debug.xcconfig"; path = "Pods/Target Support Files/Pods-ARKit+CoreLocation/Pods-ARKit+CoreLocation.debug.xcconfig"; sourceTree = "<group>"; };
 		932EF22A20BE7E6E0052210C /* ARCLTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = ARCLTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
-		932EF23120BE7E6E0052210C /* ARCLTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ARCLTests.swift; sourceTree = "<group>"; };
+		932EF23120BE7E6E0052210C /* CLLocationExtensionsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CLLocationExtensionsTests.swift; sourceTree = "<group>"; };
 		932EF23320BE7E6E0052210C /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		987CF5558E9B786105AD6834 /* Pods-ARKit+CoreLocation-ARCLTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-ARKit+CoreLocation-ARCLTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-ARKit+CoreLocation-ARCLTests/Pods-ARKit+CoreLocation-ARCLTests.release.xcconfig"; sourceTree = "<group>"; };
 		994C280456570735FF51D68E /* Pods-ARKit+CoreLocation.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-ARKit+CoreLocation.debug.xcconfig"; path = "Pods/Target Support Files/Pods-ARKit+CoreLocation/Pods-ARKit+CoreLocation.debug.xcconfig"; sourceTree = "<group>"; };
@@ -152,7 +152,7 @@
 		932EF23020BE7E6E0052210C /* ARCLTests */ = {
 			isa = PBXGroup;
 			children = (
-				932EF23120BE7E6E0052210C /* ARCLTests.swift */,
+				932EF23120BE7E6E0052210C /* CLLocationExtensionsTests.swift */,
 				932EF23320BE7E6E0052210C /* Info.plist */,
 			);
 			path = ARCLTests;
@@ -389,7 +389,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
-				932EF23220BE7E6E0052210C /* ARCLTests.swift in Sources */,
+				932EF23220BE7E6E0052210C /* CLLocationExtensionsTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

ARKit+CoreLocation.xcodeproj/xcshareddata/xcschemes/ARKit+CoreLocation.xcscheme

@@ -26,7 +26,17 @@
       buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
-      shouldUseLaunchSchemeArgsEnv = "YES">
+      shouldUseLaunchSchemeArgsEnv = "YES"
+      codeCoverageEnabled = "YES">
+      <MacroExpansion>
+         <BuildableReference
+            BuildableIdentifier = "primary"
+            BlueprintIdentifier = "49A3FBD71F09988100AA59C8"
+            BuildableName = "ARKit+CoreLocation.app"
+            BlueprintName = "ARKit+CoreLocation"
+            ReferencedContainer = "container:ARKit+CoreLocation.xcodeproj">
+         </BuildableReference>
+      </MacroExpansion>
       <Testables>
          <TestableReference
             skipped = "NO">
@@ -37,19 +47,55 @@
                BlueprintName = "ARCLTests"
                ReferencedContainer = "container:ARKit+CoreLocation.xcodeproj">
             </BuildableReference>
+            <SkippedTests>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingFarNorth10000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingFarNorth500()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingFarNorth50000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingLowLatitude10000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingLowLatitude500()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingLowLatitude50000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingMidLatitude10000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingMidLatitude500()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingMidLatitude50000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingSouthernHemisphere10000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingSouthernHemisphere500()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testCoordinateWithBearingSouthernHemisphere50000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testTranslationFarNorth10000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testTranslationFarNorth50000()">
+               </Test>
+               <Test
+                  Identifier = "CLLocationExtensionsTests/testTranslationMidLatitude500Old()">
+               </Test>
+            </SkippedTests>
          </TestableReference>
       </Testables>
-      <MacroExpansion>
-         <BuildableReference
-            BuildableIdentifier = "primary"
-            BlueprintIdentifier = "49A3FBD71F09988100AA59C8"
-            BuildableName = "ARKit+CoreLocation.app"
-            BlueprintName = "ARKit+CoreLocation"
-            ReferencedContainer = "container:ARKit+CoreLocation.xcodeproj">
-         </BuildableReference>
-      </MacroExpansion>
-      <AdditionalOptions>
-      </AdditionalOptions>
    </TestAction>
    <LaunchAction
       buildConfiguration = "Debug"
@@ -71,8 +117,6 @@
             ReferencedContainer = "container:ARKit+CoreLocation.xcodeproj">
          </BuildableReference>
       </BuildableProductRunnable>
-      <AdditionalOptions>
-      </AdditionalOptions>
    </LaunchAction>
    <ProfileAction
       buildConfiguration = "Release"

ARKit+CoreLocation/POIViewController.swift

@@ -159,20 +159,6 @@ class POIViewController: UIViewController {
     }
 }
 
-// MARK: - LNTouchDelegate
-
-@available(iOS 11.0, *)
-extension POIViewController: LNTouchDelegate {
-
-    func locationNodeTouched(node: AnnotationNode) {
-        if let parent = node.parent as? LocationNode {
-            let location = sceneLocationView.locationOfLocationNode(parent)
-            let locText = "Latitude: \(location.coordinate.latitude)° Longitude: \(location.coordinate.longitude)° \nAltitude: \(location.altitude) m"
-            nodePositionLabel.text = locText
-        }
-    }
-}
-
 // MARK: - MKMapViewDelegate
 
 @available(iOS 11.0, *)

Podfile.lock

@@ -13,4 +13,4 @@ SPEC CHECKSUMS:
 
 PODFILE CHECKSUM: 070cf25cad5040a75beb13f2dc4f72dbb0242f55
 
-COCOAPODS: 1.7.2
+COCOAPODS: 1.7.5

Sources/ARKit-CoreLocation/Extensions/BaseTypes+Extensions.swift

@@ -10,10 +10,12 @@ import Foundation
 
 public extension Double {
 
+    /// Returns radians. Distance divided by the WGS-84 equatorial radius. Dubious geodesy.
     var metersToLatitude: Double {
         return self / (6_378_137.0)
     }
 
+    /// Returns radians. Distance divided by the WGS-84 polar radius. Dubious geodesy; invalid except at equator.
     var metersToLongitude: Double {
         return self / (6_356_752.3)
     }

Sources/ARKit-CoreLocation/Extensions/CLLocation+Extensions.swift

@@ -23,6 +23,7 @@ public extension CLLocation {
 
     /// Translates distance in meters between two locations.
     /// Returns the result as the distance in latitude and distance in longitude.
+    /// The approximation used here gives reasonable accuracy out to a radius of 50 km except at high latitudes.
     func translation(toLocation location: CLLocation) -> LocationTranslation {
         let inbetweenLocation = CLLocation(latitude: self.coordinate.latitude, longitude: location.coordinate.longitude)
 
@@ -61,6 +62,11 @@ public extension CLLocation {
                           timestamp: self.timestamp)
     }
 
+    /// Bearing from `self` to another point. Returns bearing in +/- degrees from north 
+    /// This function uses the haversine formula to compute a geodesic (great circle), assuming a spherical earth.
+    /// Note that, especially at high latitudes and with relatively distant points, `a.bearing(between: b)`
+    /// is not necessarily 180 degrees opposite to `b.bearing(between: a)`.
+    /// - Parameter point: second point to compute bearing to.
     func bearing(between point: CLLocation) -> Double {
         let lat1 = self.coordinate.latitude.degreesToRadians
         let lon1 = self.coordinate.longitude.degreesToRadians
@@ -84,6 +90,12 @@ public extension CLLocation {
 
 public extension CLLocationCoordinate2D {
 
+    /// Returns a new `CLLocationCoordinate2D` at the given bearing and distance from the original point.
+    /// This function uses neither a geodesic nor a rhumb line formula. Instead, it uses a rectangular approximation of a sphere.
+    /// For very short distances, this method is probably accurate enough, but for long distances, a geodesic (great circle)
+    /// formula is required instead. Unit testing shows inaccuracy even as close as 500 meters.
+    /// - Parameter bearing: bearing in degrees clockwise from north.
+    /// - Parameter distanceMeters: distance in meters.
     func coordinateWithBearing(bearing: Double, distanceMeters: Double) -> CLLocationCoordinate2D {
         //The numbers for earth radius may be _off_ here
         //but this gives a reasonably accurate result..

Sources/ARKit-CoreLocation/Extensions/SCNNode+Extensions.swift

@@ -19,6 +19,8 @@ extension SCNNode {
             geom.materials.forEach { $0.readsFromDepthBuffer = false }
         }
     }
+
+    /// Returns a node similar to the one displayed when an `ARSCNView`'s `.debugOptions` includes `.showWorldOrigin`
     class func axesNode(quiverLength: CGFloat, quiverThickness: CGFloat) -> SCNNode {
         let quiverThickness = (quiverLength / 50.0) * quiverThickness
         let chamferRadius = quiverThickness / 2.0

Sources/ARKit-CoreLocation/Location Manager/SceneLocationManager.swift

@@ -13,7 +13,7 @@ import MapKit
 
 ///Different methods which can be used when determining locations (such as the user's location).
 public enum LocationEstimateMethod {
-    ///Only uses core location data.
+    ///Only uses Core Location data.
     ///Not suitable for adding nodes using current position, which requires more precision.
     case coreLocationDataOnly
 

Sources/ARKit-CoreLocation/Nodes/LocationAnnotationNode.swift

@@ -9,6 +9,7 @@ import Foundation
 import SceneKit
 import CoreLocation
 
+/// A `LocationNode` which has an attached `AnnotationNode`.
 open class LocationAnnotationNode: LocationNode {
     /// Subnodes and adjustments should be applied to this subnode
     /// Required to allow scaling at the same time as having a 2D 'billboard' appearance
@@ -38,8 +39,8 @@ open class LocationAnnotationNode: LocationNode {
     /// background image, labels, etc.
     ///
     /// - Parameters:
-    ///   - location: The location of the node in the world.
-    ///   - view: The view to display at the specified location.
+    ///   - location:The location of the node in the world.
+    ///   - view:The view to display at the specified location.
     public convenience init(location: CLLocation?, view: UIView) {
         self.init(location: location, image: view.image)
     }
@@ -66,6 +67,9 @@ open class LocationAnnotationNode: LocationNode {
         fatalError("init(coder:) has not been implemented")
     }
 
+    /// Note: we repeat code from `LocationNode`'s implementation of this function. Is this because of the use of `SCNTransaction`
+    /// to wrap the changes? It's legal to nest the calls, should consider this if any more changes to
+    /// `LocationNode`'s implementation are needed.
     override func updatePositionAndScale(setup: Bool = false, scenePosition: SCNVector3?,
                                          locationNodeLocation nodeLocation: CLLocation,
                                          locationManager: SceneLocationManager,
@@ -105,6 +109,10 @@ open class LocationAnnotationNode: LocationNode {
             }
         }
 
+        // Adjust the pivot in the Y axis so the label will show above the actual node location.
+        // The actual adjustment should probably be a parameter or property of some sort instead
+        // of a magic "-1.1". Some applications want the label drawn exactly in place, and some
+        // might even want to use a positive pivot, to draw below its true location.
         self.pivot = SCNMatrix4MakeTranslation(0, -1.1 * scale, 0)
 
         SCNTransaction.commit()

Sources/ARKit-CoreLocation/Nodes/LocationNode.swift

@@ -35,6 +35,8 @@ open class AnnotationNode: SCNNode {
 /// layout purposes.  To adjust the scale and position of items within a node,
 /// you can add them to a child node and adjust them there
 open class LocationNode: SCNNode {
+    // FIXME: figure out why this is hardcoded and why it would ever be different from the scene's sitting?
+    /// This seems like it should be a bug? Why is it hardcoded? Why would it ever be different from the scene's setting?
     var locationEstimateMethod: LocationEstimateMethod = .mostRelevantEstimate
 
     /// Location can be changed and confirmed later by SceneLocationView.
@@ -53,7 +55,7 @@ open class LocationNode: SCNNode {
     }
 
     /// Whether a node's position should be adjusted on an ongoing basis
-    /// based on its' given location.
+    /// based on its given location.
     /// This only occurs when a node's location is within 100m of the user.
     /// Adjustment doesn't apply to nodes without a confirmed location.
     /// When this is set to false, the result is a smoother appearance.
@@ -150,6 +152,8 @@ open class LocationNode: SCNNode {
         return adjustedDistance
     }
 
+    /// See `LocationAnnotationNode`'s override of this function. Because it doesn't invoke `super`'s version, any changes
+    /// made in this file must be repeated in `LocationAnnotationNode`.
     func updatePositionAndScale(setup: Bool = false, scenePosition: SCNVector3?, locationNodeLocation nodeLocation: CLLocation,
                                 locationManager: SceneLocationManager, onCompletion: (() -> Void)) {
         guard let position = scenePosition, locationManager.currentLocation != nil else {

Sources/ARKit-CoreLocation/Nodes/PolylineNode.swift

@@ -82,6 +82,9 @@ private extension PolylineNode {
 
             let bearing = -currentLocation.bearing(between: nextLocation)
 
+            // This pivot translation in the +Z direction will cause the box to be
+            // rendered a bit away from its true location. The actual factor should
+            // probably be a parameter or property of some sort.
             boxNode.pivot = SCNMatrix4MakeTranslation(0, 0, 0.5 * Float(distance))
             boxNode.eulerAngles.y = Float(bearing).degreesToRadians
 

Sources/ARKit-CoreLocation/Nodes/ScalingScheme.swift

@@ -9,13 +9,13 @@ import Foundation
 
 /// A set of schemes that can be used to scale a LocationNode.
 ///
-/// - normal: The default way of scaling
-/// - tiered: A way of scaling everything beyond a specific distance (threshold) at
-/// a specific scale.
-/// - doubleTiered: A way of scaling everything beyond 2 specific distances at two
+/// Values:
+/// - normal: The default way of scaling, Hardcoded value out to 3000 meters, and then 0.75 that factor beyond 3000 m.
+/// - tiered (threshold, scale): Return 1.0 at distance up to `threshold` meters, or `scale` beyond.
+/// - doubleTiered (firstThreshold, firstCale, secondThreshold, secondScale): A way of scaling everything beyond 2 specific distances at two
 /// specific scales.
-/// - linear: linearly scales an object based on its distance.
-/// - linearBuffer: linearly scales an object based on its distance as long as it is
+/// - linear (threshold): linearly scales an object based on its distance.
+/// - linearBuffer (threshold, buffer): linearly scales an object based on its distance as long as it is
 /// further than the buffer distance, otherwise it just returns 100% scale.
 public enum ScalingScheme {
 
@@ -25,6 +25,8 @@ public enum ScalingScheme {
     case linear(threshold: Double)
     case linearBuffer(threshold: Double, buffer: Double)
 
+    /// Returns a closure to compute appropriate scale factor based on the current value of `self` (a `ScalingSchee`).
+    /// The closure accepts two parameters and returns the scale factor to apply to an `AnnotationNode`.
     public func getScheme() -> ( (_ distance: Double, _ adjustedDistance: Double) -> Float) {
         switch self {
         case .tiered(let threshold, let scale):
@@ -52,7 +54,7 @@ public enum ScalingScheme {
                 let absThreshold = abs(threshold)
                 let absAdjDist = abs(adjustedDistance)
 
-                let scaleToReturn =  Float( max (maxSize - (absAdjDist / absThreshold), 0.0))
+                let scaleToReturn =  Float( max(maxSize - (absAdjDist / absThreshold), 0.0))
 //                print("threshold: \(absThreshold) adjDist: \(absAdjDist) scaleToReturn: \(scaleToReturn)")
                 return scaleToReturn
             }