Use Heightgraph in lieu of Elevation to show the elevation profile

[alexcojocaru]

Things to note:

• d3 3.x was used only by Elevation, I removed both from the project dependencies;
  that is because Heightgraph only works with d3 5.x, whereas this project
  used d3 3.x;
• Since Heightgraph is written with ES6 (as is d3 5.x I believe),
  I had to use its transpiled bundle (which includes d3 5.x);
• There were a few definitions in the transpiled Heightgraph which conflicted
  with the rest of brouter-web; I had to comment them out via a patch
  applied in the postinstall phase.

This PR is related to #343;
since I used the transpiled Heightgraph, the upgrade to d3 5.x is not necessary.

[nrenner]

Thanks for working on this!

Some issues:

1. Leaflet.Heightgraph and the plugin wrapper Heightgraph.js don't seem to handle missing elevation data:
   • TypeError: Cannot read property 'text' of undefined at NewClass._prepareData (L.Control. Heightgraph.js:4994)
     • http://localhost:3000/index.html#map=12/53.5235/8.5705/osm-mapnik-german_style&lonlats=8.545475,53.520411;8.565903,53.529595;8.584442,53.538267&profile=fastbike
   • TypeError: Cannot read property 'geometry' of undefined at Heightgraph.js:267
     • http://localhost:3000/index.html#map=17/44.44707/15.18322/standard&lonlats=15.183293,44.447082;15.181603,44.448656&profile=fastbike
     • http://localhost:3000/index.html#map=12/59.9929/5.4975/Stamen.Terrain&lonlats=5.51033,60.028328;5.506211,59.9679
   • see issues abrensch/brouter#137, #203 and Leaflet.Elevation#63 (comment)
2. collapsed state is not remembered for the next session
   • this is due to missing initCollapse method and it's call in index.js
3. missing bottom tiles after resizing window and closing height graph
   • map._onResize(); call after collapse missing, also in initCollapse method
4. applying a patch to a distribution bundle is a bit fragile and will probably fail (need maintenance) with some future update
   • wrapping the bundle inside a closure using an IIFE (immediately invoked function expression), so d3 doesn't pollute the global scope, seems to work as well:

     (function() {
       ...
     }());
     

     Automatically adding those lines at the top and bottom of the bundle file would be less fragile, so I would prefer that instead of the patch, if there is a way

[alexcojocaru]

Thanks for looking at this, @nrenner . I will have a look at issues 1 through 3 and fix them.

About 4 (patching the Heightgraph bundle):

• I set a fixed Heightgraph version in package.json, which shouldn't break the patch unless the Heightgraph dependency is explicitly updated, but I agree that this patch is fragile and it would probably have to be redone when switching to newer versions of the dependency, so it's just a temporary solution
• there's this discussion which I started on the Heightgraph project, where I asked if it's possible to wrap the whole code in the bundle; given there hasn't been much feedback, I expect the worst (i.e. not going to happen any time soon, if any)
• it looks like I can modify the patch to apply the IIFE; that would make it more resilient to further changes to the Heightgraph bundle, until the IIFE is integrated in the bundle, at which point the patch could be removed; does this sound like an acceptable solution for the time being?

[nrenner]

it looks like I can modify the patch to apply the IIFE [...] does this sound like an acceptable solution for the time being?

I guess that would already be an improvement and maybe easier to check if the patch is still ok.

I intend to open separate/new Heightgraph issues for 1. and 4.

[alexcojocaru]

The geojson for the heightgraph profile is built in the Heightgraph plugin. I could easily filter out all latLng points which don't have an ele property, although that might result in a poorly approximated distance shown on the heightgraph profile. Another option is to set the elevation on such points to the elevation of the previous point which has such property.

[nrenner]

What I have in mind is the same approach I implemented for Leaflet.Elevation in PR MrMufflon/Leaflet.Elevation#84 (we're using my fork).

Leaflet.Heightgraph should support undefined/null values for all or part of the elevation values and show them as gap in the graph using a d3 defined accessor function.

[nrenner]

I opened an issue for 4.: GIScience/Leaflet.Heightgraph#107.

The minified bundle has an IIFE, so we might just use that for now, but for some reason it didn't get included when I had a quick try changing the main reference to it.

[alexcojocaru]

The minified bundle has an IIFE, so we might just use that for now, but for some reason it didn't get included when I had a quick try changing the main reference to it.

It's not obvious why, all I can say is that the Heightgraph attribute is not available in that scope.

I have been working on handling points with undefined altitude coordinate without errorring out, as there is some manipulation of such points in the plugin. Once that is done, I will look at issues #2 and #3 you described earlier.

[alexcojocaru]

A short update on the progress:

1. Missing elevation data: the code in Heightgraph.js can handle such points without breaking; I have tested many edge cases and it works as expected. Leaflet.Heightgraph has to be fixed to deal with such point on its end though.
2. My bad on ignoring some of the code in Elevation.js (resize, collapse/expand, etc). I worked on an old version of your project and did not notice these improvement, nor they conflicted with my changes (since Elevation.js is removed in my branch).
3. same as above
4. The patch was updated to use an IIFE. Still couldn't get the minified Heightgraph to work.

I am currently working on fixing items 2 and 3 above.

[alexcojocaru]

Just finished fixing items 2 and 3.

To summarize: the only outstanding issue currently is in Leaflet.Heightgraph, when the track contains points without elevation data. Given the recent activity on that project (or the lack thereof), I am not too optimistic about it being fixed soon.

[alexcojocaru]

Like I said in the previous comment, I have the feeling that the bug in Leaflet.Heightgraph around handling points without elevation would take quite a while to get fixed. Because of this, I handled such points in the Heightgraph plugin and inferred their elevation from the closest points with elevation in the track: d83fddd

Speaking of this bug in Leaflet.Heightgraph, since you fixed the similar one in Leaflet.Elevation and you have more context, could you open an issue for it please? Thanks.

[nrenner]

Thanks for the updates.

I'm not sure about interpolating missing elevation values. While interpolation might work for some cases, it will be misleading or just wrong for others. I personally consider showing the data as it is - with gaps in the graph for missing values - as the proper solution, which is why I implemented this for Leaflet.Elevation. Switching to Heightgraph without this feature feels like a step back in this regard.

I have opened an issue for missing elevation data: Heightgraph#108. And I will look into making a PR for this.

[alexcojocaru]

👍 to your statement. Undoing it is easy, I just have to revert the last commit.
Looking forward to having the Heightgraph issue fixed...

[nrenner]

Thanks for your contribution!

[alexcojocaru]

Thanks for integrating this change, I look forward to using it.

My impression was that the major blocker to using Heightgraph was its inability to handle tracks with points with undefined elevation. I see that you fixed it, and the IIFE issue as well, but the PRs were not merged in.