remove assertions that could be triggered by bad data

[MoKob]

Some of the assertions in various coordinate extraction steps are not correct. It's possible for valid OSM geometry to contain nodes that are at the same coordinate (either via users placing duplicate coordinates, or decimal rounding snapping nearby points to the same place). We are currently asserting that this can't happen, even though it can.

This PR disables those assertions.

In addition, because there is no definition of "bearing" between two identical points, the current behaviour is undefined. This PR also adds some tests that when we encounter this situation, we return 0 for the bearing. Although still incorrect, at least it's consistent and testable.

A full fix for this requires #3470 which is more complex, this PR is just to unblock the demo server updates. We will live with #3470 being a known issue for now.

Tasklist

• review
• adjust for comments

Requirements / Relations

Related to: #3470

[TheMarex]

The main problem I see with just disabling the assertions is that right now we have a function that can fail. For functions like that the calling code needs to check if it was successful.

Even if you want to remove this failure handling in the future because you found a nicer way to deal with it, it needs to be there in the current iteration.

[MoKob]

I don't really see what you imagine to be in here. There is nothing else that can be returned in that situation. As long as you have ways from a to b with loc(a) == loc(b) there is now way of finding a coordinate c on that way with loc(c) != loc(a). The coordinate will always be at the same location.
As long as the code provides these coordinates to the graph creation, graph creation will not be able to come up with a way around these.
But feel free to implement handling against pushing down these to the graph creation in the first place.

Edit: to be clear: failing here gives a random bearing value. A road without any bearing will introduce randomness in detection obvious turns/forks and introduce a random value in the intersection classification. As long as this broken data cannot be excluded, I don't see what else we can do. Especially not at this location.

Especially, given the input, the function returns the only valid coordinate. It does, in itself, not fail. It only does not report something that has any meaning, but that's the case the input data reflects. I don't see a reason the code should make cases more reasonable than the input suggests.

[daniel-j-h]

Hrm yeah we really should filter those already in the extraction (thanks for opening #3470), maybe write out a warning and throw them away. Or even build a small tool for same coordinate detection. If this makes our life easier.

I don't think wrapping the result in an optional is semantically more correct here, it's just our graph is already broken to some degree.

[TheMarex]

@MoKob to illustrate what I mean by error handling I just pushed the change. I would really like to see this handled in the graph creation as well, but as of now we don't do that, hence we need the explicit handling.

The problem is handled at the call site, since the function doesn't know that the caller will put the result into a bearing calculation. Hence the lambda that checks if the returned coordinate is not the same as the start coordinate of the segment needed a rename, since in some cases it can be a valid result as you pointed out.

[MoKob]

I have to disagree here. The problem is not handled at the call site. The only handling that is done is logging. The changes to setting bearing are void, since bearing is just as well defined to a single constant value for two identical coordinates. Looking at the math, it should even be zero.

If you actually want to handle it at the call site, we would have to both consider it correctly in roundabout handling (possibly disable roundabout intersections for these cases) and remove them from intersection_classification/turn_discovery. I am not clear on whether this could be done by simply removing the segment (unlikely given all strange modelling in OSM). Handling this case is a complex matter, right now we are only logging it (duplicated even, since the log at the roundabout handling is only called on intersections that the normal intersection generator also generates)

The only effective change done is renaming an assert that at some point will be possible to assert (and at that point would also be fine the way that it was). If you need to change this behaviour to fix the demo server, go ahead.

[MoKob]

removing this will actually negatively affect the turn discovery. It has nothing to do with the changes necessary and has a negative impact on discovery in traffic loops.
I have to say though that I don't remember at which location I had to add this fallback.

[danpat]

I agree with @MoKob, the assertions need to go. They're faulty as-is - we're asserting an invalid proposition that ignores the possibility of zero-length edges. Although the data is stupid, it is valid in OSM, we shouldn't crash hard when we come across it. Printing a warning would be reasonable if we can't work around it.

There is all kinds of junk data in OSM - duplicated ways with identical node coordinates are very common, as are duplicated nodes (iD at one point had a bug that made it easy to add many nodes at the same spot). We have to figure out a way to live with this data.

@MoKob Do you think you could sketch out a few cucumberjs scenarios for this ugliness? I suspect there are some situations we can probably resolve and some that are just going to result in garbage results - it'd be great to catalogue these so we understand what junk we can handle and what we can't.

If we can systematically identify totally unusable geometry, we can produce a report and the OSM community and/or the Mapbox data team can help clean up the bad stuff.

[MoKob]

@danpat that would be part of #3470. Here we kind of don't have any possibilities other than disabling the assertion, for now. At some point we would want to re-introduce it, since when we handle the case before, we should never get into the bad situation here...

[danpat]

@MoKob @daniel-j-h I've tidied up and squashed this PR. Can you please sanity check that I didn't break anything during the rebase?

I've added a test case to verify that we always return 0. as a bearing for identical coordinates. The bearing is still invalid, but at least it's a known value we might recognize if someone notices the problem in a route.

If everything looks correct, lets get this merged into master and unblock the demo server. @daniel-j-h feel free to hit the merge button, or I can do it during my day tomorrow.

I've tidied up the checks. Full fix should be done with #3470.

[daniel-j-h]

Is .back() always safe here or do we have to assert for !empty?

[MoKob]

since we have at least the intersection node, we cannot have less than one coordinate

[daniel-j-h]

Please clearly ticket the TODOs and the reasoning behind them in #3470 or even make a separate ticket. I can see us forgetting about them, as it is always the case with TODOs / FIXMEs.

[MoKob]

I've added tasks to the todos to #3470.

[daniel-j-h]

Running a staging demo server with this - let's wait till we know for sure it runs through this time.

[danpat]

So close, but not quite. @daniel-j-h the staging server just asserted the following:

[assert] /mnt/ebs/osrm/2017-01-04-161647/src/extractor/guidance/sliproad_handler.cpp:309
in: virtual osrm::extractor::guidance::Intersection osrm::extractor::guidance::SliproadHandler::operator()(NodeID, EdgeID, osrm::extractor::guidance::Intersection) const: coords.size() >= 2
terminate called without an active exception

I'm out of time to dig into this deeper today :-(

[MoKob]

I've had a look. Simply was another case of these duplicated coordinates. Since the coordinate extractor filters these duplicates for easy handling as a precondition, we can end up seeing only one coordinate in the result.

The sliproad handler assumed to have at least two. I've checked all instances of the coordinate_extractor used in the handlers and found this to be the only occurrence where it happened.

[daniel-j-h]

Starting the toolchain on the staging demo server anew.

[danpat]

[assert] /mnt/ebs/osrm/2017-01-05-121703/src/extractor/guidance/coordinate_extractor.cpp:166
in: osrm::util::Coordinate osrm::extractor::guidance::CoordinateExtractor::ExtractRepresentativeCoordinate(NodeID, EdgeID, bool, NodeID, uint8_t, std::vector<osrm::util::Coordinate>) const: not_same_as_start(result)
terminate called without an active exception

Getting closer, we're now on a different assert (about 85-90% of the way through Generating edge-expanded edges).

[daniel-j-h]

Starting another toolchain run - maybe this time. cc @danpat

[MoKob]

from the on first glance unrelated change, the tile size check failed. @danpat do you have any further insight into why this happens/if that adjustment would be fine?

Is there a way we can check tile sizes other than exact sizes which may change when the file is updated (e.g. since we need new features) or can be affected by changes like these?
I am thinking of some appropriate range to make sure than the tile size does not explode, but we don't have to adjust on changes like the turn angles I just edited.

[danpat]

@MoKob If some of the turn angles changed, then it's likely that the varint packing in the PBF vector tile ended up with a different number of bytes.

Changes to angles in the tiles should only vary the size by a couple of bytes, so adding a +/- of a small amount is probably fine.

Some of the tile tests verify that the expected turn values are in place, like here:

https://github.com/Project-OSRM/osrm-backend/blob/master/unit_tests/library/tile.cpp#L332-L334

maybe we should drop the size check now that the platform-dependent differences in tile size problems have been corrected.

If it's fixed for now, I'm inclined to leave it alone though - if it happens again, let's adjust the tile tests.