-
Notifications
You must be signed in to change notification settings - Fork 10
fix: skip an invalid camera instead of crashing the map #160
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
28 changes: 28 additions & 0 deletions
28
package/android/src/main/java/com/margelo/nitro/nitromaps/Camera+Validity.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package com.margelo.nitro.nitromaps | ||
|
|
||
| /** | ||
| * `CameraPosition.Builder.tilt` throws for a non-finite pitch, which unwinds the Fabric mount | ||
| * transaction, and a non-finite zoom or heading is taken silently and leaves the camera reading | ||
| * back as `NaN`. | ||
| */ | ||
| internal fun Camera.isValid(): Boolean = | ||
| isValidCoordinate(center.latitude, center.longitude) && | ||
| zoom.isDrawableAsFloatOrAbsent() && | ||
| heading.isDrawableAsFloatOrAbsent() && | ||
| pitch.isFiniteOrAbsent() && | ||
| altitude.isFiniteOrAbsent() | ||
|
|
||
| /** | ||
| * `CameraPosition` holds zoom and bearing as `Float`, so the value the SDK receives is the | ||
| * converted one: a `Double` past `Float.MAX_VALUE` - `Double.MAX_VALUE` among them - becomes | ||
| * `Infinity`, which the builder takes without complaint and then normalizes into a `NaN` bearing. | ||
| * Checking after the conversion is what makes this guard match what the map is handed. | ||
| */ | ||
| private fun Double?.isDrawableAsFloatOrAbsent(): Boolean = this == null || toFloat().isFinite() | ||
|
|
||
| /** | ||
| * Pitch and altitude stay `Double`: pitch is coerced into the drawable range before it is narrowed, | ||
| * and altitude never reaches `CameraPosition` at all. An absent value is filled in from the camera | ||
| * the map already has, so only a supplied one is checked. | ||
| */ | ||
| private fun Double?.isFiniteOrAbsent(): Boolean = this == null || isFinite() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
103 changes: 103 additions & 0 deletions
103
package/android/src/test/java/com/margelo/nitro/nitromaps/CameraValidityTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package com.margelo.nitro.nitromaps | ||
|
|
||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertFalse | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
|
|
||
| class CameraValidityTest { | ||
| @Test | ||
| fun acceptsACameraTheSdkCanRepresent() { | ||
| assertTrue(camera().isValid()) | ||
| assertTrue(camera(latitude = -90.0, longitude = 180.0).isValid()) | ||
| } | ||
|
|
||
| @Test | ||
| fun acceptsACameraThatSuppliesNoFramingValues() { | ||
| assertTrue(camera(zoom = null, heading = null, pitch = null, altitude = null).isValid()) | ||
| } | ||
|
|
||
| @Test | ||
| fun rejectsANonFiniteCenter() { | ||
| assertFalse(camera(latitude = Double.NaN, longitude = Double.NaN).isValid()) | ||
| assertFalse(camera(longitude = Double.POSITIVE_INFINITY).isValid()) | ||
| } | ||
|
|
||
| @Test | ||
| fun rejectsACenterOutsideTheWorld() { | ||
| assertFalse(camera(latitude = 1000.0).isValid()) | ||
| assertFalse(camera(longitude = -180.0001).isValid()) | ||
| } | ||
|
|
||
| @Test | ||
| fun rejectsANonFiniteFramingValue() { | ||
| assertFalse(camera(zoom = Double.NaN).isValid()) | ||
| assertFalse(camera(heading = Double.POSITIVE_INFINITY).isValid()) | ||
| assertFalse(camera(pitch = Double.NaN).isValid()) | ||
| assertFalse(camera(altitude = Double.NEGATIVE_INFINITY).isValid()) | ||
| } | ||
|
|
||
| /** A pitch past the drawable range is clamped rather than rejected, so the camera still applies. */ | ||
| @Test | ||
| fun acceptsAPitchOutsideTheDrawableRange() { | ||
| assertTrue(camera(pitch = 120.0).isValid()) | ||
| assertTrue(camera(pitch = -10.0).isValid()) | ||
| } | ||
|
|
||
| /** | ||
| * `CameraPosition.Builder.tilt` throws `IllegalArgumentException` for anything outside 0..90, so | ||
| * the clamp is what keeps a valid camera with an unsupported pitch from taking the mount | ||
| * transaction down. | ||
| */ | ||
| @Test | ||
| fun clampsAPitchTheSdkWouldRefuse() { | ||
| assertEquals(90.0f, camera(pitch = 120.0).toCameraPosition().tilt, 0.0f) | ||
| assertEquals(0.0f, camera(pitch = -10.0).toCameraPosition().tilt, 0.0f) | ||
| assertEquals(45.0f, camera(pitch = 45.0).toCameraPosition().tilt, 0.0f) | ||
| } | ||
|
|
||
| /** | ||
| * Zoom and bearing are narrowed to `Float` on the way into `CameraPosition`, so a `Double` past | ||
| * `Float.MAX_VALUE` is not something the map can be handed, however finite it is as a `Double`. | ||
| */ | ||
| @Test | ||
| fun rejectsAFramingValueThatOverflowsAFloat() { | ||
| assertFalse(camera(zoom = Double.MAX_VALUE).isValid()) | ||
| assertFalse(camera(heading = -Double.MAX_VALUE).isValid()) | ||
| } | ||
|
|
||
| /** | ||
| * What the guard above prevents, pinned against the real SDK: the builder takes the overflowed | ||
| * value without complaint, and its `% 360` normalization turns an infinite bearing into `NaN`. | ||
| */ | ||
| @Test | ||
| fun anOverflowingFramingValueWouldReachTheSdkAsInfinity() { | ||
| val position = camera(zoom = Double.MAX_VALUE, heading = Double.MAX_VALUE).toCameraPosition() | ||
|
|
||
| assertEquals(Float.POSITIVE_INFINITY, position.zoom, 0.0f) | ||
| assertTrue(position.bearing.isNaN()) | ||
| } | ||
|
|
||
| /** Pitch is coerced into the drawable range before it is narrowed, so an absurd one still draws. */ | ||
| @Test | ||
| fun clampsAPitchThatOverflowsAFloat() { | ||
| assertTrue(camera(pitch = Double.MAX_VALUE).isValid()) | ||
| assertEquals(90.0f, camera(pitch = Double.MAX_VALUE).toCameraPosition().tilt, 0.0f) | ||
| } | ||
|
|
||
| private fun camera( | ||
| latitude: Double = 52.23, | ||
| longitude: Double = 21.01, | ||
| zoom: Double? = 12.0, | ||
| heading: Double? = 90.0, | ||
| pitch: Double? = 45.0, | ||
| altitude: Double? = 1000.0, | ||
| ): Camera = | ||
| Camera( | ||
| center = Coordinate(latitude = latitude, longitude = longitude), | ||
| zoom = zoom, | ||
| heading = heading, | ||
| pitch = pitch, | ||
| altitude = altitude, | ||
| ) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| extension Camera { | ||
| /// Whether the camera can be handed to `MKMapView.camera` without MapKit raising. | ||
| var isValid: Bool { | ||
| center.isValid | ||
| && CameraFraming.isDrawable( | ||
| zoom: zoom, | ||
| heading: heading, | ||
| pitch: pitch, | ||
| altitude: altitude | ||
| ) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import Foundation | ||
|
|
||
| /// The framing half of a camera - zoom, heading, pitch and altitude - which | ||
| /// `CLLocationCoordinate2DIsValid` says nothing about. | ||
| enum CameraFraming { | ||
| /// Every value the caller supplied has to be a real number. `MKMapCamera` | ||
| /// does not reject a `NaN` one: it collapses the altitude to the minimum the | ||
| /// map allows, or leaves `MKMapView.region` reading back as `NaN`. On Android | ||
| /// a non-finite tilt throws out of `CameraPosition` instead. | ||
| static func isDrawable( | ||
| zoom: Double?, | ||
| heading: Double?, | ||
| pitch: Double?, | ||
| altitude: Double? | ||
| ) -> Bool { | ||
| isRealAsFloatOrAbsent(zoom) | ||
| && isRealOrAbsent(heading) | ||
| && isRealOrAbsent(pitch) | ||
| && isRealOrAbsent(altitude) | ||
| } | ||
|
|
||
| /// `GMSCameraPosition` holds zoom as a `Float`, and `CameraPosition` does the | ||
| /// same on Android, so the value the SDK receives is the narrowed one: a | ||
| /// `Double` past `Float.greatestFiniteMagnitude` arrives as `infinity`. Zoom | ||
| /// is the only value narrowed that way - heading, pitch and altitude stay | ||
| /// `Double` through `CLLocationDirection` and `MKMapCamera`. | ||
| private static func isRealAsFloatOrAbsent(_ value: Double?) -> Bool { | ||
| guard let value else { | ||
| return true | ||
| } | ||
|
|
||
| return value.isFinite && Float(value).isFinite | ||
| } | ||
|
|
||
| /// An omitted value is filled in from the camera the map already has, so only | ||
| /// a supplied one has to be checked. | ||
| private static func isRealOrAbsent(_ value: Double?) -> Bool { | ||
| guard let value else { | ||
| return true | ||
| } | ||
|
|
||
| return value.isFinite | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import Testing | ||
|
|
||
| @testable import NitroMapsGeometry | ||
|
|
||
| @Test | ||
| func acceptsFramingValuesTheMapCanUse() { | ||
| #expect(CameraFraming.isDrawable(zoom: 12, heading: 90, pitch: 45, altitude: 1000)) | ||
| #expect(CameraFraming.isDrawable(zoom: 0, heading: 0, pitch: 0, altitude: 0)) | ||
| } | ||
|
|
||
| @Test | ||
| func acceptsAbsentFramingValues() { | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: nil, altitude: nil)) | ||
| #expect(CameraFraming.isDrawable(zoom: 12, heading: nil, pitch: nil, altitude: nil)) | ||
| } | ||
|
|
||
| @Test | ||
| func rejectsANonFiniteFramingValue() { | ||
| #expect(!CameraFraming.isDrawable(zoom: .nan, heading: nil, pitch: nil, altitude: nil)) | ||
| #expect(!CameraFraming.isDrawable(zoom: nil, heading: .infinity, pitch: nil, altitude: nil)) | ||
| #expect(!CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: .nan, altitude: nil)) | ||
| #expect(!CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: nil, altitude: -.infinity)) | ||
| } | ||
|
|
||
| /// A pitch past the range the SDKs draw is pulled back to it rather than | ||
| /// rejected, so the camera still reaches the map. | ||
| @Test | ||
| func acceptsAPitchOutsideTheDrawableRange() { | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: 120, altitude: nil)) | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: -10, altitude: nil)) | ||
| } | ||
|
|
||
| /// Zoom is narrowed to a `Float` on its way into both SDKs, so a `Double` too | ||
| /// large for one is not a zoom the map can be handed, however finite it is. | ||
| @Test | ||
| func rejectsAZoomThatOverflowsAFloat() { | ||
| #expect(!CameraFraming.isDrawable(zoom: .greatestFiniteMagnitude, heading: nil, pitch: nil, altitude: nil)) | ||
| #expect(!CameraFraming.isDrawable(zoom: -.greatestFiniteMagnitude, heading: nil, pitch: nil, altitude: nil)) | ||
| #expect(CameraFraming.isDrawable(zoom: 3.4e38, heading: nil, pitch: nil, altitude: nil)) | ||
| } | ||
|
|
||
| /// Heading, pitch and altitude stay `Double` all the way to `MKMapCamera` and | ||
| /// `CLLocationDirection`, so a large one is still a value the map can take. | ||
| @Test | ||
| func keepsALargeHeadingPitchOrAltitude() { | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: .greatestFiniteMagnitude, pitch: nil, altitude: nil)) | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: .greatestFiniteMagnitude, altitude: nil)) | ||
| #expect(CameraFraming.isDrawable(zoom: nil, heading: nil, pitch: nil, altitude: .greatestFiniteMagnitude)) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.