Repository navigation
fix: simplify tolerance conversion using longitude instead of latitude - #253
nathancahill wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Review (bot-assisted, verified locally on the PR head dd62071):
- The fix now matches
GISTool.degrees(fromMeters:atLatitude:)and every other cos-of-latitude conversion in the codebase.
Two optional suggestions inline below: dedupe the conversion via the existing helper (mind the small constant change), and one cheap planar assertion to round out projection coverage.
| let oneDegreeLongitudeDistanceInMeters: CLLocationDistance = (cos(firstCoordinate.latitude * .pi / 180.0) * 111.0) * 1000.0 | ||
| let toleranceInDegrees: CLLocationDegrees = toleranceInMeters / oneDegreeLongitudeDistanceInMeters |
There was a problem hiding this comment.
Non-blocking, optional: this now duplicates the conversion that already exists as Coordinate3D.degrees(fromMeters:) (Sources/GISTools/Algorithms/Conversions.swift). Suggested change below reuses it.
Note that this also swaps the hardcoded 111.0 km/degree for GISTool.earthCircumference / 360.0 (~111.32 km/degree), i.e. it is slightly more accurate but does change results marginally. Entirely reasonable to defer this refactor to a follow-up if you want this PR strictly minimal.
| let oneDegreeLongitudeDistanceInMeters: CLLocationDistance = (cos(firstCoordinate.latitude * .pi / 180.0) * 111.0) * 1000.0 | |
| let toleranceInDegrees: CLLocationDegrees = toleranceInMeters / oneDegreeLongitudeDistanceInMeters | |
| let toleranceInDegrees = firstCoordinate.degrees(fromMeters: toleranceInMeters).longitudeDegrees |
| // vertices must survive regardless of the meridian. | ||
| #expect(simplifiedAtGreenwich.coordinates.count > 50) | ||
| #expect(simplifiedAtMinus90.coordinates.count == simplifiedAtGreenwich.coordinates.count) | ||
| #expect(simplifiedAtMinus111.coordinates.count == simplifiedAtGreenwich.coordinates.count) |
There was a problem hiding this comment.
Non-blocking: bug fixes should include tests for all affected projections. The degree conversion only runs on the geographic branch, so one cheap planar assertion in the new test is enough (keeps the regression test self-contained; broader coverage already exists in simplify3857). Note Coordinate3D(x:y:) defaults to .epsg3857.
| #expect(simplifiedAtMinus111.coordinates.count == simplifiedAtGreenwich.coordinates.count) | |
| #expect(simplifiedAtMinus111.coordinates.count == simplifiedAtGreenwich.coordinates.count) | |
| // The degree conversion only applies to geographic projections; | |
| // planar projections (e.g. EPSG:3857) pass meters through directly. | |
| let planarLine = try #require(LineString([ | |
| Coordinate3D(x: 0.0, y: 0.0), | |
| Coordinate3D(x: 250.0, y: 250.0), | |
| Coordinate3D(x: 500.0, y: 0.0), | |
| ])) | |
| #expect(planarLine.simplified(tolerance: 100.0).projection == .epsg3857) |
Problem
The meter-to-degree tolerance conversion in
Simplify.simplify(coordinates:toleranceInMeters:highQuality:)usedcos(longitude)where it must becos(latitude):Longitude has no physical bearing on meters-per-degree of longitude, so for geometries away from the equator the conversion was nonsensical. Worst case near longitude ±90° (e.g. the western US:
cos(-90°) ≈ 6e-17): one-degree-distance ≈ 0 → tolerance in degrees ≈ ∞ → valid geometry collapses almost completely with the default tolerance. (Visible to callers viaGeoJson.simplified/simplify, which always pass meters into this path.)Fix
Use
cos(latitude)as everywhere else in the codebase (cf.GISTool.degrees(fromMeters:atLatitude:)):This changes simplified output for geographic geometries away from longitude 0° vs. previous releases — toward correct behavior.
Notes / possible follow-ups (non-blocking)
cos(±90°) → 0→ infinite degree tolerance (pre-existing heuristic behavior).simplifyVisvalingamWhyattcurrently uses latitude-independentprojection.crsLength(fromMeters:); aligning the two conventions could be a separate task.