diff --git a/docs/architecture.adoc b/docs/architecture.adoc index 3a198eb..57ac2df 100644 --- a/docs/architecture.adoc +++ b/docs/architecture.adoc @@ -181,8 +181,16 @@ same GeoJSON `FeatureCollection` shape so the frontend doesn't need to know which format it got. `Gpx` parses the GPX with REXML: `` waypoints become `Point` features (with `name`, `desc` and `time` carried through as properties), each `` becomes one -`MultiLineString` feature (one line per ``); `` routes -are not handled. `Fit` reads Garmin's binary FIT format directly +`MultiLineString` feature (one line per ``), and each `` +becomes one too (a route is a single continuous line by definition, no +``-style grouping) — the two are told apart only by a `kind` +property (`"track"` vs `"route"`; waypoints are `"waypoint"`), which +`Gpx.kinds` also exposes per-file so the file tree can show a dashed +route icon next to a route file and a solid one for a track +(`public/js/icons.js`'s `ROUTE_ICON`/`TRACK_ICON`, +`public/js/gpx.js`'s `_kindIcon`). `` has no GPX equivalent of +``'s ``, so a route's `activity_type` is always `nil`. +`Fit` reads Garmin's binary FIT format directly (hand-rolled — see below) into a single `MultiLineString` `track` feature; FIT activity files don't really have a GPX-waypoint equivalent, so it never produces `Point` features. MapLibre's @@ -280,7 +288,17 @@ below). For FIT, those extras are the `record` message's own device-specific `` block under each `` — usually Garmin's `TrackPointExtension`, matched by local element name only (`hr`/`cad`/`speed`) so it doesn't care which XML namespace prefix a -given file happens to declare. +given file happens to declare. That `` block is walked by +hand (a plain recursive `elements.each`) rather than REXML's +`"extensions//*"` XPath, which this used originally: the `//` +descendant axis costs low-single-digit milliseconds *per call* +regardless of how small the subtree actually is, and this runs once +per point — negligible on a short file, but ~30s for a real +8000-point one (enough to hit a proxy timeout in production, so the +track just silently never rendered). Plain traversal is ~40x faster on +the same file; `test/test_gpx.rb`'s +`test_parsing_many_points_with_extensions_stays_fast` is a regression +test for it. + Each chart is its own single-series `` (elevation/speed/cadence/ heart rate are on wildly different scales, so this is small multiples diff --git a/lib/maptool/gpx.rb b/lib/maptool/gpx.rb index b7aa293..e948b1f 100644 --- a/lib/maptool/gpx.rb +++ b/lib/maptool/gpx.rb @@ -4,9 +4,9 @@ require_relative "track_stats" module Maptool # Converts a GPX file to a GeoJSON FeatureCollection: waypoints () - # become Point features, tracks () become MultiLineString features - # (one line per ). Routes () are not handled — GPX in - # practice is either points or recorded tracks for this app. + # become Point features, tracks () and routes () both become + # MultiLineString features — "kind" (=> "track"/"route") is the only + # difference in shape, so the file tree can show which one a file is. module Gpx class ParseError < StandardError; end @@ -20,6 +20,7 @@ module Maptool kinds = [] kinds << "waypoint" if content.include?(" "FeatureCollection", "features" => features } rescue REXML::ParseException => e @@ -58,43 +60,88 @@ module Maptool trk.each_element("trkseg") do |seg| points = [] seg.each_element("trkpt") do |pt| - point = coordinate(pt) + point, time, extra = process_point(pt) points << point all_points << point - times << parse_time(child_text(pt, "time")) - extras << point_extensions(pt) + times << time + extras << extra end lines << points unless points.empty? end + line_feature(kind: "track", name: child_text(trk, "name"), activity_type: child_text(trk, "type"), + lines: lines, all_points: all_points, times: times, extras: extras) + end + private_class_method :track_feature + + # Structurally simpler than a track: holds directly, no + # -style grouping — a route is one continuous line by + # definition, planned rather than recorded. + def self.route_feature(rte) + points = [] + times = [] + extras = [] + rte.each_element("rtept") do |pt| + point, time, extra = process_point(pt) + points << point + times << time + extras << extra + end + + line_feature(kind: "route", name: child_text(rte, "name"), activity_type: nil, + lines: points.empty? ? [] : [points], all_points: points, times: times, extras: extras) + end + private_class_method :route_feature + + def self.line_feature(kind:, name:, activity_type:, lines:, all_points:, times:, extras:) { "type" => "Feature", "geometry" => { "type" => "MultiLineString", "coordinates" => lines }, - "properties" => { "kind" => "track", "name" => child_text(trk, "name"), "activity_type" => child_text(trk, "type") } + "properties" => { "kind" => kind, "name" => name, "activity_type" => activity_type } .merge(TrackStats.compute(all_points, times: times)) .merge("profile" => TrackStats.build_profile(all_points, times: times, extras: extras)), } end - private_class_method :track_feature + private_class_method :line_feature + + def self.process_point(pt) + [coordinate(pt), parse_time(child_text(pt, "time")), point_extensions(pt)] + end + private_class_method :process_point # Heart rate/cadence/speed live in a device-specific block # (usually Garmin's own TrackPointExtension) — matched by local element # name only (`Element#name` already strips the namespace prefix), so # this doesn't care which extension schema a given file actually # declares, as long as it names its elements the same way Garmin's does. + # + # Walked by hand (elements.each with no argument — plain child + # iteration) rather than the `"extensions//*"` XPath this used + # originally: REXML's `//` (descendant) axis turned out to cost + # milliseconds *per call* regardless of how small the subtree actually + # is, and this runs once per point — fine on a short test file, ~30s + # for an 8000-point real one. Plain traversal is ~40x faster on that + # same file. def self.point_extensions(point_el) values = {} - point_el.elements.each("extensions//*") do |el| - case el.name - when "hr" then values[:heart_rate_bpm] = el.text&.strip&.to_i - when "cad" then values[:cadence_rpm] = el.text&.strip&.to_i - when "speed" then values[:speed_mps] = el.text&.strip&.to_f - end - end + extensions = point_el.elements["extensions"] + collect_extension_values(extensions, values) if extensions values end private_class_method :point_extensions + def self.collect_extension_values(el, values) + el.elements.each do |child| + case child.name + when "hr" then values[:heart_rate_bpm] = child.text&.strip&.to_i + when "cad" then values[:cadence_rpm] = child.text&.strip&.to_i + when "speed" then values[:speed_mps] = child.text&.strip&.to_f + end + collect_extension_values(child, values) + end + end + private_class_method :collect_extension_values + def self.parse_time(text) text && Time.parse(text) rescue ArgumentError diff --git a/public/js/gpx.js b/public/js/gpx.js index 0c1cb7d..a2529e8 100644 --- a/public/js/gpx.js +++ b/public/js/gpx.js @@ -1,5 +1,5 @@ import { Popup } from "/vendor/maplibre-gl/maplibre-gl.mjs"; -import { GPX_ICON, NEW_FOLDER_ICON, UPLOAD_ICON, WAYPOINT_ICON, TRACK_ICON } from "/js/icons.js"; +import { GPX_ICON, NEW_FOLDER_ICON, UPLOAD_ICON, WAYPOINT_ICON, TRACK_ICON, ROUTE_ICON } from "/js/icons.js"; import { notifySidePanelOpen, onlyOneSidePanelOpen } from "/js/panels.js"; import { formatActivityType } from "/js/track_profile.js"; @@ -226,6 +226,9 @@ export class GpxPanelControl { if (kind === "waypoint") { icon.innerHTML = WAYPOINT_ICON; icon.title = "Has waypoints"; + } else if (kind === "route") { + icon.innerHTML = ROUTE_ICON; + icon.title = "Has a route"; } else { icon.innerHTML = TRACK_ICON; icon.title = "Has a track"; diff --git a/public/js/icons.js b/public/js/icons.js index 6cfc1af..a0e10a3 100644 --- a/public/js/icons.js +++ b/public/js/icons.js @@ -48,6 +48,16 @@ export const WAYPOINT_ICON = ` export const TRACK_ICON = GPX_ICON; +// Same shape as TRACK_ICON, dashed — a route is planned rather than +// recorded, the same visual convention most map apps use for the +// distinction. +export const ROUTE_ICON = ` + + + + +`; + export const LIVETRACK_ICON = ` diff --git a/test/fixtures/sample_route.gpx b/test/fixtures/sample_route.gpx new file mode 100644 index 0000000..b030fe8 --- /dev/null +++ b/test/fixtures/sample_route.gpx @@ -0,0 +1,12 @@ + + + + Planned Loop + + 200 + + + 210 + + + diff --git a/test/test_gpx.rb b/test/test_gpx.rb index 1f40a94..05442a0 100644 --- a/test/test_gpx.rb +++ b/test/test_gpx.rb @@ -41,6 +41,46 @@ class GpxTest < Minitest::Test assert_equal [17.2, 48.1], [profile[0]["lon"], profile[0]["lat"]] end + def test_kinds_detects_routes + assert_equal ["route"], Maptool::Gpx.kinds(fixture_path("sample_route.gpx")) + end + + def test_to_geojson_produces_a_route_multilinestring + geojson = Maptool::Gpx.to_geojson(fixture_path("sample_route.gpx")) + route = geojson["features"].find { |f| f["properties"]["kind"] == "route" } + + assert_equal "MultiLineString", route["geometry"]["type"] + assert_equal 2, route["geometry"]["coordinates"][0].size + assert_equal "Planned Loop", route["properties"]["name"] + assert_nil route["properties"]["activity_type"], " has no GPX equivalent of " + assert_operator route["properties"]["distance_meters"], :>, 0 + end + + # Regression test for a real bug: point_extensions originally used the + # "extensions//*" XPath (REXML's `//` descendant axis), which cost + # milliseconds *per call* regardless of subtree size — fine on this + # suite's small fixtures, ~30s on a real 8000-point file. This fixture is + # smaller than that (keeping the suite fast) but big enough that an + # O(n^2) reintroduction would make itself obvious well under the bound. + def test_parsing_many_points_with_extensions_stays_fast + points = Array.new(2000) do |i| + <<~TRKPT + 200 + 140 + + TRKPT + end.join + path = File.join(Dir.mktmpdir, "big.gpx") + File.write(path, "#{points}") + + started = Time.now + geojson = Maptool::Gpx.to_geojson(path) + elapsed = Time.now - started + + assert_equal 2000, geojson["features"][0]["geometry"]["coordinates"][0].size + assert_operator elapsed, :<, 5, "took #{elapsed}s for 2000 points — was O(n^2) via an XPath descendant search once, see comment above" + end + def test_invalid_xml_raises_parse_error path = File.join(Dir.mktmpdir, "bad.gpx") File.write(path, "")