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

Interpolates elevation for tunnels and bridges, fixes issue #713 #798

Closed
wants to merge 16 commits into from

Conversation

@highsource
Copy link
Contributor

commented Sep 19, 2016

This is the fix for #713. It adds elevation interpolation for tunnels and bridges.

For this to work you'll need to turn on the generic flag encoder:

graph.flag_encoders=generic,car,foot

This will probably also require using 8 bytes for flags:

graph.bytes_for_flags=8

Elevation can be turned off via:

graph.elevation.interpolate.bridges=false
graph.elevation.interpolate.tunnels=false

(Both settings are true by default.)

See this tweet for example.

How it works.

For each tunnel (or bridge) edge, the algorithm first finds a connected component of tunnel (bridge) edges. Thus we can find "inner" and "outer" nodes of the tunnel. Outer nodes have tunnel and non-tunnel edges whereas inner nodes only have tunnel edges. Outer nodes are considered to be on the Earth surface, therefore we can trust the elevation provider for their elevation.

Once we've found inner and outer nodes, we'll interpolate elevations of inner nodes based on the elevations of the outer nodes. Interpolation method depends on the number of outer nodes. Cases for 0 or 1 outer nodes are trivial. For 2 outer nodes we use linear interpolation. For 3 outer nodes we use planar interpolation. For 4 and more outer nodes we use the following method.

This is how we get elevations of the inner nodes. Elevation of pillar points is then calculated using linear interpolation using distances from the pilar node to the tower nodes.

Few further hints.

  • I had to add some default implementation to DataFlagEncoder.flagsDefault, as it was called during initialization.
  • I had to add motorroad and forrestry to the highwayList in DataFlagEncoder, otherwise GraphHopper was failing with Graph not prepared for highway=forestry error during initialization.
  • DataFlagEncoder seems to require 26 bits which I think is a bit too much. It does not feel completely finished yet, but is sufficient for this issue.

I'd be grateful for the review and merge.

@karussell karussell added this to the 0.8 milestone Sep 19, 2016

@karussell

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

Few further hints.

Thanks! The DataFlagEncoder is a bit unpolished at the moment.

DataFlagEncoder seems to require 26 bits which I think is a bit too much

This is correct and will continue to grow probably to 64bit. E.g. we store the maximum speed for cars currently forward&backward leading already to 2*5bits - we could probably do a bit better if we do not store multiples of 5 but use e.g. 0,1,2,3,4, ... for common speeds like 5, 10, 20, 30, 50?

Also the intention is to fully move everything to this encoder (as often "raw data" is necessary) and potentially breaking the long-flags into two ints and adding a third int, then providing edge.getData(int). This way we are no longer limited by 64bits. But for the most use cases 64 will be sufficient, at least this is the plan :)

Elevation can be turned off via:

You mean interpolation here? I would not make this configurable. I would enable interpolation as soon as elevation is enabled and the DataFlagEncoder is there.

@highsource

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2016

I understood the intention of the DataFlagEncoder but I also think we'll need more that 64 bits in the long run. Optimizing speed bits would give 2-4 bits most (I don't think we can go below 8 different speeds, that's at least 3 bits per direction, but 4 bits is more realistic).

Elevation can be turned off via:
You mean interpolation here? I would not make this configurable. I would enable interpolation as soon as elevation is enabled and the DataFlagEncoder is there.

Yes, interpolation. I've added these configuration options for a backward compatibility path. I'm OK to drop it. Should I do it or could you do it upon merge?

@karussell

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

(I don't think we can go below 8 different speeds, that's at least 3 bits per direction, but 4 bits is more realistic).

What is even worse: we need two speed values for hgv and potentially others too. Highly likely a dynamic storage size per edge should be introduced but then we cannot easily use it like an array and would need a binary search for every index. Not sure yet how to solve this efficiently.

I've added these configuration options for a backward compatibility path.

Currently no one uses DataFlagEncoder so we should be save :)

I'm OK to drop it. Should I do it or could you do it upon merge?

I can do this

// throw new RuntimeException("do not call flagsDefault");
// TODO This is called on each of the encoders so I had to replace the runtime exception with something,
// but I'm not sure this is correct.
return setAccess(0, forward, backward);

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Will have a look why this is called and what to do, normally it is:

    long flags = speedEncoder.setDefaultValue(0);
    return setAccess(flags, forward, backward);

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

I think speedEncoder was null, but I'll check again. I did not want to change the DataFlagEncoder a lot, that did not feel completely finished yet and it was not in the focus.


@Test
public void testHighwaySpeed() {
Map<String, Double> map = new HashMap<>();
Map<String, Double> map = new LinkedHashMap<>();

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Thanks!

final FlagEncoder genericFlagEncoder = ghStorage.getEncodingManager().getEncoder("generic");
if (!(genericFlagEncoder instanceof DataFlagEncoder)) {
throw new IllegalStateException("Generic flag encoder equired for elevation interpolation of bridges and tunnels is enabled but does not habe the expected type " + DataFlagEncoder.class.getName() + ".");
} else {

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

I know this is a bit of taste: but I prefer to avoid nested if-else and here one could avoid the else as the if stops in every case.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Ok, that's really a matter of taste. I prefer this type if/else blocks, make me think less (like I don't have to check if the if block stops or not). I'd like to leave it as is if it's not that important for you.

import gnu.trove.set.TIntSet;
import gnu.trove.set.hash.TIntHashSet;

public abstract class AbstractEdgeElevationInterpolator {

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Would you mind to add one sentence description to every new class. I know the current state is not always this way but we should improve inline documentation a bit :)

Also the license header is missing in the new classes, plus feel free to add the @author tag here.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Will do.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

ps. We could also use License Maven Plugin, it can automatically add or check license headers. I used it for a number of GPL projects and it worked very well.

int firstNodeId = edge.getBaseNode();
int secondNodeId = edge.getAdjNode();

double lat0 = storage.getNodeAccess().getLat(firstNodeId);

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

I would assign nodeAccess to a local variable outside of the loop. Potentially the JVM is already doing this but it also reads a bit faster :)

this.nodeElevationInterpolator = new NodeElevationInterpolator(storage);
}

protected abstract boolean isStructureEdge(EdgeIteratorState edge);

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Can we rename this method? E.g. isInterpolableEdge or something similar?

public static final double EPSILON = 0.00001;
public static final double EPSILON2 = EPSILON * EPSILON;

public double calculateElevation(double lat, double lon, double lat0, double lon0, double ele0, double lat1,

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

It should be more clear what this method does. Especially as the parameters are somehow unexpected. Add a comment here?

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Ah, now I understood. This is the case for 2 outer points, the other calculateElevation methods or for 3 outer points and 4+

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

I'll give these methods more elaborate names.

@karussell
Copy link
Member

left a comment

Thanks!

Would you mind also to check the code formatting, see #770 (comment)

// throw new RuntimeException("do not call flagsDefault");
// TODO This is called on each of the encoders so I had to replace the runtime exception with something,
// but I'm not sure this is correct.
return setAccess(0, forward, backward);

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

I'll check why this got called. But we could just remove this method and the super method will be called:

long flags = speedEncoder.setDefaultValue(0);
return setAccess(flags, forward, backward);

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

I think speedEncoder was null, but I'll check.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Yes, speedEncoder is null. I'll leave return setAccess(0, forward, backward); for now, I've also filed #800 for this.

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

Ah, yes. One would need the carFwdFlagEncoder here ... but we also have backward, hmmh. I would like to get rid of this.

public class BridgeElevationInterpolatorTest {

private static final double EPSILON= 0.0000000001d;
private static final ReaderWay BRIDGE_WAY;

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

I do not like mutable static variables that much - you do not write to it but it is allowed and could then introduce problems for multithreaded unit test execution. In test we also do not need to care about object creation so I'd vote to just remove the static and put the initialization in the @Before method.

graph = new GraphHopperStorage(new RAMDirectory(), new EncodingManager("generic,foot", 8),
true, new GraphExtension.NoOpExtension()).create(100);

dataFlagEncoder = (DataFlagEncoder) graph.getEncodingManager().getEncoder("generic");

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

BTW: If you dislike casts you can also do:

dataFlagEncoder = new DataFlagEncoder();
FootFlagEncoder foot = new FootFlagEncoder();
graph = new GraphHopperStorage(new RAMDirectory(), new EncodingManager(Arrays.asList(dataFlagEncoder,foot), 8),
                true, new GraphExtension.NoOpExtension()).create(100);

import gnu.trove.set.hash.TIntHashSet;

public class TunnelElevationInterpolatorTest {

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Do we really need a separate test for tunnels? What is different with them from the code concept? Or maybe create an abstract tester with two subclasses or one test with two instances?

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Tunnel test is actually the main test. Test for bridges is to smoke-test (if it works at all) as well to check interpolation of pillar nodes. So that's the same concept but these tests test different parts of interpolation.

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

Ok, that is good.

@@ -56,17 +59,64 @@ public void testHighway() {
edge = GHUtility.createMockedEdgeIteratorState(0, flags);
assertEquals("_default", encoder.getHighwayAsString(edge));
}

@Test
public void testTunnel() {

This comment has been minimized.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Perhaps formatting? Spaces vs. tabs.


@Test
public void testHighwaySpeed() {
Map<String, Double> map = new HashMap<>();
Map<String, Double> map = new LinkedHashMap<>();

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Ah, ups - thanks! Maybe we should require a LinkedHashMap for encoder.getHighwaySpeedMap etc?

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

I think it's a bit weird to pass have an implementation class like LinkedHashMap in the signature. I'll think of options.


assertEquals(Helper.createPointList3D(
43.7405648173518, 7.429926635993867, 17.0,
43.74016602501644, 7.42986703134776, 19.042999267578125, // Without interpolation: 23.0

This comment has been minimized.

Copy link
@karussell

karussell Sep 19, 2016

Member

Oh, the many digits after comma are a bit ugly as they suggests a false precision. Should we round it when interpolating e.g. to cm or even just dm?

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

This is due to using equals of PointList. Normally doubles are compared with certain precision, this does not work when doing equals on the whole object.
I think I'll just check elevations with some epsilon.

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

I meant in general: when we store elevation with so many digits people could assume that we have also very precise data, so regardless of this ugliness in the test it might be wise to cut off precision?

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Makes sense.

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

I think I'll round it to 0.5m. I think that's the precision we get from elevation provider so we shouldn't pretend our interpolation is much better.
Next, 0.5m would allow us to use just 16 bits for elevations (14 bits for 0...16384, 1 bit for sign, 1 bit for .0/0.5).

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

0.5m is good here but might not be sufficient in general, not sure. E.g. I know from a customer using GH with more precise elevation data plus doing interpolation in between so they really need cm precision at least otherwise they would have to big steps in-between making some other routing algorithms a bit more ugly

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

Ok, unerstood. 0.01m then.

@highsource

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

Thank you for the review! I'll make the changes and commit shortly.

@Test
public void interpolatesElevationOfTunnelWithFourOuterNodes() {

// @formatter:off

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

You should be able to trick all formatters via // on all lines

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

No, not Eclipse. :( It strips leading spaces in comments.


@After
public void tini() {
graph.close();

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

Here it is safer to use Helper.close(graph) as this checks for null and null could happen e.g. if init failed.

BTW: tini is a fun name :) I used setUp and tearDown

/**
* @author Alexey Valikov
*/
public abstract class AbstractEdgeElevationInterpolatorTest {

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

The abstract test class should be named XYTester otherwise it will be executed automatically from maven and fail

This comment has been minimized.

Copy link
@highsource

highsource Sep 20, 2016

Author Contributor

No problem to rename, but I don't think Maven/JUnit does not execute test in abstract classes (I've just rechecked it).

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

Ah, ok then you can keep it. Maybe this changed in some more recent maven or junit version.

@highsource

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

Done.

.gitignore Outdated
@@ -2,7 +2,7 @@ target/
*~
TODO.txt
*-gh/
*.osm
*.osm*

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

This is a bit dangerous as we have many *osm.gz files under core/files/ ... probably better to create a folder like osm-data (and put the temp osm files there?) which is then ignored?

* Abstract base class for tunnel/bridge edge elevation interpolators. This
* class estimates elevation of inner nodes of a tunnel/bridge based on
* elevations of entry nodes. See
* <a href="https://github.com/graphhopper/graphhopper/issues/713">#713</a> for

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

I do not link to github explicitly inside of the code and just say #xy. This way we can easily migrate to other issue trackers in the future.

}

public double roundToPrecision(double value) {
return Math.round(value / PRECISION) * PRECISION;

This comment has been minimized.

Copy link
@karussell

karussell Sep 20, 2016

Member

See also Helper.round (not sure if applicable though)

karussell added a commit that referenced this pull request Sep 23, 2016

@karussell

This comment has been minimized.

Copy link
Member

commented Sep 23, 2016

Thanks again!

Merged via 90a3844

@karussell karussell closed this Sep 23, 2016

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.