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

new edge smoothing: "ramer" #2634

Merged
merged 9 commits into from
Aug 15, 2022
Merged

new edge smoothing: "ramer" #2634

merged 9 commits into from
Aug 15, 2022

Conversation

karussell
Copy link
Member

@karussell karussell commented Jul 25, 2022

The big problem of the current smoothing method (#1220) is that the result is optimized towards reducing the ascend&descend, but it makes the elevation data nearly unusable for routing (as the slope increases often extremely). This was also partially discussed in #1953 (see #1953 (comment)).

So I thought about a smoothing that does not make the slope more extreme (slope can be positive or negative) and is still able to remove some small elevation changes. A simple idea is to use the Ramer-Douglas-Peucker algorithm and instead of removing the points that are below a maximum elevation change the smoothing algorithm sets them to the elevation resulting from the average slope. The result is quite good and the decrease of the ascend&descend still comes close to the old smoothing, sometimes even a bit smaller:

For this route

  • no smoothing ↗37m ↘43m:
    grafik

  • old smoothing ↗33m ↘36m:
    grafik

  • new smoothing (max 5m) ↗32m ↘34m:
    grafik

Or see this route which should be very smooth with a small ascend/descend still our raw CGIAR elevation data makes it hard for us due to the hills nearby:

  • no smoothing ↗86m ↘92m:
    grafik

  • old smoothing ↗55m ↘61m:
    grafik

  • new smoothing ↗46m ↘52m:
    grafik

See the config option:

graph.elevation.edge_smoothing: ramer
graph.elevation.edge_smoothing.ramer.max_elevation: 10
# old smoothing via:
# graph.elevation.edge_smoothing: window
# an exception is thrown if the the old parameter 'graph.elevation.smoothing' is used

One interesting property of this new method is that if you set the graph.elevation.edge_smoothing.ramer.max_elevation to a very big value it will set all pillar nodes to the elevation resulting from the average slope of the tower nodes leading to the most extreme form of elevation simplification we currently can do (without increasing maximum slope and without changing the elevation for tower nodes).

It would be nice to combine this with long edge sampling, but to do that we would have to swap the order of the calls. I'm unsure why the current order was picked - see discussion about this topic here.

@otbutz
Copy link
Contributor

otbutz commented Jul 25, 2022

Quick comparison with Google Maps:

First route ↗️ 14m ↘️ 27m:

grafik

Tried to replicate the second route as good as possible ↗️ 67m ↘️ 57m:

grafik

With Google Maps as reference, it looks like a clear improvement :)

@karussell
Copy link
Member Author

From experience the elevation data from Google Map is not the best reference and I would choose Komoot instead (although not for their still suboptimal bike routing ;))

@boldtrn
Copy link
Member

boldtrn commented Jul 25, 2022

Visually I think the new results look quite interesting.

I also think the property that it does not increase the slope is very interesting.

I am not sure if Douglas Peucker is removing too many details? If there is actually a very steep part on an edge DP might remove this steepness. If you think about bicycle routing, you really want to avoid very steep parts.

On the other hand, DP keeps the extremes. Often these extremes, if they only occur for a very short part of the road, are data glitches and not actual changes on the road.

@otbutz
Copy link
Contributor

otbutz commented Jul 25, 2022

Have you tried Visvalingam or a simple moving average?

@karussell
Copy link
Member Author

karussell commented Jul 25, 2022

No, but feel free to try it out :)

From Wikipedia:

Visvalingam
Disadvantages: The algorithm does not differentiate between sharp spikes and shallow features, meaning that it will clean up sharp spikes that may be important.

The biggest convincing argument towards the adapted RDP is for me that it won't increase the maximum slope while still keeping outstanding elevation changes, which are two nice properties to have plus even some smoothing :)
I don't think that we can do a lot better (please proof me wrong :D), except of course when we consider the entire graph and start changing tower nodes too like in a post processing. But this is a lot trickier to do well (as we would have to change the distance ... leading to changes to all average_speed EVs etc ... ).

I am not sure if Douglas Peucker is removing too many details?

You can configure it. Also there is a TODO NOW in the code that instead of a maximum elevation change it might make sense to make this relative to the distance (i.e. somehow define a maximum slope change). Not sure if this is a good idea though.

@boldtrn
Copy link
Member

boldtrn commented Jul 25, 2022

while still keeping outstanding elevation changes

I would argue, that this is a downside of RDP. Outstanding elevation changes have a high likelyhood of being data errors (e.g. short peaks or drops).

moving average?

Isn't the current implementation (window) a moving average?

@otbutz
Copy link
Contributor

otbutz commented Jul 25, 2022

somehow define a maximum slope change

That one might make sense. It's elevation data for routes after all. An unpassable incline should be filtered out.

@karussell
Copy link
Member Author

karussell commented Jul 25, 2022

Outstanding elevation changes have a high likelyhood of being data errors (e.g. short peaks or drops).

But short peaks or drops are smoothed away if they fall under the maximum value and so the remaining outstanding elevation changes are likely no data errors.

An unpassable incline should be filtered out.

Not that easy to define because this depends on the vehicle. What I meant was to convert the current "maximum elevation change limit" into a "maximum slope change limit". But now that I'm thinking about it, it could lead to more filtering of real elevation changes (low frequent changes) :) ... we are only interested to filter away the high frequent noise, but only if these changes are not too big, so IMO exactly what the adapted RDP does atm.

Also please note that the old smoothing can still be used. (Shall we rename it into moving_average or something?)

@karussell karussell added this to the 6.0 milestone Jul 27, 2022
@karussell karussell mentioned this pull request Aug 15, 2022
@karussell karussell changed the title new edge smoothing new edge smoothing: "ramer" Aug 15, 2022
@karussell karussell merged commit 52dd050 into master Aug 15, 2022
@karussell karussell deleted the ramer_smoothing branch August 15, 2022 19:26
karussell added a commit that referenced this pull request Aug 20, 2022
jp-lopez added a commit to StuartApp/graphhopper-matrix that referenced this pull request Dec 1, 2022
* fix travis release for tagged commits; use jdk 17 instead of 11 for github releases

* allow skipping the LM constraint check for A* (graphhopper#2544)

* LM constraint should be ignored for A*

* make it possible to skip checkLMConstraints call

* move checkLMConstraints call to LMSolver. Fix createSolver for mixed preparations

* avoid fundamental default change but throw proper exception when lm.disable parameter missing

* Add names for travis build jobs (to be shown under 'Jobs and Stages on GitHub)'

* Disable doclint explicitly

* travis: only keep release build, related to graphhopper#2552

* Clean up javadoc comments (graphhopper#2569)

* fix javadoc errors

* remove javadoc plugin

* make comment more readable

* include javadoc again, revert unreadable comments

* disable doclint for javadoc

* Add flex alternatives to measurement

* Do not remove SPT elements from the priority queue, speeds up flexible algorithms (graphhopper#2571)

* Split flag encoders into encoded values and tag parsers (graphhopper#2561)

* Remove final from VehicleEncodedValues

* Log available country rules (graphhopper#2579)

Co-authored-by: Thomas Butz <thomas.butz@optitool.de>

* Give access to vehicle tag parsers

* Update bestFwd/BwdEntry for flexible algorithms when deleting SPT entries, fix graphhopper#2581

* Travis: Fix warnings in build config validation

It said:

Build config validation
root: deprecated key sudo (The key `sudo` has no effect anymore.)
root: missing os, using the default linux
root: key matrix is an alias for jobs, using jobs

* Add VehicleEncodedValues#isHGV

* Use more recent node/npm versions to fix the build (graphhopper#2583)

* Update readme for 5.3

* update deployment guide (graphhopper#2576)

* update deployment guide

* Update deploy.md

* review comments

* mention max_visited_nodes

* Replace deprecated dropwizard parameter types (graphhopper#2574)

Co-authored-by: Thomas Butz <thomas.butz@optitool.de>

* Use epsilon=0.9 for LM map-matching

* Update changelog

* Use edge keys consistently, store keys in CH shortcuts (graphhopper#2567)

* Add comment about slow non-CH alternatives

* Remove outdated/wrong comments

* Add a mapmatching test

* Remove 'shared' encoded values with $ sign in name (graphhopper#2585)

* remove OSMAccessParser

* removed $ from encoded values

* no if clause necessary

* fix changelog

* downgrade todonow

* Matrix client improvements (graphhopper#2587)

* tmp

* fix tests

* update okhttp to 4.9.3

* fixes regarding code review

Co-authored-by: easbar <easbar.mail@posteo.net>

* European toll fallback rules (graphhopper#2450)

Co-authored-by: Thomas Butz <thomas.butz@optitool.de>

* use for loop in PathDetailsBuilderFactory as discussed in graphhopper#2585 (graphhopper#2589)

* Fix: Put back block_private and block_fords for racingbike, this got lost in graphhopper#2561

* Make motor vehicle and hgv properties configurable for roads encoder (graphhopper#2594)

* Disable edge+alt measurements, they take too long

* Use separate deleted flag for SPTEntry, prevent exception when calculating routes with non-feasible approximator (graphhopper#2600)

Also revert epsilon=0.9 'fix' in map matching now that we know what is going on

* LMApproximator: Align variable names with equation numbers

* Log map-matching took value in ms instead of seconds (graphhopper#2591)


Co-authored-by: Andi <easbar.mail@posteo.net>

* rename StringIndex to EdgeKVStorage in preparation for graphhopper#2597

* windows does not need a separate documentation due to WSL (graphhopper#2599)

* windows does not need a separate documentation due to WSL

* link to install steps directly

* EdgeKVStorage: store more than Strings (graphhopper#2597)

* copied code and tests from byteindex branch

* a bit more docu and tests

* behaviour change: throw exception if string is too long and so cut string before storing way_name

* renamed StringIndex -> EdgeKVStorage

* more consistent: reject null values for all cases

* include lost changes again

* ensure same version of storage is used

* changes from review

* fix variable names for key handling

* comments to javadoc

* better 'compression' in case of identical map with byte[] array

* throw exception earlier

* Fix bug in non-CH AlternativeRoute (graphhopper#2603)

* fix plateau-end check

* Revert "Disable edge+alt measurements, they take too long"

This reverts commit fb39266.

* increase count to 10 for Measurement

* AlternativeRoute: follow up refactoring, graphhopper#2603

* MiniGraphUI fixes to make it work again

* move quickstart info into more prominent installation of README (graphhopper#2605)

* Measurement: reduce routingCH_alt after it was accidentally raised in graphhopper#2603

* Remove GraphHopperStorage (graphhopper#2606)

* Rename: GraphHopper#getGraphHopperStorage -> GraphHopper#getBaseGraph
* Remove GraphHopperStorage

* reduce benchmark load e.g. exclude too slow LM16 case

* AlternativeRoute: closer to CH version; default is now a better compromise of speed vs. alternatives found (alternative rate is 1.8 for DE and 1.9 for bavaria)

* Move/Rename: EncodingManager.Access -> WayAccess

* AlternativeRoute: fix test

* Clean up EncodingManager Builder

* minor: Use Bike/FootNetwork.KEY

* Rename freshFlags -> refreshFlags

* BaseGraph: remove copying flags on write, see graphhopper#2593

* added elevation data to reduce internet-dependence while running test

* Add comment about GraphHopper#init

* Merged graphhopper#2593, i.e. revert b8d9b55

Co-authored-by: Andi <contactammmagamma@gmail.com>
Co-authored-by: Thomas Butz <thomas.butz@optitool.de>

* Add another elevation file used in tests

* Use access and speed EVs rather than flag encoder for GHUtility#setSpeed

* Fix broken imports

* More use access and speed EVs instead of encoder for GHUtility#setSpeed

* Replace flag encoder with turn cost encoded value in turn cost provider

* Remove FlagEncoder from ShortestWeighting and CustomWeighting

* Find strongly-connected components of station (stop) graph (graphhopper#2608)

* Load EncodingManager from properties instead of config (graphhopper#2607)

* Helper.cutString should be only for KVStorage and reduce length to 250 to fix graphhopper#2609

* CHPreparationGraph bug fix: unsigned shift required (and increase limit for getKeyWithFlags graphhopper#2567)

* make error message more clear

* CHPreparationGraph: revert to previously correct signed shift

* Increase maximum edge-based CH key for preparation to 2^30 again (graphhopper#2612)

* Add test for large edge ID, graphhopper#2612

* custom_model: introduce value expression (graphhopper#2568)

* value expression: findMax needs to be evaluated

* less strict if * or + operator

* refactored to findMinMax

* made core tests pass

* fix some more tests

* fix more tests

* add encoded values to initialization plus tests

* check minimum for custom_model of query

* improve on check of minimum and negative factors

* less overhead for constant values

* use Double.parse instead of ExpressionEvaluator if number to significantly improve speed

* minor cosmetics

* clarify LATER and removed TODO NOW

* adapted docs

* minor optimization and cleanup

* fix FindMinMax.checkLMConstraints call

* do not allow / as operation

* reduce power and complexity of right hand side a bit more and allow only a single EncodedValue for now

* better readable via extra MinMax class instead of double[]

* exclude changes from master

* a bit more comments

* Create weighting after graph in two tests

* Update custom model editor: operator values are strings now

* Update custom model editor: validation + auto-complete for rhs expressions (graphhopper#2615)

* Allow graph.encoded_values for which there is no tag parser

* Use actual speed maximum rather than flagEncoder#getMaxSpeed() for weighting.getMinWeight() (graphhopper#2614)

* Rename getMin/MaxInt -> getMin/MaxStorableInt, getMaxSetValueOrMax -> getMaxOrMaxStorable (graphhopper#2616)

* Remove FlagEncoder#getMaxSpeed() (graphhopper#2619)

* Remove car/bike_access evs from DefaultEncodedValueFactory, was added only for graphhopper#2523

* CustomModelParser: no need for max speed

* EdgeKVStorage: store name and ref tags separately (graphhopper#2598)

* copied code and tests from byteindex branch

* a bit more docu and tests

* behaviour change: throw exception if string is too long and so cut string before storing way_name

* renamed StringIndex -> EdgeKVStorage

* store name and ref tags separately; create separate path detail for ref; use ref as fallback in instruction text

* added text to changelog

* minor comment changes

* review: fix comments

* review comments and add getValue method to avoid creating HashMaps

* try if speed up was really from the change getKeyValues->edge.getValue

* Revert "try if speed up was really from the change getKeyValues->edge.getValue"

This reverts commit 58a34d6.

Co-authored-by: easbar <easbar.mail@posteo.net>

* update i18n

* Remove FlagEncoder (graphhopper#2611)

* EdgeKVStorage: remove smallCache as too much complexity. we accept disk usage increases by 12% and more uniq keys later possible

* Move outdoor vehicles list

* Relax speed EV requirement for EncodingManager#getVehicles

* Check vehicle instead of access/speed in checkProfilesConsistency

* reduce distance when average_speed path details can be missing, fixes graphhopper#2620

* Valid if PathDetail is null (graphhopper#2618)

* Valid if object is null

* Adding my name in contributors

* Adjusting indentation

* Update CONTRIBUTORS.md

Co-authored-by: Rafael Telles <rafael@bzion.com.br>
Co-authored-by: Peter <graphhopper@gmx.de>

* Support for forward & backward key value pairs (graphhopper#2622)

* fix test

* minor update to docs for map matching

* Update README.md

* add more info to exception

* profiles.md: updated rhs to strings

* minor test change to avoid downloading elevation tile

* CustomModelParser: minor cleanup

* EncodedValue: bug fix for negateReverseDirection

* ConditionalExpressionVisitor: allow '-' as unary operation

* fixed link to CGIAR attribution

* make global time&distance values consistent with path details and instructions (graphhopper#2626)

* make global time&distance values consistent with time&distance path details and also the sum of time&distance of the instructions

* minor simplification

* mention change in changelog

* minor comment fix for graphhopper#2626

* Update github actions v2->v3

* Include destination in instructions (graphhopper#2624)

* support for destination and add test for non repetitive instructions when destination is included

* include destination:ref

* add missing osm file

* try getKeyValues instead getValue

* try speed of getKeyValues (fetching REVERSE_STATE necessary as getKeyValues is align with 'internal' storage direction)

* use simpler edge.getValue call again

* Make profile resolving more flexible for web API (graphhopper#2629)

* ProfileResolver only resolves name of the profile (graphhopper#2629)

* car: do not pass jersey_barrier

https://www.openstreetmap.org/node/8857224952

* move cut string method to EdgeKVStorage

* EdgeKVStorage: no need for cacheSize parameter anymore

* Examples: remove incorrect comment

* fix PathDetails e.g. -1 as default for DecimalDetails doesn't work as a value of the EV could be negative too

* renamed DouglasPeucker to RamerDouglasPeucker

* client-hc: add examples to get translated ("raw") turn instructions

* HeightTile: minor correction, DecimalEncodedValueImpl: minor clarification

* EdgeKVStorage: allow 32 bits (graphhopper#2632)

* allow 32 bits instead of just 31 for EdgeKVStorage

* remove incorrect exception

* minor change to wayGeo limit check

* Maps: Hide gpx export button when custom model box is open, fix graphhopper#2635

* Add hint about custom_model_file vs. custom_model to exception error message (graphhopper#2638)

* Add comma in routing logs

* Move code that creates EncodingManager from properties to EncodingManager

* Return boolean result from BaseGraph#loadExisting

* Move code that puts EncodingManager into properties to EncodingManager

* add hgt provider

* AbstractTiffElevationProvider: use correct long cast

* pt: hint on how to use multiple files

* Add BaseGraph#debugPrint

* new edge smoothing: "ramer" (graphhopper#2634)

* initial version of new edge smoothing

* improve comments

* let's switch the order: first do sampling, then smoothing

* use moving_average as name and a few more comments

* use same max_elevation like in example config

* fix moving_average

* again moving_average rename

* New average_slope EV (graphhopper#2645)

* initial version of new edge smoothing

* improve comments

* let's switch the order: first do sampling, then smoothing

* encoded values for elevation-aware routing: average_slope and max_slope

* for tunnels and bridges ignore elevation changes for pillar nodes

* SlopeSetter -> SlopeCalculator

* fix link for GTFS demo file for graphhopper#2639

* removed defaultIsInfinity and fix bug in DecimalEncodedValueImpl (graphhopper#2646)

* removed defaultIsInfinity and fix bug in DecimalEncodedValueImpl

* changelog: fix issue number

* for useMaximumAsInfinity: also throw an exception if value is too big, but not for infinity

* fix merge of master

* Revert "for useMaximumAsInfinity: also throw an exception if value is too big, but not for infinity"

This reverts commit e3d8826.

* print warning if max_width, max_weight etc values are too large

* fix link for GTFS demo file for graphhopper#2639

* Revert "fix link for GTFS demo file for graphhopper#2639"

This reverts commit d9bb4b9.

* NOTICE: updated dependencies and copyright

* make ramer edge smoothing more robust, graphhopper#2634

* TurnCostParser: no need for method createTurnCostEncodedValues

* improvements for bike (graphhopper#2631)

* lower speed for bad smoothness and steps; avoid hazmat; faster if paving_stones

* clean up of smoothness handling

* revert to 4kmh pushing section speed and tweak speed for certain surfaces

* mtb+racing: do not overwrite highway and surface speed if identical in subclass

* consider multiple from members for no_entry (graphhopper#2648)

* allow configuring turn costs and TransportationMode for RoadsTagParser, related to graphhopper#2460

* add hgv enum to make truck access configurable

* add test for hgv encoded value

* for now do not log as too many warnings, graphhopper#2646

* SlopeCalculator: skip calculation if PointList is empty or 2D only

* Do not get elevations for nodes we don't need while parsing OSM

* Custom weighting calcEdgeMillis truncate instead of round

* just for consistency and easier migration with/from FastestWeighting

* CustomWeighting: handle special case for barrier edges with distance==0 and average_speed>0

* Maps: Add slope detail to elevation diagram (graphhopper#2654)

* Custom and fastest weighting calcEdgeMillis round instead of truncate

* otherwise they are not always the same, because one multiplies by 3600 and the other by 3.6*1000 ...

* Fix pt tests

* Disable flaky test

* custom and fastest calcEdgeMillis again: make them actually the same

* Validate that osm ids are not negative (graphhopper#2652)

* Add comment about negative osm ids, code formatting

* OSMParsers: don't accept null TagParser

* replace car4wd with roads and a custom_model (graphhopper#2651)

* Revert "OSMParsers: don't accept null TagParser"

This reverts commit 8f5c7b2.

* Add urban density encoded value to identify built-up areas (graphhopper#2637)

* Rename Development -> UrbanDensity (graphhopper#2637)

* Support ";" access delimiter for car access. (graphhopper#2655)

* Support ";" access delimiter for car access.
One example is motor_vehicle = agricultural;forestry

* Add support for ";" access delimiter for MotorcycleTagParser and road_access EV.

* Move logic for access delimiter handling for ";" from RoadAccess into OSMRoadAccessParser

* Fix for OSMRoadAccessParserTest

* map match without graphhopper class (graphhopper#2657)

* Solve Viterbi problem with Dijkstra to calculate fewer routes

* Solve Viterbi problem with Dijkstra to calculate fewer routes

* use 6.x/6.0 in readme

* announcement

* fix critical bug in client-hc for Matrix API

* SBI Algo

Co-authored-by: Peter <graphhopper@gmx.de>
Co-authored-by: easbar <easbar.mail@posteo.net>
Co-authored-by: otbutz <tbutz@optitool.de>
Co-authored-by: Thomas Butz <thomas.butz@optitool.de>
Co-authored-by: Michael Zilske <michael.zilske@tu-berlin.de>
Co-authored-by: Robin <boldtrn@users.noreply.github.com>
Co-authored-by: Andi <contactammmagamma@gmail.com>
Co-authored-by: Rafael Sanches Telles <sanchesetelles@gmail.com>
Co-authored-by: Rafael Telles <rafael@bzion.com.br>
Co-authored-by: Alexey Abel <dev@abelonline.de>
Co-authored-by: Lukas Weber <32765578+lukasalexanderweber@users.noreply.github.com>
Co-authored-by: ratrun <ratrun@gmx.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants