From 53a422d22d66a4600e301e9627db30c7d033479f Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 23 Sep 2026 09:04:58 +0200 Subject: [PATCH] fix(roadtrip): other ways come from the router that draws the leg, and a choice is saved once it holds The picker of other ways asked OSRM with nothing avoided while the rail drives a trip that avoids a road class through Valhalla, so the list had no current road, called a road the rail was not on the fastest, and choosing it changed nothing. A choice was stored as one via where the offer strayed furthest, without asking whether the router then drove it: a point on a ferry was pulled to the pier and the drive went overland, and a way weighed away from motorways kept the motorway after its one point. An OSRM answer standing in for a Valhalla that did not respond was cached as the avoided road, unmarked. The rail now hands out a router per leg with the leg's own mode, avoided classes and engine, and its days carry the line of every leg. Offers come from that engine, and the rail's own leg always heads the list as Current. A choice is pinned and proven against that router with at most three pins before anything is written; a way it will not follow is not saved and says why, with a hint when it crosses by ferry, and a leg that changed meanwhile or a device offline writes nothing. The OSRM stand-in is filed under the plain key only and flags the day as not avoided. --- .../components/Map/RouteCalculator.test.ts | 197 ++++++++++ client/src/components/Map/RouteCalculator.ts | 276 +++++++++++--- .../src/components/Map/valhallaRoute.test.ts | 61 +++- client/src/components/Map/valhallaRoute.ts | 105 +++++- .../Roadtrip/RoadtripAlternativesBar.test.tsx | 39 +- .../Roadtrip/RoadtripAlternativesBar.tsx | 31 +- .../Roadtrip/RoadtripSidebar.test.tsx | 11 + .../components/Roadtrip/RoadtripSidebar.tsx | 7 +- .../Roadtrip/alternativeOverlays.test.ts | 68 +++- .../Roadtrip/alternativeOverlays.ts | 51 ++- .../Roadtrip/alternativePins.test.ts | 151 ++++++++ .../components/Roadtrip/alternativePins.ts | 100 ++++++ .../Roadtrip/useRoadtripRoutes.test.ts | 76 +++- .../components/Roadtrip/useRoadtripRoutes.ts | 145 ++++++-- .../Roadtrip/useRouteAlternatives.test.ts | 212 +++++++---- .../Roadtrip/useRouteAlternatives.ts | 173 ++++++--- .../trip/roadtrip/MRtAlternativesBar.tsx | 2 +- .../trip/roadtrip/useMRtAlternatives.ts | 16 +- .../useTripPlanner.roadtrip.test.tsx | 336 ++++++++++++++++-- .../src/pages/tripPlanner/useTripPlanner.ts | 174 +++++---- client/src/types.ts | 8 +- client/tests/helpers/legAlternatives.ts | 24 ++ .../tests/unit/mobile/trip/MMapArea.test.tsx | 3 +- .../unit/mobile/trip/MRoadtripTab.test.tsx | 5 +- .../mobile/trip/MRtAlternativesBar.test.tsx | 17 +- .../mobile/trip/useMRtAlternatives.test.ts | 21 +- .../tests/unit/nest/roadtrip-planning.test.ts | 16 + shared/src/i18n/ar/roadtrip.ts | 4 + shared/src/i18n/br/roadtrip.ts | 5 + shared/src/i18n/ca/roadtrip.ts | 5 + shared/src/i18n/cs/roadtrip.ts | 5 + shared/src/i18n/de/roadtrip.ts | 5 + shared/src/i18n/en/roadtrip.ts | 5 + shared/src/i18n/es/roadtrip.ts | 5 + shared/src/i18n/fr/roadtrip.ts | 6 + shared/src/i18n/gr/roadtrip.ts | 5 + shared/src/i18n/hu/roadtrip.ts | 5 + shared/src/i18n/id/roadtrip.ts | 5 + shared/src/i18n/it/roadtrip.ts | 5 + shared/src/i18n/ja/roadtrip.ts | 5 + shared/src/i18n/ko/roadtrip.ts | 4 + shared/src/i18n/nl/roadtrip.ts | 5 + shared/src/i18n/pl/roadtrip.ts | 5 + shared/src/i18n/ru/roadtrip.ts | 5 + shared/src/i18n/sv/roadtrip.ts | 5 + shared/src/i18n/tr/roadtrip.ts | 4 + shared/src/i18n/uk/roadtrip.ts | 5 + shared/src/i18n/vi/roadtrip.ts | 4 + shared/src/i18n/zh-TW/roadtrip.ts | 4 + shared/src/i18n/zh/roadtrip.ts | 4 + shared/src/roadtrip/assemble.spec.ts | 43 ++- shared/src/roadtrip/assemble.ts | 7 + shared/src/roadtrip/planning-types.ts | 14 + 53 files changed, 2121 insertions(+), 378 deletions(-) create mode 100644 client/src/components/Roadtrip/alternativePins.test.ts create mode 100644 client/src/components/Roadtrip/alternativePins.ts create mode 100644 client/tests/helpers/legAlternatives.ts diff --git a/client/src/components/Map/RouteCalculator.test.ts b/client/src/components/Map/RouteCalculator.test.ts index dc92b877d..40752bc36 100644 --- a/client/src/components/Map/RouteCalculator.test.ts +++ b/client/src/components/Map/RouteCalculator.test.ts @@ -13,6 +13,10 @@ import { parsePluginProfile, withHotelBookends, calculateAlternatives, + avoidedClasses, + routeEngineFor, + sameRoad, + furthestFrom, } from './RouteCalculator' // Every route now goes to the FOSSGIS per-profile hosts. The car-only project-osrm.org @@ -1049,3 +1053,196 @@ describe('turning round at a stop', () => { useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) }) }) + +// ── a trip that avoids something ────────────────────────────────────────────── + +/** + * The rail drives a trip with avoided classes through Valhalla, and everything that talks + * about one of its legs has to use the same engine. The offers used to come from OSRM with + * nothing avoided, so the picker described a different drive from the rail's; and an OSRM + * stand-in for a Valhalla that did not answer was filed as the avoided road for the rest + * of the session, without a word on the rail. + */ +describe('a trip that avoids something', () => { + const VALHALLA = 'https://valhalla1.openstreetmap.de/route' + + /** Polyline6, the way Valhalla sends its shapes. */ + const encode6 = (points: [number, number][]): string => { + const part = (value: number): string => { + let n = value < 0 ? ~(value << 1) : value << 1 + let out = '' + while (n >= 0x20) { out += String.fromCharCode((0x20 | (n & 0x1f)) + 63); n >>= 5 } + return out + String.fromCharCode(n + 63) + } + let lat = 0 + let lng = 0 + return points.map(([pLat, pLng]) => { + const a = Math.round(pLat * 1e6) + const b = Math.round(pLng * 1e6) + const piece = part(a - lat) + part(b - lng) + lat = a + lng = b + return piece + }).join('') + } + const trip = (points: [number, number][], summary: Record = {}) => ({ + legs: [{ shape: encode6(points) }], + summary: { length: 100, time: 4000, has_toll: false, has_highway: false, has_ferry: false, ...summary }, + }) + const NORTH: [number, number][] = [[61, 10], [61.5, 11], [61, 12]] + const SOUTH: [number, number][] = [[61, 10], [60.5, 11], [61, 12]] + const DIRECT: [number, number][] = [[61, 10], [61, 11], [61, 12]] + const from = { lat: 61, lng: 10 } + const to = { lat: 61, lng: 12 } + + it('FE-COMP-ROUTECALCULATOR-059: the ways come from Valhalla with the trip classes, and OSRM is never asked', async () => { + let osrm = 0 + const bodies: Array> = [] + server.use( + http.get(`${FOSSGIS.driving}/:coords`, () => { osrm++; return HttpResponse.json(buildOsrmRouteResponse()) }), + http.post(VALHALLA, async ({ request }) => { + bodies.push(await request.json() as Record) + return HttpResponse.json({ + trip: trip(DIRECT, { time: 3600 }), + alternates: [{ trip: trip(NORTH, { time: 4200, has_ferry: true }) }, { trip: trip(SOUTH, { time: 4800 }) }], + }) + }), + ) + + const routes = await calculateAlternatives(from, to, 'driving', { avoid: ['toll'] }) + + expect(osrm).toBe(0) + expect(bodies).toHaveLength(1) + expect(bodies[0]).toMatchObject({ alternates: 2, costing_options: { auto: { use_tolls: 0 } } }) + expect(bodies[0].locations).toHaveLength(2) + expect(routes.map(r => r.engine)).toEqual(['valhalla', 'valhalla', 'valhalla']) + expect(routes.map(r => r.duration)).toEqual([3600, 4200, 4800]) + expect(routes.map(r => r.hasFerry)).toEqual([false, true, false]) + // Pinnable like any other offer: measured against the preferred one. + expect(routes[0].divergence).toBeNull() + expect(routes[1].divergence).toEqual({ lat: 61.5, lng: 11 }) + }) + + it('FE-COMP-ROUTECALCULATOR-060: a Valhalla with nothing to say is the router not answering, not the OSRM roads', async () => { + let osrm = 0 + server.use( + http.get(`${FOSSGIS.driving}/:coords`, () => { osrm++; return HttpResponse.json(buildOsrmRouteResponse()) }), + http.post(VALHALLA, () => new HttpResponse(null, { status: 503 })), + ) + + await expect(calculateAlternatives({ lat: 62, lng: 10 }, { lat: 62, lng: 12 }, 'driving', { avoid: ['ferry'] })).rejects.toThrow() + expect(osrm).toBe(0) + }) + + it('FE-COMP-ROUTECALCULATOR-061: one way back is offered a second only for a class the trip does not avoid yet', async () => { + const asked: Array> = [] + server.use(http.post(VALHALLA, async ({ request }) => { + const body = await request.json() as { alternates?: number; costing_options: { auto: Record } } + asked.push(body.costing_options.auto) + if (body.alternates) return HttpResponse.json({ trip: trip(DIRECT, { has_toll: true }) }) + // Asked to leave out the tolls as well: a road that really is without them. + return HttpResponse.json({ trip: trip(NORTH, { time: 5000, has_toll: false }) }) + })) + + const routes = await calculateAlternatives({ lat: 61, lng: 10.5 }, { lat: 61, lng: 12 }, 'driving', { avoid: ['motorway'] }) + + // Motorways are avoided already, so the first extra question is about tolls, and it + // keeps the trip's own class in. + expect(asked).toEqual([{ use_highways: 0 }, { use_highways: 0, use_tolls: 0 }]) + expect(routes).toHaveLength(2) + expect(routes[1]).toMatchObject({ avoids: 'toll', engine: 'valhalla', duration: 5000 }) + }) + + it('FE-COMP-ROUTECALCULATOR-062: only a driving leg is weighed; a walk is asked of OSRM as before', async () => { + let valhalla = 0 + server.use( + http.get(`${FOSSGIS.walking}/:coords`, () => HttpResponse.json({ code: 'Ok', routes: [{ geometry: { coordinates: [[10, 61], [12, 61]] }, distance: 1000, duration: 900 }] })), + http.post(VALHALLA, () => { valhalla++; return HttpResponse.json({ trip: trip(DIRECT) }) }), + ) + + const routes = await calculateAlternatives(from, to, 'walking', { avoid: ['toll'] }) + + expect(valhalla).toBe(0) + expect(routes).toHaveLength(1) + expect(routes[0].engine).toBeUndefined() + expect(avoidedClasses('walking', ['toll'])).toEqual([]) + expect(avoidedClasses('driving', ['toll', 'ferry'])).toEqual(['ferry', 'toll']) + expect(routeEngineFor('driving', ['toll'])).toBe('valhalla') + expect(routeEngineFor('driving', [])).toBe('osrm') + expect(routeEngineFor('plugin:ev/fast', ['toll'])).toBe('plugin') + }) + + it('FE-COMP-ROUTECALCULATOR-063: an OSRM stand-in says nothing was avoided, and is not filed as the avoided road', async () => { + const [a, b] = freshWaypoints() + let osrm = 0 + let valhallaUp = false + server.use( + http.get(`${FOSSGIS.driving}/:coords`, () => { osrm++; return HttpResponse.json(buildLegsResponse()) }), + http.post(VALHALLA, () => (valhallaUp + ? HttpResponse.json({ trip: { legs: [{ shape: encode6([[a.lat, a.lng], [b.lat, b.lng]]), summary: { length: 5, time: 700, has_toll: false, has_ferry: true } }] } }) + : new HttpResponse(null, { status: 503 }))), + ) + + const fallback = await calculateRouteWithLegs([a, b], { avoid: ['toll'] }) + expect(osrm).toBe(1) + expect(fallback.avoidance).toEqual({ asked: ['toll'], achieved: [], fellBack: true }) + expect(fallback.distance).toBe(4200) + + // Valhalla is back: the same drive asks it again instead of keeping the OSRM answer. + valhallaUp = true + const weighed = await calculateRouteWithLegs([a, b], { avoid: ['toll'] }) + expect(weighed.avoidance).toEqual({ asked: ['toll'], achieved: ['toll'] }) + expect(weighed.distance).toBe(5000) + expect(weighed.hasFerry).toBe(true) + + // The stand-in was filed where it is true: as the plain OSRM answer, which an ordinary + // caller now gets without asking again, and without the avoidance attached. + const plain = await calculateRouteWithLegs([a, b]) + expect(osrm).toBe(1) + expect(plain.distance).toBe(4200) + expect(plain.avoidance).toBeUndefined() + }) + + it('FE-COMP-ROUTECALCULATOR-064: a road that runs the whole of another and turns off to a point and back is not that road', () => { + // The shape of a leg bent by a via: every point of the plain road lies on it, so + // asked only one way round the two read as one. + const plainRoad = { coordinates: [[61, 10], [61, 11], [61, 12]] as [number, number][], distance: 100_000, duration: 3600 } + const spur = { coordinates: [[61, 10], [61, 11], [61.3, 11], [61, 11], [61, 12]] as [number, number][], distance: 166_000, duration: 5400 } + + expect(sameRoad(plainRoad, spur)).toBe(false) + expect(sameRoad(spur, plainRoad)).toBe(false) + expect(sameRoad(plainRoad, { ...plainRoad, distance: 104_000, duration: 3900 })).toBe(true) + }) + + it('FE-COMP-ROUTECALCULATOR-065: one road drawn with a different spacing of points is still one road', () => { + // Two engines' lines of one long road share almost no vertex. Measured to the nearest + // sampled vertex, a point on the same road sat half a sample off it: kilometres here. + // The dense line is sampled every tenth vertex, 0.05 degrees apart; the sparse one sits + // exactly between those samples, 1.8 km from the nearest, on the very same road. + const dense = Array.from({ length: 2001 }, (_, i) => [50, i * 0.005] as [number, number]) + const sparse: [number, number][] = [ + [50, 0], + ...Array.from({ length: 200 }, (_, i) => [50, 0.025 + i * 0.05] as [number, number]), + [50, 10], + ] + const a = { coordinates: dense, distance: 700_000, duration: 25_000 } + const b = { coordinates: sparse, distance: 712_000, duration: 27_000 } + + expect(sameRoad(a, b)).toBe(true) + const off = furthestFrom([[50.02, 5]], dense) + expect(off?.index).toBe(0) + expect(off?.km).toBeCloseTo(2.22, 1) + }) + + it('FE-COMP-ROUTECALCULATOR-066: the avoidance offer beside an OSRM leg says when it crosses by ferry', async () => { + server.use( + http.get(`${FOSSGIS.driving}/:coords`, () => HttpResponse.json({ code: 'Ok', routes: [{ geometry: { coordinates: [[10, 63], [11, 63], [12, 63]] }, distance: 100000, duration: 3600 }] })), + http.post(VALHALLA, () => HttpResponse.json({ trip: trip([[63, 10], [63.6, 11], [63, 12]], { has_highway: false, has_ferry: true }) })), + ) + + const routes = await calculateAlternatives({ lat: 63, lng: 10 }, { lat: 63, lng: 12 }) + + expect(routes).toHaveLength(2) + expect(routes[1]).toMatchObject({ avoids: 'motorway', engine: 'valhalla', hasFerry: true }) + }) +}) diff --git a/client/src/components/Map/RouteCalculator.ts b/client/src/components/Map/RouteCalculator.ts index d201ee2de..e8eb59bbe 100644 --- a/client/src/components/Map/RouteCalculator.ts +++ b/client/src/components/Map/RouteCalculator.ts @@ -4,7 +4,7 @@ import type { DistanceUnit, RouteResult, RouteSegment, RouteWithLegs, SnappedWay import { haversineKm } from '../../utils/geo' import { formatDistance } from '../../utils/units' import { countRoute } from './routeUsageCounter' -import { valhallaRouteAvoiding, valhallaRun, valhallaAvailable, legAvoids, type AvoidClass } from './valhallaRoute' +import { valhallaRouteAvoiding, valhallaRun, valhallaAvailable, valhallaAlternates, valhallaSpacing, legAvoids, type AvoidClass, type ValhallaLeg } from './valhallaRoute' import type { RouteUsageSurface } from '@trek/shared' // FOSSGIS hosts OSRM with real per-profile routing (car/foot/bike) — the @@ -216,6 +216,33 @@ export function parsePluginProfile(profile: string): { pluginId: string; profile return { pluginId: rest.slice(0, slash), profileId: rest.slice(slash + 1) } } +/** + * Which engine priced a route: OSRM, the second engine that weighs road classes away, or + * a route provider plugin. Their figures are three different speed models, so a figure + * from one read against a figure from another measures the models, not the roads. + */ +export type RouteEngine = 'osrm' | 'valhalla' | 'plugin' + +/** + * The classes a request really weighs away, in a stable order, and none when it cannot. + * + * Only the driving profile has classes to leave out and only Valhalla can leave them out, + * so every other request is asked plainly whatever the trip says. One function for the + * day routing and for the offers beside it, so the two cannot disagree about which engine + * a leg belongs to. + */ +export function avoidedClasses(profile: RouteProfileKey, avoid: readonly AvoidClass[]): AvoidClass[] { + return profile === 'driving' && avoid.length > 0 && valhallaAvailable() + ? [...avoid].sort((a, b) => a.localeCompare(b)) + : [] +} + +/** The engine `calculateRouteWithLegs` hands a request with this profile and avoidance to. */ +export function routeEngineFor(profile: RouteProfileKey, avoid: readonly AvoidClass[]): RouteEngine { + if (parsePluginProfile(profile)) return 'plugin' + return avoidedClasses(profile, avoid).length ? 'valhalla' : 'osrm' +} + /** Fetches a full route via OSRM and returns coordinates, distance, and duration estimates for driving/walking. */ export async function calculateRoute( waypoints: Waypoint[], @@ -480,9 +507,7 @@ export async function calculateRouteWithLegs( return { coordinates: [], distance: 0, duration: 0, legs: [] } } - const avoiding = profile === 'driving' && avoid.length > 0 && valhallaAvailable() - ? [...avoid].sort((a, b) => a.localeCompare(b)) - : [] + const avoiding = avoidedClasses(profile, avoid) const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') // The cached result carries formatted leg distances, so the active distance unit is @@ -497,23 +522,30 @@ export async function calculateRouteWithLegs( // way is worse: this Map is shared with the ordinary day planner, the studio and the // booking geometry, none of which ask for avoidance, so an avoid-routed answer stored // under the plain key would hand them a road nobody drives and a second engine's - // times. `avoiding` is empty whenever the request goes to OSRM after all, so the key - // also separates the two engines. + // times. So the avoiding key only ever holds a Valhalla answer: when Valhalla cannot + // answer and OSRM drives the request after all, that answer is filed under the plain + // key, where it is exactly what an ordinary caller would have got. Filed under the + // avoiding key it stood in for the avoided road until the next reload, however soon + // Valhalla came back. const pluginScope = profile.startsWith('plugin:') ? `:${tripId ?? ''}:${dayId ?? ''}` : '' const avoidScope = avoiding.length ? `:avoid=${avoiding.join(',')}` : '' - const cacheKey = `${profile}:${getDistanceUnit()}:${coords}${pluginScope}${avoidScope}` + const plainKey = `${profile}:${getDistanceUnit()}:${coords}${pluginScope}` + const cacheKey = `${plainKey}${avoidScope}` const cached = routeCache.get(cacheKey) if (cached) return cached if (avoiding.length) { const routed = await routeAvoidingWithLegs(waypoints, avoiding, signal) - // Null is a Valhalla that could not answer at all. Falling through to OSRM keeps a - // day drawn rather than blank, and `avoidance` stays absent on that result so the - // rail can say the avoidance did not happen instead of implying it did. if (routed) { cacheRoute(cacheKey, routed) return routed } + // Null is a Valhalla that could not answer at all. OSRM keeps the day drawn rather + // than blank, and the answer says that nothing was avoided, with the reason, so the + // rail flags the day instead of implying the setting held on a road nobody weighed. + const plain = routeCache.get(plainKey) ?? await osrmWithLegs(waypoints, coords, profile, signal) + cacheRoute(plainKey, plain) + return { ...plain, avoidance: { asked: avoiding, achieved: [], fellBack: true } } } // Plugin profile (`plugin:/`): the server invokes that routeProvider @@ -558,6 +590,18 @@ export async function calculateRouteWithLegs( return result } + const result = await osrmWithLegs(waypoints, coords, profile, signal) + cacheRoute(cacheKey, result) + return result +} + +/** The OSRM answer for a chain, one leg per waypoint pair. Throws like every OSRM call here. */ +async function osrmWithLegs( + waypoints: Waypoint[], + coords: string, + profile: RouteProfileKey, + signal?: AbortSignal, +): Promise { // Written as literals rather than narrowing `profile`: its type is an open string union // (plugins name their own modes), which no comparison narrows to the three OSRM knows. const osrmProfile: 'driving' | 'walking' | 'cycling' = @@ -592,9 +636,7 @@ export async function calculateRouteWithLegs( ) const snapped = readSnapped(data, waypoints) - const result: RouteWithLegs = { coordinates, distance: route.distance, duration: route.duration, legs, ...(snapped ? { snapped } : {}) } - cacheRoute(cacheKey, result) - return result + return { coordinates, distance: route.distance, duration: route.duration, legs, ...(snapped ? { snapped } : {}) } } /** Store a route and drop the oldest once the cache is over its cap. */ @@ -659,6 +701,7 @@ async function routeAvoidingWithLegs( legs, snapped: run.snapped, avoidance: { asked: avoid, achieved: avoid.filter(cls => legAvoids(run.total, cls)) }, + hasFerry: run.total.hasFerry, } } @@ -714,7 +757,7 @@ function formatDuration(seconds: number): string { return `${m} min` } -/** One way of driving a leg, as OSRM offers it. */ +/** One way of driving a leg, as a router offers it. */ export interface RouteAlternative { coordinates: [number, number][] distance: number @@ -724,20 +767,24 @@ export interface RouteAlternative { /** Set when this way exists because a road class was left out of it. */ avoids?: AvoidClass /** - * Which engine priced this route, when it was not the one that priced the rest. + * Which engine priced this route. Absent means OSRM, which is what answers every route + * of a trip that avoids nothing, except the avoidance offer on a default install. * - * Absent for everything OSRM answered, which is every route in a list except the - * avoidance offer on a default install. It exists because the two engines do not - * agree on speed: measured over twelve European legs the per-leg drive times run - * from 14.7 % under OSRM's to 13.2 % over, and the sign depends on the region: - * Valhalla is faster on Spanish autovía and slower through a city. So a figure from - * one of them subtracted from a figure from the other is not a difference in - * driving time, it is the gap between two speed models, and the reader has no way - * of telling the two apart. + * It exists because the engines do not agree on speed: measured over twelve European + * legs the per-leg drive times run from 14.7 % under OSRM's to 13.2 % over, and the + * sign depends on the region: Valhalla is faster on Spanish autovía and slower through + * a city. So a figure from one of them subtracted from a figure from the other is not a + * difference in driving time, it is the gap between two speed models, and the reader + * has no way of telling the two apart. */ - engine?: 'valhalla' + engine?: RouteEngine + /** Whether the way crosses by ferry, where the engine that drew it says so. */ + hasFerry?: boolean } +/** What two routes are compared by: their line, and their figures where there is none. */ +export type RoadLine = Pick + /** * Road classes worth asking the router to leave out, in the order they are tried. * @@ -780,10 +827,16 @@ const SAME_ROAD_KM = 0.5 * * The geometry does not care which engine drew it, so it decides whenever there is one. * The numbers stay as the fallback for a line with no shape to compare. + * + * Measured both ways round. Asked only whether `b` stays near `a`, a road that runs the + * whole of `b` and then turns off to a point and back passed as `b`, because every point + * of `b` is on it; that is exactly the shape of a leg bent by a via, and the picker and + * the check behind a choice both have to tell it from the plain road. */ -function sameRoad(a: RouteAlternative, b: RouteAlternative): boolean { - const stray = furthestFrom(b.coordinates, a.coordinates) - if (stray) return stray.km < SAME_ROAD_KM +export function sameRoad(a: RoadLine, b: RoadLine): boolean { + const there = furthestFrom(b.coordinates, a.coordinates) + const back = furthestFrom(a.coordinates, b.coordinates) + if (there && back) return Math.max(there.km, back.km) < SAME_ROAD_KM return Math.abs(a.distance - b.distance) < 200 && Math.abs(a.duration - b.duration) < 60 } @@ -862,12 +915,18 @@ async function valhallaExcluding( // and every routing request an instance makes still lands in the one counter. if (!signal?.aborted) countRoute({ ...sample, failed: !leg }) if (!leg || !legAvoids(leg, exclude)) return null + return valhallaOffer(leg) +} + +/** A Valhalla leg as an offer, carrying the engine that priced it and what it crosses. */ +function valhallaOffer(leg: ValhallaLeg): RouteAlternative { return { coordinates: leg.coordinates, distance: leg.distance, duration: leg.duration, divergence: null, engine: 'valhalla', + hasFerry: leg.hasFerry, } } @@ -917,12 +976,89 @@ async function osrmExcluding( * It is what makes a choice persistable: saving that point as a via forces the router back * onto this road on every future request, without storing a polyline that would go stale * with the next OSM update. + * + * `avoid` is the classes the trip weighs away. A driving leg of such a trip is routed by + * Valhalla, so its offers come from Valhalla too, weighed the same way, and never from + * OSRM: a list holding both engines is what offered a road the rail was not on as the + * quicker one, and clicking it changed nothing. */ export async function calculateAlternatives( from: Waypoint, to: Waypoint, profile: 'driving' | 'walking' | 'cycling' = 'driving', - { signal, limit = 3 }: { signal?: AbortSignal; limit?: number } = {}, + { signal, limit = 3, avoid = [] }: { signal?: AbortSignal; limit?: number; avoid?: readonly AvoidClass[] } = {}, +): Promise { + const avoiding = avoidedClasses(profile, avoid) + const routes = avoiding.length + ? await alternativesAvoiding(from, to, avoiding, limit, signal) + : await alternativesFromOsrm(from, to, profile, limit, signal) + if (!routes.length) return routes + + // The first route is what the router would have given anyway; the others are measured + // against it so each one can be pinned by the point that makes it different. + const [primary, ...rest] = routes + for (const alt of rest) alt.divergence = furthestPointFrom(alt.coordinates, primary.coordinates) + return routes +} + +/** + * The offers for a leg of a trip that weighs classes away, every one of them Valhalla's. + * + * Throws when Valhalla has nothing to say, so the picker reports a router that is not + * answering. Quietly offering OSRM's roads instead is the very mix this path exists to end. + * + * One way back gets the question the OSRM path asks too: the same drive with one more + * class left out, only of those the trip does not already avoid, and only where the road + * that comes back really is without it. Paced like the pieces of a split day, because the + * public instance allows one request a second. + */ +async function alternativesAvoiding( + from: Waypoint, + to: Waypoint, + avoiding: AvoidClass[], + limit: number, + signal?: AbortSignal, +): Promise { + const sample = { + profile: 'driving' as const, + surface: 'alternatives' as const, + selfHosted: !!useSettingsStore.getState().settings.routing_base_url?.trim(), + waypoints: 2, + km: haversineKm(from, to), + } + const answered = await valhallaAlternates(from, to, 'driving', avoiding, Math.max(0, limit - 1), signal) + if (!signal?.aborted) countRoute({ ...sample, failed: !answered }) + if (!answered?.length) throw new Error('No route found') + + const routes: RouteAlternative[] = [] + for (const leg of answered) { + const offer = valhallaOffer(leg) + if (!routes.some(r => sameRoad(r, offer))) routes.push(offer) + } + if (routes.length >= 2) return routes + + for (const cls of EXCLUDABLE_CLASSES) { + if (avoiding.includes(cls)) continue + await valhallaSpacing(signal) + if (signal?.aborted) break + const run = await valhallaRun([from, to], 'driving', [...avoiding, cls], signal) + if (!signal?.aborted) countRoute({ ...sample, failed: !run }) + if (!run || !legAvoids(run.total, cls)) continue + const detour = valhallaOffer(run.total) + if (routes.some(r => sameRoad(r, detour))) continue + routes.push({ ...detour, avoids: cls }) + break + } + return routes +} + +/** The offers for a leg of a trip that avoids nothing: OSRM's, and one avoidance offer. */ +async function alternativesFromOsrm( + from: Waypoint, + to: Waypoint, + profile: 'driving' | 'walking' | 'cycling', + limit: number, + signal?: AbortSignal, ): Promise { const coords = `${from.lng},${from.lat};${to.lng},${to.lat}` const url = `${routeBaseFor(profile)}/${coords}?alternatives=${limit}&overview=full&geometries=geojson` @@ -960,11 +1096,6 @@ export async function calculateAlternatives( } } } - - // The first route is what the router would have given anyway; the others are measured - // against it so each one can be pinned by the point that makes it different. - const [primary, ...rest] = routes - for (const alt of rest) alt.divergence = furthestPointFrom(alt.coordinates, primary.coordinates) return routes } @@ -982,31 +1113,78 @@ function furthestPointFrom(line: [number, number][], reference: [number, number] * nothing for it. The winner is converted once at the end, because `sameRoad` needs a * real distance rather than a rank, and half a degree is a different number of metres in * Andalusia than in Lapland. + * + * `index` is where on `line` the point sits, so points taken off one line can be put back + * in the order that line drives through them. */ -function furthestFrom( +export function furthestFrom( line: [number, number][], reference: [number, number][], -): { lat: number; lng: number; km: number } | null { +): { index: number; lat: number; lng: number; km: number } | null { if (!line.length || !reference.length) return null // Every tenth vertex is plenty: alternatives differ over kilometres, not metres, and a // full cross product of two thousand-point lines is not worth the milliseconds. const step = Math.max(1, Math.floor(reference.length / 200)) - let best: { lat: number; lng: number } | null = null + let bestIndex = -1 let bestNeighbour: [number, number] | null = null let bestDist = -1 for (let i = 0; i < line.length; i += Math.max(1, Math.floor(line.length / 200))) { - const [lat, lng] = line[i] - let nearest = Infinity - let nearestPoint: [number, number] | null = null - for (let j = 0; j < reference.length; j += step) { - const dLat = lat - reference[j][0] - const dLng = (lng - reference[j][1]) * Math.cos((lat * Math.PI) / 180) - const d = dLat * dLat + dLng * dLng - if (d < nearest) { nearest = d; nearestPoint = reference[j] } - } - if (nearest > bestDist) { bestDist = nearest; best = { lat, lng }; bestNeighbour = nearestPoint } + const near = nearestOn(line[i], reference, step) + if (near.d > bestDist) { bestDist = near.d; bestIndex = i; bestNeighbour = near.at } } - if (!best) return null - const km = bestNeighbour ? haversineKm(best, { lat: bestNeighbour[0], lng: bestNeighbour[1] }) : 0 - return { ...best, km } + if (bestIndex < 0) return null + const [lat, lng] = line[bestIndex] + const km = bestNeighbour ? haversineKm({ lat, lng }, { lat: bestNeighbour[0], lng: bestNeighbour[1] }) : 0 + return { index: bestIndex, lat, lng, km } +} + +/** + * Where `reference` passes closest to `point`, as a squared distance in the same scaled + * degrees and the spot on the line it was measured to. + * + * Coarse, then fine. The sampled vertices find the stretch, and the segments either side + * of the nearest one find the road itself. Measured to the sampled vertex alone, a point + * on the very same road sat up to half a sample apart from it, which on a thousand + * kilometre leg is kilometres: two engines' lines of one motorway then read as two roads. + */ +function nearestOn( + point: [number, number], + reference: [number, number][], + step: number, +): { d: number; at: [number, number] } { + const cos = Math.cos((point[0] * Math.PI) / 180) + const squared = (p: [number, number]): number => { + const dLat = point[0] - p[0] + const dLng = (point[1] - p[1]) * cos + return dLat * dLat + dLng * dLng + } + let nearestIndex = 0 + let nearest = Infinity + for (let j = 0; j < reference.length; j += step) { + const d = squared(reference[j]) + if (d < nearest) { nearest = d; nearestIndex = j } + } + let at = reference[nearestIndex] + const last = Math.min(reference.length - 1, nearestIndex + step) + for (let k = Math.max(0, nearestIndex - step); k < last; k++) { + const on = closestOnSegment(point, reference[k], reference[k + 1], cos) + const d = squared(on) + if (d < nearest) { nearest = d; at = on } + } + return { d: nearest, at } +} + +/** The point of segment a-b closest to `p`, with longitude scaled by `cos` so the plane is fair. */ +function closestOnSegment( + p: [number, number], + a: [number, number], + b: [number, number], + cos: number, +): [number, number] { + const dx = (b[1] - a[1]) * cos + const dy = b[0] - a[0] + const length = dx * dx + dy * dy + if (length === 0) return a + const t = Math.max(0, Math.min(1, (((p[1] - a[1]) * cos) * dx + (p[0] - a[0]) * dy) / length)) + return [a[0] + (b[0] - a[0]) * t, a[1] + (b[1] - a[1]) * t] } diff --git a/client/src/components/Map/valhallaRoute.test.ts b/client/src/components/Map/valhallaRoute.test.ts index 14526e08e..49b1c7f5b 100644 --- a/client/src/components/Map/valhallaRoute.test.ts +++ b/client/src/components/Map/valhallaRoute.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, afterEach } from 'vitest' import { http, HttpResponse } from 'msw' import { server } from '../../../tests/helpers/msw/server' import { useSettingsStore } from '../../store/settingsStore' -import { valhallaBase, valhallaAvailable, valhallaRouteAvoiding, valhallaRun, runFrom, legAvoids } from './valhallaRoute' +import { valhallaBase, valhallaAvailable, valhallaRouteAvoiding, valhallaRun, valhallaAlternates, alternatesFrom, runFrom, legAvoids } from './valhallaRoute' const FOSSGIS_VALHALLA = 'https://valhalla1.openstreetmap.de/route' @@ -365,3 +365,62 @@ describe('valhallaRun over a whole day', () => { await expect(valhallaRun(stops(6), 'driving', [])).resolves.toBeNull() }) }) + +describe('valhallaAlternates', () => { + /** The same leg read three ways, the second of them crossing by ferry. */ + const withAlternates = (alternates: unknown[]) => ({ + ...answer({ time: 11_460 }), + alternates, + }) + + it('VALHALLA-ALT-001: reads the preferred way, then every alternate, each with its own flags', () => { + const ways = alternatesFrom(withAlternates([ + answer({ time: 12_000, has_ferry: true }), + answer({ time: 13_000, has_toll: true }), + ])) + + expect(ways?.map(w => w.duration)).toEqual([11_460, 12_000, 13_000]) + expect(ways?.map(w => w.hasFerry)).toEqual([false, true, false]) + expect(ways?.map(w => w.hasToll)).toEqual([false, false, true]) + expect(ways?.[1].coordinates).toEqual(SHAPE_POINTS) + }) + + it('VALHALLA-ALT-002: an alternate that cannot be read is dropped, a preferred way that cannot is no answer', () => { + // One bad entry is no reason to lose the leg's other ways, but the others are offered + // against the preferred one and mean nothing without it. + expect(alternatesFrom(withAlternates([{ trip: { legs: [] } }, answer({ time: 13_000 })]))?.map(w => w.duration)) + .toEqual([11_460, 13_000]) + expect(alternatesFrom(answer())).toHaveLength(1) + expect(alternatesFrom({ alternates: [answer()] })).toBeNull() + expect(alternatesFrom({ error_code: 442, error: 'No path could be found for input' })).toBeNull() + }) + + it('VALHALLA-ALT-003: asks for the leg between exactly its two ends, weighed the way the trip is, with the number of alternates', async () => { + const bodies: Record[] = [] + server.use(http.post(FOSSGIS_VALHALLA, async ({ request }) => { + bodies.push(await request.json() as Record) + return HttpResponse.json(withAlternates([answer({ time: 12_000 })])) + })) + + const ways = await valhallaAlternates(HAMBURG, BERLIN, 'driving', ['toll', 'ferry'], 2) + await valhallaAlternates(HAMBURG, BERLIN, 'driving', ['toll'], 0) + + expect(ways).toHaveLength(2) + expect(bodies[0]).toMatchObject({ alternates: 2, costing: 'auto', costing_options: { auto: { use_tolls: 0, use_ferry: 0 } } }) + expect(bodies[0].locations).toHaveLength(2) + // No alternates asked for, none named: the request stays the plain one. + expect(bodies[1]).not.toHaveProperty('alternates') + }) + + it('VALHALLA-ALT-004: a refusal is asked once more, then is no answer; an instance with its own OSRM asks nothing', async () => { + let calls = 0 + server.use(http.post(FOSSGIS_VALHALLA, () => { calls++; return new HttpResponse(null, { status: 429 }) })) + + await expect(valhallaAlternates(HAMBURG, BERLIN, 'driving', ['toll'], 2)).resolves.toBeNull() + expect(calls).toBe(2) + + setSettings({ routing_base_url: 'https://osrm.example.org' }) + await expect(valhallaAlternates(HAMBURG, BERLIN, 'driving', ['toll'], 2)).resolves.toBeNull() + expect(calls).toBe(2) + }) +}) diff --git a/client/src/components/Map/valhallaRoute.ts b/client/src/components/Map/valhallaRoute.ts index fe736c0fd..5c4369327 100644 --- a/client/src/components/Map/valhallaRoute.ts +++ b/client/src/components/Map/valhallaRoute.ts @@ -204,14 +204,7 @@ export async function valhallaRun( // rate limit on the last piece of a split day would otherwise throw away the pieces // that already answered and ask for all of them again — three times over, against a // host that allows one request a second. - let run = await requestRun(base, chunk, profile, avoid, signal) - for (let attempt = 0; !run && attempt < CHUNK_RETRIES && !signal?.aborted; attempt++) { - // Not when the host just proved it is not a Valhalla at all: that answer will not - // change on a second ask, and asking anyway is the request the memo exists to save. - if (notValhalla.has(base)) break - await pause(CHUNK_PAUSE_MS, signal) - run = await requestRun(base, chunk, profile, avoid, signal) - } + const run = await askWithRetry(base, () => requestRun(base, chunk, profile, avoid, signal), signal) // A day is one answer or none. Half a day of legs would be worse than no answer: // the schedule chains leg times blindly and cannot tell that it is missing some, so // it would print a complete, plausible, wrong timetable rather than a gap. @@ -260,14 +253,39 @@ function pause(ms: number, signal?: AbortSignal): Promise { }) } -/** One request, at most ten locations. */ -async function requestRun( - base: string, +/** + * Waits out the host's one request a second before the next question is put. + * + * For a caller that asks this engine several things in a row on its own, such as the + * offers for one leg: the same pause the pieces of a split day keep, so a picker cannot + * fire at a pace the day routing was careful never to. + */ +export function valhallaSpacing(signal?: AbortSignal): Promise { + return pause(CHUNK_PAUSE_MS, signal) +} + +/** + * One question, and one more try after a pause when it earned no answer. + * + * Not when the host just proved it is not a Valhalla at all: that answer will not change + * on a second ask, and asking anyway is the request the memo exists to save. + */ +async function askWithRetry(base: string, ask: () => Promise, signal?: AbortSignal): Promise { + let answer = await ask() + for (let attempt = 0; !answer && attempt < CHUNK_RETRIES && !signal?.aborted; attempt++) { + if (notValhalla.has(base)) break + await pause(CHUNK_PAUSE_MS, signal) + answer = await ask() + } + return answer +} + +/** The request body for a chain of waypoints, weighted away from `avoid`. */ +function routeBody( waypoints: Waypoint[], profile: 'driving' | 'walking' | 'cycling', avoid: readonly AvoidClass[], - signal?: AbortSignal, -): Promise | null> { +): Record { const costing = COSTING[profile] // All of them in one request: the options are independent weightings, so asking to // leave out tolls and ferries together is one question rather than two routes to @@ -275,13 +293,16 @@ async function requestRun( // away together answered 656.9 km against 339.2 km for the plain route. const options: Record = {} for (const cls of avoid) options[AVOID_OPTION[cls]] = 0 - const body = { + return { locations: waypoints.map(w => ({ lat: w.lat, lon: w.lng, radius: SNAP_RADIUS_M })), costing, costing_options: { [costing]: options }, directions_options: { units: 'kilometers' }, } +} +/** The parsed answer to one POST, or null for every failure. */ +async function postRoute(base: string, body: Record, signal?: AbortSignal): Promise { try { const response = await fetch(`${base}/route`, { method: 'POST', @@ -299,12 +320,66 @@ async function requestRun( if (response.status === 404 || response.status === 405) notValhalla.add(base) return null } - return runFrom(await response.json()) + return await response.json() } catch { return null } } +/** One request, at most ten locations. */ +async function requestRun( + base: string, + waypoints: Waypoint[], + profile: 'driving' | 'walking' | 'cycling', + avoid: readonly AvoidClass[], + signal?: AbortSignal, +): Promise | null> { + const data = await postRoute(base, routeBody(waypoints, profile, avoid), signal) + return data === null ? null : runFrom(data) +} + +/** + * The ways of driving one leg this engine would offer, its preferred one first, or null. + * + * The offers for a trip that avoids something have to come from the engine that routes + * that trip's days, or the list and the rail describe two different drives: OSRM's + * quickest road beside a day Valhalla had routed was offered as the faster way, and + * taking it changed nothing. Valhalla answers `alternates` between exactly two locations, + * which is all one leg is, and weighs every one of them the way it weighed the day. + */ +export async function valhallaAlternates( + from: Waypoint, + to: Waypoint, + profile: 'driving' | 'walking' | 'cycling', + avoid: readonly AvoidClass[], + count: number, + signal?: AbortSignal, +): Promise { + const base = valhallaBase() + if (!base || notValhalla.has(base)) return null + const body = { ...routeBody([from, to], profile, avoid), ...(count > 0 ? { alternates: count } : {}) } + return askWithRetry(base, async () => { + const data = await postRoute(base, body, signal) + return data === null ? null : alternatesFrom(data) + }, signal) +} + +/** + * Every way in an answer that asked for alternates: `trip` first, then each + * `alternates[].trip`, read the way `runFrom` reads a single leg. + * + * Null when the preferred one cannot be read, because the others are offered against it. + * An alternate that cannot be read is dropped on its own: one bad entry is no reason to + * lose the leg's other ways. + */ +export function alternatesFrom(data: unknown): ValhallaLeg[] | null { + const primary = runFrom(data) + if (!primary) return null + const extra = (data as { alternates?: unknown })?.alternates + const others = Array.isArray(extra) ? extra.map(alt => runFrom(alt)?.total ?? null) : [] + return [primary.total, ...others.filter((leg): leg is ValhallaLeg => leg !== null)] +} + /** * Several legs read as one, the way a caller that only wants the whole drive sees it. * diff --git a/client/src/components/Roadtrip/RoadtripAlternativesBar.test.tsx b/client/src/components/Roadtrip/RoadtripAlternativesBar.test.tsx index fa9034809..3bdc18cbd 100644 --- a/client/src/components/Roadtrip/RoadtripAlternativesBar.test.tsx +++ b/client/src/components/Roadtrip/RoadtripAlternativesBar.test.tsx @@ -5,9 +5,10 @@ import { useSettingsStore } from '../../store/settingsStore' import RoadtripAlternativesBar from './RoadtripAlternativesBar' import type { AlternativeOverlay } from './alternativeOverlays' import type { LegAlternatives } from './useRouteAlternatives' +import { openLeg } from '../../../tests/helpers/legAlternatives' /** - * FE-ALTBAR-001..009 — the ways of driving one leg, offered over the map. + * FE-ALTBAR-001..012: the ways of driving one leg, offered over the map. * * Every value shown comes from the same overlay the map draws. Working them out * here separately is what once made the list label the driven route "Fastest" @@ -31,9 +32,7 @@ const overlay = (over: Partial = {}): AlternativeOverlay => ...over, }) -const leg = (over: Partial = {}): LegAlternatives => ({ - dayId: 4, index: 1, routes: [], loading: false, error: false, ...over, -}) +const leg = (over: Partial = {}): LegAlternatives => openLeg({ dayId: 4, index: 1, ...over }) beforeEach(() => { useSettingsStore.setState({ settings: { distance_unit: 'metric' } as never }) @@ -158,4 +157,36 @@ describe('RoadtripAlternativesBar', () => { ) expect(screen.getAllByLabelText(/avoidance router/i)).toHaveLength(1) }) + + it('FE-ALTBAR-011: while a choice is checked and saved the list stands still, and the one in hand says so', () => { + // A second click would race the first, and the router's answer to it could land on + // the leg after it. + const onChoose = vi.fn() + render( + , + ) + const [, current, checked, other] = screen.getAllByRole('button') + expect([current, checked, other].every(chip => (chip as HTMLButtonElement).disabled)).toBe(true) + expect(checked).toHaveAttribute('aria-busy', 'true') + expect(other).toHaveAttribute('aria-busy', 'false') + expect(checked.parentElement).toHaveAttribute('aria-busy', 'true') + fireEvent.click(other) + expect(onChoose).not.toHaveBeenCalled() + // Closing stays available: it is how a check nobody wants to wait for is abandoned. + expect(screen.getByRole('button', { name: /close/i })).not.toBeDisabled() + }) + + it('FE-ALTBAR-012: with nothing being checked every offer can be taken', () => { + render( + , + ) + const chips = screen.getAllByRole('button').slice(1) + expect(chips.every(chip => !(chip as HTMLButtonElement).disabled)).toBe(true) + expect(chips[0].parentElement).toHaveAttribute('aria-busy', 'false') + }) }) diff --git a/client/src/components/Roadtrip/RoadtripAlternativesBar.tsx b/client/src/components/Roadtrip/RoadtripAlternativesBar.tsx index 02cb319be..9de8a62ac 100644 --- a/client/src/components/Roadtrip/RoadtripAlternativesBar.tsx +++ b/client/src/components/Roadtrip/RoadtripAlternativesBar.tsx @@ -1,11 +1,11 @@ import React from 'react' -import { Shuffle, X, AlertTriangle, Info } from 'lucide-react' +import { Shuffle, X, AlertTriangle, Info, Loader2 } from 'lucide-react' import { useTranslation } from '../../i18n/TranslationContext' import { Tooltip } from '../shared/Tooltip' import { useSettingsStore } from '../../store/settingsStore' import { formatDistance } from '../../utils/units' import type { LegAlternatives } from './useRouteAlternatives' -import { alternativesPhase, alternativeSubline, type AlternativeOverlay } from './alternativeOverlays' +import { alternativesBusy, alternativesPhase, alternativeSubline, type AlternativeOverlay } from './alternativeOverlays' interface RoadtripAlternativesBarProps { open: LegAlternatives | null @@ -35,6 +35,9 @@ export default function RoadtripAlternativesBar({ const distanceUnit = useSettingsStore(s => s.settings.distance_unit) if (!open) return null const phase = alternativesPhase(open, overlays) + // While a choice is checked against the router and written, the list stands still: a + // second click would race the first, and its answer could land on the leg after it. + const busy = alternativesBusy(open) return (
@@ -64,25 +67,31 @@ export default function RoadtripAlternativesBar({ // One route back means there is genuinely only one sensible way to drive it.

{t('roadtrip.alt.onlyOne')}

) : ( -
+
{overlays.map(alt => (