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

Check required tags of maneuver relations #4905

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Feb 21, 2018

Issue

Accordingly to https://github.com/Project-OSRM/osrm-backend/wiki/Maneuver-override-tag ways from, to and node via roles are required in relations with type=maneuver.

PR adds an additional check for via_node ids to filter out invalid maneuver relations.

/cc @karenzshea @TheMarex

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@oxidase oxidase requested a review from karenzshea February 21, 2018 12:05
@oxidase oxidase force-pushed the maneuver-required-tags branch from e9f5b75 to 8b6fe41 Compare February 21, 2018 12:06
@oxidase oxidase requested a review from TheMarex February 21, 2018 12:06
@oxidase oxidase self-assigned this Feb 21, 2018
@oxidase oxidase added this to the 5.16.0 milestone Feb 21, 2018
@oxidase oxidase added the Review label Feb 21, 2018
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

I'm not sure this is really fixing the issue, seems to me that via ways now only work if you also pass in a via node.

@@ -103,18 +104,12 @@ ManeuverOverrideRelationParser::TryParse(const osmium::Relation &relation) const
}
}

if (from && (via || via_ways.size() > 0) && to)
if (from != SPECIAL_OSM_WAYID && to != SPECIAL_OSM_WAYID && via_node != SPECIAL_OSM_NODEID)
Copy link
Member

Choose a reason for hiding this comment

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

This check won't work when there are only via ways right? In that case via_node will not be set only via_ways.

Copy link
Member

Choose a reason for hiding this comment

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

As per chat, that is intended behavior, because a way is not sufficient to infer the instruction position.

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

Looks good. 👍 The spec is a little confusing, but there needs to be a via node even if there is a via way.

@@ -103,18 +104,12 @@ ManeuverOverrideRelationParser::TryParse(const osmium::Relation &relation) const
}
}

if (from && (via || via_ways.size() > 0) && to)
if (from != SPECIAL_OSM_WAYID && to != SPECIAL_OSM_WAYID && via_node != SPECIAL_OSM_NODEID)
Copy link
Member

Choose a reason for hiding this comment

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

As per chat, that is intended behavior, because a way is not sufficient to infer the instruction position.

@oxidase oxidase force-pushed the maneuver-required-tags branch from 8b6fe41 to d7ad051 Compare February 21, 2018 13:06
@oxidase
Copy link
Contributor Author

oxidase commented Feb 21, 2018

@TheMarex I've added checks for Required and only one member., otherwise a maneuver placement can be ambiguous.

@oxidase oxidase force-pushed the maneuver-required-tags branch from d7ad051 to 0b2d772 Compare February 21, 2018 13:20
@oxidase oxidase merged commit 5acf660 into master Feb 21, 2018
@oxidase oxidase deleted the maneuver-required-tags branch February 21, 2018 13:49
datendelphin added a commit to fossgis-routing-server/osrm-backend that referenced this pull request Jun 10, 2018
  - Changes from 5.15.2:
    - Guidance
      - ADDED Project-OSRM#4676: Support for maneuver override relation, allowing data-driven overrides for turn-by-turn instructions [Project-OSRM#4676](Project-OSRM#4676)
      - CHANGED Project-OSRM#4830: Announce reference change if names are empty
      - CHANGED Project-OSRM#4835: MAXIMAL_ALLOWED_SEPARATION_WIDTH increased to 12 meters
      - CHANGED Project-OSRM#4842: Lower priority links from a motorway now are used as motorway links [Project-OSRM#4842](Project-OSRM#4842)
      - CHANGED Project-OSRM#4895: Use ramp bifurcations as fork intersections [Project-OSRM#4895](Project-OSRM#4895)
      - CHANGED Project-OSRM#4893: Handle motorway forks with links as normal motorway intersections[Project-OSRM#4893](Project-OSRM#4893)
      - FIXED Project-OSRM#4905: Check required tags of `maneuver` relations [Project-OSRM#4905](Project-OSRM#4905)
    - Profile:
      - FIXED: `highway=service` will now be used for restricted access, `access=private` is still disabled for snapping.
      - ADDED Project-OSRM#4775: Exposes more information to the turn function, now being able to set turn weights with highway and access information of the turn as well as other roads at the intersection [Project-OSRM#4775](Project-OSRM#4775)
      - FIXED Project-OSRM#4763: Add support for non-numerical units in car profile for maxheight [Project-OSRM#4763](Project-OSRM#4763)
      - ADDED Project-OSRM#4872: Handling of `barrier=height_restrictor` nodes [Project-OSRM#4872](Project-OSRM#4872)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants