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

Add Average Speed as PathDetails to Response #1091

Merged
merged 46 commits into from
Aug 16, 2017
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
d38aa5c
First proposal to add path details
boldtrn Jun 17, 2017
887cbf4
Use number of points to determin the interval for pathdetails
boldtrn Jun 27, 2017
f10da8a
Improved JS debugging code
boldtrn Jun 27, 2017
6082c39
Changed the way PathDetails are stored to a list instead of a Map
boldtrn Jul 1, 2017
b16205f
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Jul 1, 2017
3d87388
Store pathdetails with number of points, not intervals
boldtrn Jul 1, 2017
dfa6e34
Store pathdetails in a map and let jackson handle the serialization
boldtrn Jul 4, 2017
e281e9b
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Jul 4, 2017
75660e9
Updated according to review comments
boldtrn Jul 4, 2017
7b95358
Fixed Via Point Issue
boldtrn Jul 4, 2017
66754f5
Added PathSimplification for Instructions and PathDetails
boldtrn Jul 11, 2017
b72e5c1
Changed instructions marker to red so it's easier to debug
boldtrn Jul 11, 2017
e722752
Removed temporary disabled simplification
boldtrn Jul 11, 2017
3153e10
Changed the way we call DP simplify
boldtrn Jul 11, 2017
c7768bf
Fixed tests
boldtrn Jul 14, 2017
fede463
Put PathDetails into PathDetailsCalculator
boldtrn Jul 31, 2017
e969263
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Aug 1, 2017
bf0f96e
Added some this.
boldtrn Aug 1, 2017
fe28658
Improved simplification and fixed tests
boldtrn Aug 1, 2017
f926402
Moved the PathDetails request parameter into the GHRequest
boldtrn Aug 2, 2017
02e89ad
Updated Parameters
boldtrn Aug 3, 2017
43a4c2c
Created the PathDetailsBuilder
boldtrn Aug 4, 2017
92d351a
Renamed to buildPathDetails
boldtrn Aug 4, 2017
746aff1
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Aug 4, 2017
85d1e55
Improved JavaDoc and some more small things.
boldtrn Aug 4, 2017
2a384fd
incomplete PathDetails to List<PathDetail> refactoring, TODO: JSON se…
karussell Aug 4, 2017
8d08821
Enabled serialization
boldtrn Aug 5, 2017
f135c46
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Aug 5, 2017
7f5b936
using a custom serializer for PathDetail
karussell Aug 8, 2017
1c211b4
Updated instructions logic
boldtrn Aug 9, 2017
b2a09d8
Added GraphHopperWeb integration and test
boldtrn Aug 9, 2017
9c4ad54
Removed redundant code
boldtrn Aug 9, 2017
5e20f04
Merge remote-tracking branch 'remotes/gh/master' into issue_439
boldtrn Aug 9, 2017
28b71d5
Improved GraphHopperServlet
boldtrn Aug 9, 2017
73c6b73
Updated javadoc
boldtrn Aug 9, 2017
30eec76
minor changes
karussell Aug 10, 2017
87a9a51
refactoring to use first+last in PathDetail class, WIP as simplificat…
karussell Aug 11, 2017
618967f
Fixed the simplification
boldtrn Aug 14, 2017
009c887
Minor test improvement
boldtrn Aug 14, 2017
db89861
show simplification problem
karussell Aug 14, 2017
bbbd403
show glitch in Measurement
karussell Aug 14, 2017
f398ab2
Fixed tests and glitch
boldtrn Aug 15, 2017
871c675
added integration test
karussell Aug 15, 2017
0894a03
Changed List details
boldtrn Aug 16, 2017
e96cf3e
revert commenting out
karussell Aug 16, 2017
89ea614
Merge branch 'master' into issue_439
karussell Aug 16, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions core/src/main/java/com/graphhopper/GHRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public class GHRequest {
// Headings are north based azimuth (clockwise) in (0, 360) or NaN for equal preference
private final List<Double> favoredHeadings;
private List<String> pointHints = new ArrayList<>();
private List<String> pathDetails = new ArrayList<>();
private String algo = "";
private boolean possibleToAdd = false;
private Locale locale = Locale.US;
Expand Down Expand Up @@ -253,6 +254,15 @@ public boolean hasPointHints() {
return pointHints.size() == points.size();
}

public GHRequest setPathDetails(List<String> pathDetails) {
this.pathDetails = pathDetails;
return this;
}

public List<String> getPathDetails() {
return this.pathDetails;
}

@Override
public String toString() {
String res = "";
Expand All @@ -266,6 +276,12 @@ public String toString() {
if (!algo.isEmpty())
res += " (" + algo + ")";

if (!pathDetails.isEmpty())
res += " (PathDetails: " + pathDetails + ")";

if (!hints.isEmpty())
res += " (Hints:" + hints + ")";

return res;
}
}
4 changes: 4 additions & 0 deletions core/src/main/java/com/graphhopper/GraphHopper.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.graphhopper.util.Parameters.CH;
import com.graphhopper.util.Parameters.Landmark;
import com.graphhopper.util.Parameters.Routing;
import com.graphhopper.util.details.PathDetailsBuilderFactory;
import com.graphhopper.util.exceptions.PointDistanceExceededException;
import com.graphhopper.util.exceptions.PointOutOfBoundsException;
import com.graphhopper.util.shapes.BBox;
Expand Down Expand Up @@ -1050,11 +1051,14 @@ else if (ALT_ROUTE.equalsIgnoreCase(algoStr))
boolean tmpEnableInstructions = hints.getBool(Routing.INSTRUCTIONS, enableInstructions);
boolean tmpCalcPoints = hints.getBool(Routing.CALC_POINTS, calcPoints);
double wayPointMaxDistance = hints.getDouble(Routing.WAY_POINT_MAX_DISTANCE, 1d);
PathDetailsBuilderFactory calculatorFactory = new PathDetailsBuilderFactory(request.getPathDetails(), encoder);

DouglasPeucker peucker = new DouglasPeucker().setMaxDistance(wayPointMaxDistance);
PathMerger pathMerger = new PathMerger().
setCalcPoints(tmpCalcPoints).
setDouglasPeucker(peucker).
setEnableInstructions(tmpEnableInstructions).
setPathDetailsBuilderFactory(calculatorFactory).
setSimplifyResponse(simplifyResponse && wayPointMaxDistance > 0);

if (routingTemplate.isReady(pathMerger, tr))
Expand Down
31 changes: 28 additions & 3 deletions core/src/main/java/com/graphhopper/PathWrapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
package com.graphhopper;

import com.graphhopper.util.InstructionList;
import com.graphhopper.util.PathMerger;
import com.graphhopper.util.PointList;
import com.graphhopper.util.details.PathDetail;
import com.graphhopper.util.shapes.BBox;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.*;

/**
* This class holds the data like points and instructions of a Path.
Expand All @@ -46,6 +46,7 @@ public class PathWrapper {
private PointList pointList = PointList.EMPTY;
private int numChanges;
private final List<Trip.Leg> legs = new ArrayList<>();
private Map<String, List<PathDetail>> pathDetails = new HashMap<>();
private BigDecimal fare;

/**
Expand Down Expand Up @@ -242,6 +243,30 @@ public void setInstructions(InstructionList instructions) {
this.instructions = instructions;
}

/**
* Adds the given PathDetails to the existing ones. If there are already PathDetails set, the number
* details has to be equal to <code>details</code>.
*
* @param details The PathDetails to add
*/
public void addPathDetails(Map<String, List<PathDetail>> details) {
if (!this.pathDetails.isEmpty() && !details.isEmpty() && this.pathDetails.size() != details.size()) {
throw new IllegalStateException("Details have to be the same size");
}
for (Map.Entry<String, List<PathDetail>> detailEntry : details.entrySet()) {
if (this.pathDetails.containsKey(detailEntry.getKey())) {
List<PathDetail> pd = this.pathDetails.get(detailEntry.getKey());
PathMerger.merge(pd, detailEntry.getValue());
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we should ensure that this happens only for i==0 (?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be the case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups no, forget about it :) but I think there will be either all merge or all put operations?

BTW: here is also no need to call pathDetails.put for the merge case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is also no need to call pathDetails.put for the merge case

No, if the element does not exist, we have to put it. If it exists, then we have to merge it? I removed the unnecessary put for the merge case.

this.pathDetails.put(detailEntry.getKey(), detailEntry.getValue());
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below: what is a 'merge'? And why not just adding PathDetails like the method says it?

Also instead of EMPTY_LIST and if+else I would prefer without the if and always do the same.


public Map<String, List<PathDetail>> getPathDetails() {
return this.pathDetails;
}

private void check(String method) {
if (hasErrors()) {
throw new RuntimeException("You cannot call " + method + " if response contains errors. Check this with ghResponse.hasErrors(). "
Expand Down
39 changes: 32 additions & 7 deletions core/src/main/java/com/graphhopper/routing/Path.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,20 @@
import com.carrotsearch.hppc.IntArrayList;
import com.carrotsearch.hppc.IntIndexedContainer;
import com.graphhopper.coll.GHIntArrayList;
import com.graphhopper.debatty.java.stringsimilarity.JaroWinkler;
import com.graphhopper.routing.util.DataFlagEncoder;
import com.graphhopper.routing.util.DefaultEdgeFilter;
import com.graphhopper.routing.util.FlagEncoder;
import com.graphhopper.routing.weighting.Weighting;
import com.graphhopper.storage.Graph;
import com.graphhopper.storage.NodeAccess;
import com.graphhopper.storage.SPTEntry;
import com.graphhopper.util.*;
import com.graphhopper.util.shapes.GHPoint;
import com.graphhopper.util.details.PathDetail;
import com.graphhopper.util.details.PathDetailsBuilder;
import com.graphhopper.util.details.PathDetailsBuilderFactory;
import com.graphhopper.util.details.PathDetailsFromEdges;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.*;

/**
* Stores the nodes for the found path of an algorithm. It additionally needs the edgeIds to make
Expand Down Expand Up @@ -372,6 +370,32 @@ public InstructionList calcInstructions(final Translation tr) {
return ways;
}

/**
* Calculates the PathDetails for this Path. This method will return fast, if there are no calculators.
*
* @param pathBuilderFactory Generates the relevant PathBuilders, accepts null inputs
* @return List of PathDetails for this Path
*/
public Map<String, List<PathDetail>> calcDetails(final PathDetailsBuilderFactory pathBuilderFactory, int previousIndex) {
if (!isFound() || pathBuilderFactory == null)
return Collections.EMPTY_MAP;
List<PathDetailsBuilder> pathBuilders = pathBuilderFactory.createPathDetailsBuilders();
if (pathBuilders.isEmpty())
return Collections.EMPTY_MAP;

forEveryEdge(new PathDetailsFromEdges(pathBuilders, previousIndex));

Map<String, List<PathDetail>> pathDetails = new HashMap<>(pathBuilders.size());
for (PathDetailsBuilder builder : pathBuilders) {
Map.Entry<String, List<PathDetail>> entry = builder.build();
List<PathDetail> existing = pathDetails.put(entry.getKey(), entry.getValue());
if (existing != null)
throw new IllegalStateException("Some PathDetailsBuilders use duplicate key: " + entry.getKey());
}

return pathDetails;
}

@Override
public String toString() {
return "distance:" + getDistance() + ", edges:" + edgeIds.size();
Expand All @@ -393,6 +417,7 @@ public String toDetailsString() {
*/
public interface EdgeVisitor {
void next(EdgeIteratorState edge, int index, int prevEdgeId);

void finish();
}
}
36 changes: 26 additions & 10 deletions core/src/main/java/com/graphhopper/util/DouglasPeucker.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,28 +54,44 @@ public DouglasPeucker setMaxDistance(double dist) {
}

/**
* This method removes points which are close to the line (defined by maxDist).
* Simplifies the <code>points</code>, from index 0 to size-1.
* <p>
* It is a wrapper method for {@link DouglasPeucker#simplify(PointList, int, int)}.
*
* @return removed nodes
* @return The number removed points
*/
public int simplify(PointList points) {
return simplify(points, 0, points.size() - 1);
}

/**
* Simplifies a part of the <code>points</code>. The <code>fromIndex</code> and <code>lastIndex</code>
* are guaranteed to be kept.
*
* @param points The PointList to simplify
* @param fromIndex Start index to simplify, should be >= <code>lastIndex</code>
* @param lastIndex Simplify up to this index
* @return The number of removed points
*/
public int simplify(PointList points, int fromIndex, int lastIndex) {
int removed = 0;
int size = points.getSize();
int size = lastIndex - fromIndex;
if (approx) {
int delta = 500;
int segments = size / delta + 1;
int start = 0;
int start = fromIndex;
for (int i = 0; i < segments; i++) {
// start of next is end of last segment, except for the last
removed += simplify(points, start, Math.min(size - 1, start + delta));
removed += subSimplify(points, start, Math.min(lastIndex, start + delta));
start += delta;
}
} else {
removed = simplify(points, 0, size - 1);
removed = subSimplify(points, fromIndex, lastIndex);
}

compressNew(points, removed);
if (removed > 0)
compressNew(points, removed);

return removed;
}

Expand Down Expand Up @@ -111,7 +127,7 @@ void compressNew(PointList points, int removed) {
}

// keep the points of fromIndex and lastIndex
int simplify(PointList points, int fromIndex, int lastIndex) {
int subSimplify(PointList points, int fromIndex, int lastIndex) {
if (lastIndex - fromIndex < 2) {
return 0;
}
Expand Down Expand Up @@ -145,8 +161,8 @@ int simplify(PointList points, int fromIndex, int lastIndex) {
counter++;
}
} else {
counter = simplify(points, fromIndex, indexWithMaxDist);
counter += simplify(points, indexWithMaxDist, lastIndex);
counter = subSimplify(points, fromIndex, indexWithMaxDist);
counter += subSimplify(points, indexWithMaxDist, lastIndex);
}
return counter;
}
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/com/graphhopper/util/Instruction.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class Instruction {
public static final int PT_TRANSFER = 102;
public static final int PT_END_TRIP = 103;
private static final AngleCalc AC = Helper.ANGLE_CALC;
protected final PointList points;
protected PointList points;
protected final InstructionAnnotation annotation;
protected boolean rawName;
protected int sign;
Expand Down Expand Up @@ -137,14 +137,20 @@ public Instruction setTime(long time) {
return points.getElevation(0);
}

/* This method returns the points associated to this instruction. Please note that it will not include the last point,
* i.e. the first point of the next instruction object.
*/
public PointList getPoints() {
return points;
}

public void setPoints(PointList points) {
this.points = points;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this setter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily. The issue is that we have to change the points, once the instruction is created. Changing this might be complicated, but not necessarily impossible.

Once the instructions and PathDetails are calculated, we do the simplification. When simplifying, we need to change the points. I think changing this would require quite a lot overhead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. So the setter is more explicit ... it might be uglier but more efficient to do an inplace modification of the PointList - we have to compare to understand how much this would be

}

/**
* This method returns a list of gpx entries where the time (in time) is relative to the first
* which is 0. It does NOT contain the last point which is the first of the next instruction.
* <p>
*
* @return the time offset to add for the next instruction
*/
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/com/graphhopper/util/Parameters.java
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,16 @@ public static final class NON_CH {
public static final String MAX_NON_CH_POINT_DISTANCE = ROUTING_INIT_PREFIX + NON_CH_PREFIX + "max_waypoint_distance";
}

/**
* Properties for the details response
*/
public static final class DETAILS {

public static final String PATH_DETAILS = "details";

public static final String AVERAGE_SPEED = "average_speed";
}

public static final class PT {
public static final String EARLIEST_DEPARTURE_TIME = "pt.earliest_departure_time";
public static final String PROFILE_QUERY = "pt.profile";
Expand Down