Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Leaflet.Heightgraph to show PathDetails #1569

Merged
merged 9 commits into from Mar 25, 2019

Conversation

@boldtrn
Copy link
Member

boldtrn commented Mar 8, 2019

This PR changes the elevation diagram library as proposed in #1177. If we want to merge this PR, I would like to first publish the library Leaflet.Heightgraph to NPM, this will make the PR cleaner (also see this issue).

One issue with Leaflet.Heightgraph is that it does not support a miles display yet, so the units are fixed to metric units. We could add this feature to Leaflet.Heightgraph though. I haven't studied the code of Leaflet.Heightgraph in detail yet, so I don't know how much work this would require.

The results are already pretty good, so I think this would be a nice addition :). Also the diagram itself has some nice little features included, for example you can toggle it's state from shown to hidden or highlight parts of the route that are above a certain elevation.

If you don't use any PathDetails, the resulting diagram will be a plain elevation diagram, similar to the old elevation diagram. If you add path details to the request (currently you need to add them to the url, for example add &details=average_speed&details=street_name), you can see the results of the path details in the diagram.

screenshot from 2019-03-08 15-07-20

@karussell karussell added this to the 0.12 milestone Mar 8, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 8, 2019

Really cool, thanks a lot!

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 8, 2019

Will try (and merge) on Monday latest.

@karussell karussell self-requested a review Mar 8, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 8, 2019

Just a quick question: the elevation data is always shown implicitly, right? Or could we use the height of the diagram for integer based path details like average_speed?

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 8, 2019

Or could we use the height of the diagram for integer based path details like average_speed?

This should be possible as well for number based path details like speed. We can also define the color mapping, so that higher speeds could be green and lower speeds red or anything like this. Should I add something like this?

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 8, 2019

It's possible :)

screenshot from 2019-03-08 19-15-09

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 8, 2019

This is really great! And yes please do this for at least average_speed :) (color is not that important - btw: why there are two values in purple near the yellow on the right side?)

Potentially this is also useful for max_speed, but maybe there are problems for the unlimited cases on a German Autobahn leading to infinity peaks ;)

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 8, 2019

Potentially this is also useful for max_speed, but maybe there are problems for the unlimited cases on a German Autobahn leading to infinity peaks ;)

We can catch these ;). I just pushed the code. The current solution will require manual handling for every path detail that we want to show like this instead of the elevation.

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 8, 2019

Cool - thanks!

Could we automatically enable it for all path details with numbers?

@karussell karussell added new feature and removed improvement labels Mar 10, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 10, 2019

Ok, after "npm update" this works for me!

And yes, those new features are really neat - wow! So this is an important thing to have.

There is some limitation when I use details=average_speed, then the elevation is no longer an option, I would have expected that I can see the elevation and average_speed values. It seems the values are globally changes which will be problematic when we want the height of the chart representing the values of e.g. details=max_speed and details=average_speed. Do you know what I mean?

When doing the average_speed details, then the unit should change in the chart or at least be removed - currently it is [m]. Also it still says "elevation" instead of "average_speed" for the "mouse-over"-legend. The "Type" should be removed if possible.

A tiny thing: without a details parameter the elevation is shown - here we should use the old green color like before :)

Then we need to test if it is possible to show details even if the graph has no elevation data imported.

It is already on graphhopper.com/maps :)

so the units are fixed to metric units.

This is not important to fix.

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 11, 2019

It seems the values are globally changes which will be problematic when we want the height of the chart representing the values of e.g. details=max_speed and details=average_speed. Do you know what I mean?

Thanks for finding this! I just double checked, you are right, this is rather ugly :). I was able to fix this part, but then the elevation plugin didn't work properly anymore. I reverted my latest change for now, it's not as easy as I hoped to use the height of the bars to indicate the average speed. I think this would need further changes in the plugin itself.

One issues was that with average_speed, the bars will be between 0-140. For elevation the bars might be between 300-500. The plugin did not handle this properly, so we'd need an update mechanism when switching between the different path details to also update the "height" of the graph.

Then we need to test if it is possible to show details even if the graph has no elevation data imported.

This should be possible, once we improve the plugin :). For only a single PathDetail this should be possible already.

It is already on graphhopper.com/maps :)

This is awesome :)

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 11, 2019

I would also revert change regarding the "speed as height" in the chart. In a separate PR we can make the necessary changes in this matter - WDYT?

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 11, 2019

I would also revert change regarding the "speed as height" in the chart. In a separate PR we can make the necessary changes in this matter - WDYT?

Yes :).

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 11, 2019

Can you check if my latest change regarding the color is fine?

position: "bottomright"
}
// for path details use random colors
if(jQuery.isEmptyObject(details))

This comment has been minimized.

Copy link
@karussell

karussell Mar 11, 2019

Member

Here jquery is required as details is {} if no details parameter was specified

This comment has been minimized.

Copy link
@boldtrn

boldtrn Mar 11, 2019

Author Member

We could add this part here and avoid using jQuery.

}
// for path details use random colors
if(jQuery.isEmptyObject(details))
options.mappings = { Elevation: {'elevation': {text: 'Elevation [m]', color: '#27ce49'}}};

This comment has been minimized.

Copy link
@karussell

karussell Mar 11, 2019

Member

This was a bit tricky: the first key Elevation must match the summary and the second elevation the attributeType

This comment has been minimized.

Copy link
@boldtrn

boldtrn Mar 11, 2019

Author Member

Yes that's indeed tricky :). We could think about color mappings for the other details as well.

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 11, 2019

This looks good thanks :). I slightly rectored the code, so we don't need jQuery. WDYT?

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 11, 2019

I slightly rectored the code, so we don't need jQuery. WDYT?

Sure - perfect - thanks!

@karussell karussell self-requested a review Mar 11, 2019
@karussell karussell marked this pull request as ready for review Mar 11, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 11, 2019

I had to make this PR ready for review ... is this correct that it is no longer a WIP?

@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 11, 2019

... is this correct that it is no longer a WIP?

We could merge it.

I wanted to first publish Leaflet.Heightgraph on NPM and use the dependency from NPM instead of hard copying the js file. But we can do this later as well. Also we could delete all old references from the old elevation library (like css).

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 11, 2019

Ah, ok. This would be good to be done before the merge, but of course no hurry :) (can also remove the 0.12 tag)

TODOs:

  • publish Leaflet.Heightgraph on NPM
  • make width of chart 25% smaller
@karussell karussell modified the milestones: 0.12, 0.13 Mar 20, 2019
@boldtrn

This comment has been minimized.

Copy link
Member Author

boldtrn commented Mar 25, 2019

I fixed the Todos and also the issue reported in #1573. The fix for #1573 is not a nice fix more a hotfix, but works for now. This should be improved upstream. I opened an issue here - GIScience/Leaflet.Heightgraph#39

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Mar 25, 2019

Thx!

@@ -33,7 +33,8 @@
"leaflet-loading": "0.1.24",
"moment": "2.22.1",
"uglifyify": "5.0.0",
"flatpickr": "4.4.6"
"flatpickr": "4.4.6",
"leaflet.heightgraph": "0.0.1"

This comment has been minimized.

Copy link
@karussell

karussell Mar 25, 2019

Member

Oh, interesting. So it is possible to release stuff on npm without any namespace prefix? How does one know that it is original content?

This comment has been minimized.

Copy link
@boldtrn

boldtrn Mar 26, 2019

Author Member

So it is possible to release stuff on npm without any namespace prefix?

Yes, in the beginning npm had no namespace prefix. There is now the option to register for a namespace/organisation. The dependency would then look like @npmcorp/package-name.

BTW: we could register an organisation for GH here :)

How does one know that it is original content?

Once you publish a package with a name, only you (or selected contributors) can upload another package with the same name. Usually you could first check the Github repo and check the package.json if the names match.

@karussell karussell merged commit b15536c into graphhopper:master Mar 25, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stevensnoeijen added a commit to stevensnoeijen/graphhopper that referenced this pull request Apr 2, 2019
* master: (32 commits)
  Fixes some issues with turn cost times. (graphhopper#1586)
  Android: VTM 0.11.0 OpenGL vector map library (graphhopper#1587)
  Cannot set a blocked edge to accessible again (graphhopper#1541)
  testing against JDK 12+13 instead 11+12
  web: updated tracker for maps
  Fixes compiler warning.
  Fixes wrong routing occurring when stall on demand is used with virtual via-nodes. (graphhopper#1581)
  Update README.md
  Update README.md
  Fix broken link (graphhopper#1578)
  i18n: updated zh_CN
  web ui: updated a few dependencies
  Use Leaflet.Heightgraph to show PathDetails (graphhopper#1569)
  updated android-maven-plugin
  fix latest stable links in readme
  Fix typo (graphhopper#1575)
  minor fix to parent version in android ap
  avoid traversalKey and use traversalId or edgeKey instead, depending on the use case graphhopper#1549
  Fixes endless recursion in AllCHEdgeIterator.
  braking change: rename to always use traversal key, fixes graphhopper#1549
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.