nrenner / brouter-web

Web client for BRouter, a routing engine based on OpenStreetMap
https://brouter.de/brouter-web/
MIT License
347 stars 65 forks source link

Use the newer geo-data-exchange library #781

Closed alexcojocaru closed 3 weeks ago

alexcojocaru commented 7 months ago

The newer library has a more accurate gradient calculation algorithm: there is no more segmentation of the track into 200m segments, and so the resulting features (i.e. portions of the track with constant gradient) map much better to the elevation chart.

Before: Screenshot_2023-11-13_17-38-29

After: Screenshot_2023-11-13_17-37-46

rkflx commented 6 months ago

Now the amount of smoothing depends on the route length, i.e. displayed slope values become unpredictable and can change as route planning progresses.

We should focus on accuracy, for which the first commit is a welcome improvement. However, the second commit seems to be mainly concerned with producing pretty visualizations, at the expense of other requirements. Can we have both, i.e. keep the underlying data accurate (for display in the mouseover popups of both the heightgraph and the route), and restrict smoothing to the heightgraph visualization itself?

The concept of normalization also conflicts with another use case: The heightgraph colorization can be employed to indicate short segments with very steep slopes for route planning purposes, which now would simply get hidden. Perhaps normalization (of the visualization only) can be optional, e.g. off by default with a toggle to enable it.

Finally, starting with a small window width and later switching to fullscreen and showing the heightgraph results in quite wide segments and lots of smoothing of slope values in the mouseover popups. Having the normalization depend on the initial window size is at least quite unintuitive. Maybe having two static preconfigured values for chartWidthInPixels (one regular and one for when the toolbar at the top is collapsed) might be feasible (since dynamically updating the width has its own disadvantages: performance impact due to requiring recomputation and surprising visualization changes during window resizes).

alexcojocaru commented 6 months ago

We should focus on accuracy ... The concept of normalization also conflicts with another use case: The heightgraph colorization can be employed to indicate short segments with very steep slopes for route planning purposes, which now would simply get hidden.

IIRC @nrenner had a similar ask on the PR which introduced Heightgraph. The first commit in this PR does that - it tracks all gradient changes and plots them on the chart, but the result is almost unreadable in most of the cases. I did not implement/enable normalization for beautification purposes, but to make the chart more readable. At the end of this comment is an example of an elevation profile , without and with normalization.

If showing accurate gradients is preferred, I am more than happy to disable the normalization (it'd be a very easy change).

As for normalizing:

  1. There is another normalization parameter which I haven't overridden : https://github.com/alexcojocaru/geo-data-exchange/blob/master/src/index.js#L31 . I could set it to a lower value (e.g. 3px), to keep the gradient changes more accurate and, at the same time, the chart more readable.

  2. Normalizing in Heightgraph would be a big ask, for that component is generic. This use case - showing the normalized gradients on the chart, but displaying the precise gradient on mouse over - might be too specific for the component itself. If that's the case, then this logic should go into a wrapper, in which case I don't know if the wrapper could get low-level access to the Heightgraph functionality that it would need to enable this behaviour.

  3. Having a toggle to enable/disable normalization is an interesting option; are you interested in exploring this option more (e.g. where to show this toggle, what label to use on it, etc)?

  4. Could you elaborate on "Maybe having two static preconfigured values for chartWidthInPixels (one regular and one for when the toolbar at the top is collapsed)" ? I am not following what the 2nd one is for.

  5. If using a toggle, and if the user has enabled the normalization, I could disable it on chart resize - this behaviour might be confusing to the user, but at least the chart will be accurate.

Screenshot 2023-12-01 at 14-55-51 Elevation Profile

Screenshot 2023-12-10 at 11-31-16 Elevation Profile

rkflx commented 6 months ago

Whatever you do, please don't make the popup's slope values change based on route length and window width (not even with a toggle).

Until the popup values and the heightgraph data have been decoupled, it does not make much sense to discuss the other points wrt. improving the colorization in detail.

to make the chart more readable

As mentioned before, indicating extreme slopes instead of hiding them can be seen as a feature. How to consider those for planning comes up again and again when looking at BRouter issues. Pretending they don't exist on long routes by averaging them out via normalization is not gonna fly. There is a difference between a short punchy climb with a long flat, and a steady gentle slope of equal distance (both resulting in the same average gradient), which is useful to know when planning routes.

Still, I can also understand there might be users wanting to average longer sections, e.g. a long roadbike ascent with relatively steady slopes (compared to an offroad gravel ride with much more varied slopes). It might be worth experimenting with a "smoothing/density" slider instead of a normalization checkbox, to account for both needs.

Another way could be to use a completely different approach: Do not color undulating terrain at all, and instead detect longer climbs and assign a color for the climb as a whole (in the sense of "Col du XY, Cat. 4 climb with an average gradient of Z%"). Extreme slopes could be indicated with a special icon or marker.

Lastly, major manufacturers of cycling head units (Garmin, Wahoo, Hammerhead) recently added climbing detection and visualization features to their devices and online platforms. It might be worth exploring how they are handling this, and also look at apps like OsmAnd.

If showing accurate gradients is preferred, I am more than happy to disable the normalization

I'd prefer that in case none of the other ideas get implemented, but in the end that's for @nrenner to decide.


  1. (minNormalizationDistanceInPixels)

Tuning this might help, but I don't think this will solve the conceptual issues.

  1. Normalizing in Heightgraph would be a big ask

Instead of changing the heightgraph to normalize internally, you could also keep your existing logic and rather modify (hook into?) the route popup: It could sync position between route and graph as before, but calculate slope based on raw height data (to the extent as found in BRouter's response) independently and on the fly. Not sure if/how this was done in the old elevation graph!?

This would then also allow to show the exact slope in the route popup, and the categorized slope (matching the color) in the heightgraph popup, i.e. combine the best of both worlds.

  1. where to show this toggle, what label to use on it, etc

Such a toggle (or slider) should be shown next to or overlayed on top of the heightgraph. I believe the heightgraph allows to show a legend (which we should enable, since multiple users asked for it, and rightly so), which could provide a starting point to explore this in detail.

  1. I am not following what the 2nd one is for.

On mobile devices / smartphones, the width of the graph typically is quite small (resulting in the top toolbar to collapse), while on desktop browsers it is larger. The basic idea was to hardcode an appropriate width for each one (reusing the existing Bootstrap width thresholds), to sidestep the issue of having to dynamically update the value on window resizes, yet still cater for both cases. No doubt there are better ways to solve this, and with a slider this would also be less of a problem.

  1. I could disable it on chart resize

I agree that's confusing, so I'd rather not do that.


TLDR: There are three main use cases:

  1. Show accurate slope values in the route popup (independent of route length).
  2. Indicate extreme slopes (independent of route length).
  3. Visualize average slopes for longer sections (limited by the window's width, i.e. the density depends on route length).

Stuffing everything into a single color visualization and data model does not really seem to work.

alexcojocaru commented 4 months ago

I think I found a better solution to this problem: instead of bucketing the gradients on the profile (i.e. gradients 1 through 3 are shown as group 1-3, gradients 4 through 6 as group 4-6, etc), the algorithm keeps track of each individual integer (e.g. gradient 0, gradient 1, gradient 2, etc) . This results in a much busier profile, but I've mitigated that by using a gradient palette to assign similar colors to gradients close to each other (i.e. orange for gradient 0, orange with a tiny bit of red for gradient 1, orange with slightly more red for 2, etc) - this makes the profile appear less busy (i.e. less busy than in the screenshot in my previous comment). The only downside of this approach that I can think of is the larger dataset that's generated for a given profile, which has to be rendered by Heightgraph.

Here is a screenshot of a profile generated by the new code: elevation_profile-screenshot

On a related note: since you asked to have the legend shown, I've added it right under the profile.

bagage commented 3 weeks ago

Thank you @alexcojocaru, and sorry for the delay. I'll merge this now :tada: