Fix a severe GPX parse slowdown and add route (<rte>) support
GPX tracks were silently failing to render for large real-world files: point_extensions used REXML's "extensions//*" XPath, and the // descendant axis costs low-single-digit milliseconds per call regardless of subtree size. Run once per point, that's negligible on small files but ~30s on a real 8000-point track — long enough to hit a proxy timeout in production, so the file just never appeared with no error. Replaced with plain recursive child traversal (~40x faster on the same file, confirmed 29.9s -> 3.16s); test_parsing_many_points_with_extensions_stays_fast guards against reintroducing it. Also adds support for <rte> routes (not just <trk> tracks and <wpt> waypoints), sharing the track/route conversion logic via a new line_feature helper. Routes get their own "route" kind and a dashed ROUTE_ICON in the file tree so the type is visible at a glance.
This commit is contained in:
1 parent
0394befdec
commit
708c08259b
6 files changed
+149
-19
No files matched your search
+21
-3
@@ -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: `<wpt>`
|
know which format it got. `Gpx` parses the GPX with REXML: `<wpt>`
|
||||||
waypoints become `Point` features (with `name`, `desc` and `time`
|
waypoints become `Point` features (with `name`, `desc` and `time`
|
||||||
carried through as properties), each `<trk>` becomes one
|
carried through as properties), each `<trk>` becomes one
|
||||||
`MultiLineString` feature (one line per `<trkseg>`); `<rte>` routes
|
`MultiLineString` feature (one line per `<trkseg>`), and each `<rte>`
|
||||||
are not handled. `Fit` reads Garmin's binary FIT format directly
|
becomes one too (a route is a single continuous line by definition, no
|
||||||
|
`<trkseg>`-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`). `<rte>` has no GPX equivalent of
|
||||||
|
`<trk>`'s `<type>`, 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`
|
(hand-rolled — see below) into a single `MultiLineString` `track`
|
||||||
feature; FIT activity files don't really have a GPX-waypoint
|
feature; FIT activity files don't really have a GPX-waypoint
|
||||||
equivalent, so it never produces `Point` features. MapLibre's
|
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 `<extensions>` block under each `<trkpt>` — usually
|
device-specific `<extensions>` block under each `<trkpt>` — usually
|
||||||
Garmin's `TrackPointExtension`, matched by local element name only
|
Garmin's `TrackPointExtension`, matched by local element name only
|
||||||
(`hr`/`cad`/`speed`) so it doesn't care which XML namespace prefix a
|
(`hr`/`cad`/`speed`) so it doesn't care which XML namespace prefix a
|
||||||
given file happens to declare.
|
given file happens to declare. That `<extensions>` 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 `<svg>` (elevation/speed/cadence/
|
Each chart is its own single-series `<svg>` (elevation/speed/cadence/
|
||||||
heart rate are on wildly different scales, so this is small multiples
|
heart rate are on wildly different scales, so this is small multiples
|
||||||
|
|||||||
+62
-15
@@ -4,9 +4,9 @@ require_relative "track_stats"
|
|||||||
|
|
||||||
module Maptool
|
module Maptool
|
||||||
# Converts a GPX file to a GeoJSON FeatureCollection: waypoints (<wpt>)
|
# Converts a GPX file to a GeoJSON FeatureCollection: waypoints (<wpt>)
|
||||||
# become Point features, tracks (<trk>) become MultiLineString features
|
# become Point features, tracks (<trk>) and routes (<rte>) both become
|
||||||
# (one line per <trkseg>). Routes (<rte>) are not handled — GPX in
|
# MultiLineString features — "kind" (=> "track"/"route") is the only
|
||||||
# practice is either points or recorded tracks for this app.
|
# difference in shape, so the file tree can show which one a file is.
|
||||||
module Gpx
|
module Gpx
|
||||||
class ParseError < StandardError; end
|
class ParseError < StandardError; end
|
||||||
|
|
||||||
@@ -20,6 +20,7 @@ module Maptool
|
|||||||
kinds = []
|
kinds = []
|
||||||
kinds << "waypoint" if content.include?("<wpt")
|
kinds << "waypoint" if content.include?("<wpt")
|
||||||
kinds << "track" if content.include?("<trk")
|
kinds << "track" if content.include?("<trk")
|
||||||
|
kinds << "route" if content.include?("<rte")
|
||||||
kinds
|
kinds
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -30,6 +31,7 @@ module Maptool
|
|||||||
features = []
|
features = []
|
||||||
root.each_element("wpt") { |wpt| features << waypoint_feature(wpt) }
|
root.each_element("wpt") { |wpt| features << waypoint_feature(wpt) }
|
||||||
root.each_element("trk") { |trk| features << track_feature(trk) }
|
root.each_element("trk") { |trk| features << track_feature(trk) }
|
||||||
|
root.each_element("rte") { |rte| features << route_feature(rte) }
|
||||||
|
|
||||||
{ "type" => "FeatureCollection", "features" => features }
|
{ "type" => "FeatureCollection", "features" => features }
|
||||||
rescue REXML::ParseException => e
|
rescue REXML::ParseException => e
|
||||||
@@ -58,43 +60,88 @@ module Maptool
|
|||||||
trk.each_element("trkseg") do |seg|
|
trk.each_element("trkseg") do |seg|
|
||||||
points = []
|
points = []
|
||||||
seg.each_element("trkpt") do |pt|
|
seg.each_element("trkpt") do |pt|
|
||||||
point = coordinate(pt)
|
point, time, extra = process_point(pt)
|
||||||
points << point
|
points << point
|
||||||
all_points << point
|
all_points << point
|
||||||
times << parse_time(child_text(pt, "time"))
|
times << time
|
||||||
extras << point_extensions(pt)
|
extras << extra
|
||||||
end
|
end
|
||||||
lines << points unless points.empty?
|
lines << points unless points.empty?
|
||||||
end
|
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: <rte> holds <rtept> directly, no
|
||||||
|
# <trkseg>-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",
|
"type" => "Feature",
|
||||||
"geometry" => { "type" => "MultiLineString", "coordinates" => lines },
|
"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(TrackStats.compute(all_points, times: times))
|
||||||
.merge("profile" => TrackStats.build_profile(all_points, times: times, extras: extras)),
|
.merge("profile" => TrackStats.build_profile(all_points, times: times, extras: extras)),
|
||||||
}
|
}
|
||||||
end
|
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 <extensions> block
|
# Heart rate/cadence/speed live in a device-specific <extensions> block
|
||||||
# (usually Garmin's own TrackPointExtension) — matched by local element
|
# (usually Garmin's own TrackPointExtension) — matched by local element
|
||||||
# name only (`Element#name` already strips the namespace prefix), so
|
# name only (`Element#name` already strips the namespace prefix), so
|
||||||
# this doesn't care which extension schema a given file actually
|
# 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.
|
# 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)
|
def self.point_extensions(point_el)
|
||||||
values = {}
|
values = {}
|
||||||
point_el.elements.each("extensions//*") do |el|
|
extensions = point_el.elements["extensions"]
|
||||||
case el.name
|
collect_extension_values(extensions, values) if extensions
|
||||||
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
|
|
||||||
values
|
values
|
||||||
end
|
end
|
||||||
private_class_method :point_extensions
|
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)
|
def self.parse_time(text)
|
||||||
text && Time.parse(text)
|
text && Time.parse(text)
|
||||||
rescue ArgumentError
|
rescue ArgumentError
|
||||||
|
|||||||
+4
-1
@@ -1,5 +1,5 @@
|
|||||||
import { Popup } from "/vendor/maplibre-gl/maplibre-gl.mjs";
|
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 { notifySidePanelOpen, onlyOneSidePanelOpen } from "/js/panels.js";
|
||||||
import { formatActivityType } from "/js/track_profile.js";
|
import { formatActivityType } from "/js/track_profile.js";
|
||||||
|
|
||||||
@@ -226,6 +226,9 @@ export class GpxPanelControl {
|
|||||||
if (kind === "waypoint") {
|
if (kind === "waypoint") {
|
||||||
icon.innerHTML = WAYPOINT_ICON;
|
icon.innerHTML = WAYPOINT_ICON;
|
||||||
icon.title = "Has waypoints";
|
icon.title = "Has waypoints";
|
||||||
|
} else if (kind === "route") {
|
||||||
|
icon.innerHTML = ROUTE_ICON;
|
||||||
|
icon.title = "Has a route";
|
||||||
} else {
|
} else {
|
||||||
icon.innerHTML = TRACK_ICON;
|
icon.innerHTML = TRACK_ICON;
|
||||||
icon.title = "Has a track";
|
icon.title = "Has a track";
|
||||||
|
|||||||
@@ -48,6 +48,16 @@ export const WAYPOINT_ICON = `
|
|||||||
|
|
||||||
export const TRACK_ICON = GPX_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 = `
|
||||||
|
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.6" stroke-linecap="round" stroke-linejoin="round">
|
||||||
|
<circle cx="5" cy="6" r="2"/>
|
||||||
|
<circle cx="19" cy="18" r="2"/>
|
||||||
|
<path d="M6.5 7.5C10 11 8 13 12 13s2 5 6.5 5" stroke-dasharray="2 2.2"/>
|
||||||
|
</svg>`;
|
||||||
|
|
||||||
export const LIVETRACK_ICON = `
|
export const LIVETRACK_ICON = `
|
||||||
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.6" stroke-linecap="round" stroke-linejoin="round">
|
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.6" stroke-linecap="round" stroke-linejoin="round">
|
||||||
<path d="M12 21s7-7.58 7-12a7 7 0 0 0-14 0c0 4.42 7 12 7 12z"/>
|
<path d="M12 21s7-7.58 7-12a7 7 0 0 0-14 0c0 4.42 7 12 7 12z"/>
|
||||||
|
|||||||
Vendored
+12
@@ -0,0 +1,12 @@
|
|||||||
|
<?xml version="1.0" encoding="UTF-8"?>
|
||||||
|
<gpx version="1.1" creator="test">
|
||||||
|
<rte>
|
||||||
|
<name>Planned Loop</name>
|
||||||
|
<rtept lat="48.1" lon="17.2">
|
||||||
|
<ele>200</ele>
|
||||||
|
</rtept>
|
||||||
|
<rtept lat="48.101" lon="17.201">
|
||||||
|
<ele>210</ele>
|
||||||
|
</rtept>
|
||||||
|
</rte>
|
||||||
|
</gpx>
|
||||||
@@ -41,6 +41,46 @@ class GpxTest < Minitest::Test
|
|||||||
assert_equal [17.2, 48.1], [profile[0]["lon"], profile[0]["lat"]]
|
assert_equal [17.2, 48.1], [profile[0]["lon"], profile[0]["lat"]]
|
||||||
end
|
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"], "<rte> has no GPX equivalent of <trk><type>"
|
||||||
|
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
|
||||||
|
<trkpt lat="48.1" lon="17.2"><ele>200</ele><time>2026-01-01T09:00:#{format("%02d", i % 60)}Z</time>
|
||||||
|
<extensions><gpxtpx:TrackPointExtension xmlns:gpxtpx="urn"><gpxtpx:hr>140</gpxtpx:hr></gpxtpx:TrackPointExtension></extensions>
|
||||||
|
</trkpt>
|
||||||
|
TRKPT
|
||||||
|
end.join
|
||||||
|
path = File.join(Dir.mktmpdir, "big.gpx")
|
||||||
|
File.write(path, "<gpx><trk><trkseg>#{points}</trkseg></trk></gpx>")
|
||||||
|
|
||||||
|
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
|
def test_invalid_xml_raises_parse_error
|
||||||
path = File.join(Dir.mktmpdir, "bad.gpx")
|
path = File.join(Dir.mktmpdir, "bad.gpx")
|
||||||
File.write(path, "<gpx><trk unclosed>")
|
File.write(path, "<gpx><trk unclosed>")
|
||||||
|
|||||||
Reference in new issue
Block a user