Refactor fork handling in guidance #3264

chaupow · 2016-11-08T14:59:22Z

Issue

This PR works towards #3099

Tasklist

refactor TurnHandler::findFork() code
refactor TurnHandler::isObviousOfTwo()
revise definition of forks and TurnHandler::findFork()
refactor IntersectionHandler::assignFork()
add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

Tasklist unrelated to this PR (but arose during this work)

rename struct Intersection in route_step.hpp
revise BasicTurnType
move isObviousByRoadClass in turn_handler.hpp to a dedicated .compatible() in road_class (road class refactor)
refactor struct Fork to use a begin and end iterator instead of right and left

Code Review Todolist

deduplicate leftmost / rightmost code
add OSRM_ATTR_WARN_UNUSED to setTurnTypes in turn_handler.hpp
refactor findFork() return type
capsule this in own function
revert this condition
Make member function: 1 2 3.

daniel-j-h · 2016-11-08T17:17:34Z

include/extractor/guidance/intersection.hpp

+// `ConnectedRoads` is a relative view of an intersection by an incoming edge.
+// `ConnectedRoads` are streets at an intersection ordered from from sharp right counter-clockwise to
+// sharp left where `connected_roads[0]` is _always_ a u-turn
+struct ConnectedRoads final : public std::vector<ConnectedRoad>


Why the change in the API here? This will break all users throughout osrm-backend.

Also semantically it it an intersection represented as ordered roads.

daniel-j-h · 2016-11-08T17:19:26Z

include/extractor/guidance/intersection_handler.hpp

+    //     _at_ `node_based_graph.GetTarget(via_edge)`
+    //     _from_ `NodeID`
+    //     _over_ `via_edge`
+    //     _from where_ there are other `connected_roads`


What is _at_ and _from where_ all about?

updated comment to make it less confusing

daniel-j-h · 2016-11-08T17:22:27Z

src/extractor/guidance/intersection_handler.cpp

+// Checks whether a turn from `via_edge` onto `road` is a turn
+// - on a ramp
+// - a continue
+// - or a normal turn


What does this mean wrt. our turn instructions?

https://github.com/Project-OSRM/osrm-backend/blob/master/include/extractor/guidance/turn_instruction.hpp#L33

moved to unrelated todo

daniel-j-h · 2016-11-08T17:25:57Z

src/extractor/guidance/turn_handler.cpp

+            {
+                break;
+            }
+        }


I think you can express this and above more clearly using a adjacent-ish stdlib algorithm

done. thanks!

daniel-j-h · 2016-11-08T17:27:15Z

src/extractor/guidance/turn_handler.cpp


-        const bool valid_indices = 0 < right && right < left;
+        BOOST_ASSERT(0 < right);
+        BOOST_ASSERT(right < left);


Change from a check to an assert: does this always hold?

yes. (hopefully.) yes.

daniel-j-h · 2016-11-08T18:03:49Z

src/extractor/guidance/turn_handler.cpp

+
+        // there cannot be a fork of more than three streets
+        std::size_t size_of_fork = left - right + 1;
+        const bool not_more_than_three_old = (left - right) <= 2;


daniel-j-h · 2016-11-08T18:09:02Z

Looking at your refactored findFork:

What is the return type? A std::pair<std::size_t, std::size_t> expresses no semantic meaning. Looks like those are indices into the Intersection array. What if findFork fails, what's the return type then? What about the following: either return an iterator pair, or create a local struct Fork { .. }; and change to optional<Fork> findFork(..);.
Does findFork has to work with all settings or can you put a pre-condition in to only handle specific scenarios, e.g. the Intersection array has to have a size of at least 3 (1 uturn + 2 for fork).
What about refactoring this into its own findClosestToStraight (feel free to change all namings mentioned here) function and then using e.g. a min stdlib function.
Looks like we only handle this branch. What about inverting the condition and returning early if it's not met. Otherwise we need to carry around an extra level of indentation. Related: please clang-format-3.8 your code.
The leftmost / rightmost code here is duplicated. Should be able to 1/ put it into its own function and 2/ write once, run on fwd iterators and then again on reverse iterators. You can probably use adjacent find from the stdlib for this.
Why?
Make member function: 1 2 3.
Use the GeoJSON printer and check what you classify as fork e.g. on Berlin.
Then check this heuristic.

MoKob

The actual change is completely hidden in the unrelated changes.

These should, at the least, be separated from each other and I feel we should really discuss this ConnectedRoads change, as I don't really agree with it.

MoKob · 2016-11-09T10:48:05Z

include/extractor/geojson_debug_policies.hpp

@@ -27,7 +27,7 @@ struct IntersectionPrinter
    // renders the used coordinate locations for all entries/as well as the resulting
    // intersection-classification
    util::json::Array operator()(const NodeID intersection_node,
-                                 const extractor::guidance::Intersection &intersection,
+                                 const extractor::guidance::ConnectedRoads &intersection,


can we split unrelated changes like these into a dedicated PR?

MoKob · 2016-11-09T10:49:48Z

include/extractor/guidance/turn_analysis.hpp


    std::vector<TurnOperation>
-    transformIntersectionIntoTurns(const Intersection &intersection) const;
+    transformIntersectionIntoTurns(const ConnectedRoads &intersection) const;


if you rename it, than intersection in the name here does not make any sense.

MoKob · 2016-11-09T10:50:23Z

include/extractor/guidance/turn_analysis.hpp

@@ -64,8 +65,8 @@ class TurnAnalysis
    const SliproadHandler sliproad_handler;

    // Utility function, setting basic turn types. Prepares for normal turn handling.
-    Intersection
-    setTurnTypes(const NodeID from, const EdgeID via_edge, Intersection intersection) const;
+    ConnectedRoads


we should add OSRM_ATTR_WARN_UNUSED here.

MoKob · 2016-11-09T10:52:51Z

include/extractor/guidance/intersection_generator.hpp

-    Intersection MergeSegregatedRoads(const NodeID intersection_node,
-                                      Intersection intersection) const;
+    ConnectedRoads MergeSegregatedRoads(const NodeID intersection_node,
+                                        ConnectedRoads intersection) const;


if it's important to have it named ConnectedRoads to you, then we should also change the name intersection into connected_roads to not loose sync between type and name. Right now (Intersection intersection) connects the variable to its type

MoKob · 2016-11-09T10:53:12Z

include/extractor/guidance/intersection_handler.hpp

+    //
+    //                                                 |
+    //                                                 |
+    //                                         (connected_roads[i])


comments out of sync with name below (connected_roads vs intersection)

MoKob · 2016-11-09T10:55:39Z

include/extractor/guidance/turn_classification.hpp

@@ -25,7 +25,7 @@ namespace guidance
 {

 std::pair<util::guidance::EntryClass, util::guidance::BearingClass>
-classifyIntersection(Intersection intersection);
+classifyIntersection(ConnectedRoads intersection);


Same here, if we name it roads, the connection between intersection and connected roads is not clear.

Sure, a vector<ConnectedRoad> is a list of ConnectedRoads, but so is std::set<ConnectedRoad> or std::list<ConnectedRoad>. I don't really like removing the additional information that is inherently given by having a specifically sorted list of Roads. Especially with the added functionality.

It's a dedicated class, not simply a plain collection.

chaupow · 2016-11-09T11:22:48Z

Regarding Intersection vs ConnectedRoads:

The main reason for the change is that there is another struct Intersection in route_step.hpp and I thought that might be confusing.

As the discussed Intersectionis basically a vector of <ConnectedRoad>, @TheMarex and me thought that ConnectedRoads makes sense and is also inline with the naming conventions with some other vectors and connected_roads[0] or connected_roads.size() is pretty intuitive to me.

However, if you think we should rename it to something more similar to Intersection I'm open to it and I’ll undo the Intersection changes.

MoKob · 2016-11-09T11:51:38Z

Capturing from chat:

in short: yes, we should to stuff about intersection vs intersection. But I feel renaming intersection in engine should be the way to go, since it's only a output part

[12:21]
by adding more functionality to intersection in preprocessing (e.g. findClosestTurn), we are moving away from a pure collection

[12:22]
ConnectedRoads is a really bad choice for me, since it would be valid for a set, a vector, a map, a list, basically any collection type

[12:24]
intersection.size() vs connected_roads.size() does not improve readability to me. The size of an intersection conveys some meaning, size of connected roads is (especially since s vs no s is easily lost) way less meaningful to me

[12:25]
you could easily confuse .size() with the size of a road (e.g. width)

also you have the sorting criterium which implies meaning. With the addition of .valid(), it moves away form a pure collection to an ordered list of items. So there is more meaning to it rather than just a random collection of entries

In total: We are renaming the route-step intersection. Name up for discussion.

TheMarex · 2016-11-12T14:37:07Z

in short: yes, we should to stuff about intersection vs intersection. But I feel renaming intersection in engine should be the way to go, since it's only a output part

Being on the output part is actually the prime reason I would argue against remaining it. We always refer to this object in the docs as Intersection object and having parity with docs and internal class names makes it quite easy to orient yourself in the API code. Changing anything that is touched by people besides internal developers always leads to confusion. In context of #3228 this might get even more confusing.

You could maybe use IntersectionModel or InternalIntersection or something similar to resolve the name conflict.

chaupow · 2016-11-15T01:30:58Z

I did some work on this PR, things happened so far:

commented code that was necessary to understand how fork handling works
refactor findFork()
slightly refactor other functions and files that are strongly related to findFork()

what these change not do:

changes of the behaviour of findFork(); refactor was only done regarding code readibility
refactor of code outside findFork()

With these thigns in mind, would you look at the code again before I continue work here? Thanks a lot, lot, lot! @daniel-j-h @MoKob

daniel-j-h

I left some comments on the PR - also please see #3264 (comment)

daniel-j-h · 2016-11-15T14:31:43Z

include/extractor/guidance/intersection.hpp

+// `angle`      is given counter-clockwise:
+//              0 = uturn, 90 = right, 180 = straight, 270 = left
+// `bearing`    is the direction in clockwise angle from true north after taking the turn:
+//              0 = heading north, 90 = east, 180 = south, 270 = west<<<<<<< HEAD


daniel-j-h · 2016-11-15T14:38:33Z

src/extractor/guidance/intersection.cpp

+    // if no entry was found that forbids entry, the intersection entries in the range are all
+    // valid.
+    return true;
+}


If you make this iterator based, the previous ~15 lines are

return all_of(first, last, mem_fn(&T::allowed));

daniel-j-h · 2016-11-15T14:40:27Z

src/extractor/guidance/intersection_handler.cpp

@@ -45,6 +45,8 @@ std::size_t IntersectionHandler::countValid(const Intersection &intersection) co
    });
 }

+// Checks whether a turn from `via_edge` onto `road` is a turn, on a ramp, a continue, or a normal
+// turn


Again, does this comment say the only TurnTypes the function handles is Turn, OnRamp, Continue?

daniel-j-h · 2016-11-15T14:42:17Z

include/extractor/guidance/turn_handler.hpp

@@ -45,9 +45,26 @@ class TurnHandler : public IntersectionHandler
                            Intersection intersection) const override final;

  private:
+    struct Fork;


This only forward declares Fork making it an incomplete type in this header

daniel-j-h · 2016-11-15T14:48:35Z

include/extractor/guidance/turn_handler.hpp

@@ -68,8 +85,7 @@ class TurnHandler : public IntersectionHandler
    handleDistinctConflict(const EdgeID via_edge, ConnectedRoad &left, ConnectedRoad &right) const;

    // Classification
-    std::pair<std::size_t, std::size_t> findFork(const EdgeID via_edge,
-                                                 const Intersection &intersection) const;
+    Fork findFork(const EdgeID via_edge, const Intersection &intersection) const;


but here you're trying to use an incomplete type by-value as return type.

daniel-j-h · 2016-11-15T15:15:00Z

src/extractor/guidance/turn_handler.cpp

+        }
+    }
+    return true;
+}


Can you express these two nested raw for loops via stdlib algorithms?
The outer one looks like a for each, the inner one like a any-of.

daniel-j-h · 2016-11-15T15:16:25Z

src/extractor/guidance/turn_handler.cpp

+// Checks whether a three-way-intersection coming from `via_edge` is a fork
+// with `intersection` as described as in #IntersectionExplanation@intersection_handler.hpp
+TurnHandler::Fork TurnHandler::findFork(const EdgeID via_edge,
+                                        const Intersection &intersection) const


What happens if intersection is empty (the UTurn road only)? Is size = 3 a pre-condition?

it's not a pre-condition but if intersection.size()<3 it will now return boost::none

daniel-j-h · 2016-11-15T15:17:05Z

src/extractor/guidance/turn_handler.cpp

+    // they cannot be fork candidates
+    if ((left == right) || ((left - right + 1) > 3))
+    {
+        return Fork::getEmptyFork();


As said, the default constructor already does that.

daniel-j-h · 2016-11-15T15:18:40Z

src/extractor/guidance/turn_handler.cpp

+    const std::size_t right;
+    const std::size_t left;
+    const std::size_t size;
+    const bool valid;


data + boolean valid is more or less what optional<T> is for - just make it a plain old struct T { U data; }; and then use optional<T> and check with if (myoptional) use(*myoptional);

daniel-j-h · 2016-11-15T15:19:44Z

src/extractor/guidance/turn_handler.cpp

+    // check if all entries in the fork range allow entry
+    const bool only_valid_entries = intersection.hasValidEntries(right, left);
+
+    // TODO check whether 2*NARROW_TURN is too large


Where did the 2 * narrow turn angle go?

I have no idea. It was never there when I touched it. I looked into the history in master a bit and couldnt find it either. @MoKob you remember something about this or can I remove the comment?

It looks like the comment is out of date. Feel free to remove.

daniel-j-h · 2016-12-09T16:54:32Z

include/extractor/guidance/intersection.hpp

+    bool hasValidEntries(Intersection::iterator first, Intersection::iterator last) const;
+
+    Base::iterator findClosestTurn(double angle);
+    Base::const_iterator findClosestTurn(double angle) const;


All of the functions here should no longer be attached to Intersection or IntersectionView - but instead check EnableIntersectionOps<T>. If you have the time for it, read up about the CRTP pattern.

We need this since even though this creates a inheritance hierarchy

struct base {}; struct derived : base {};

the following types stand in no relationship to each other

using BaseVec = vector<base>; using DerivedVec = vector<derived>;

using the CRTP pattern we inject the functionality into both collection types.

daniel-j-h · 2016-12-09T16:55:18Z

include/extractor/guidance/turn_handler.hpp

+        Intersection::iterator right;
+        Intersection::iterator left;
+        const std::size_t size;
+        Fork(Intersection::iterator right, Intersection::iterator left);


Should this work on Intersection or IntersectionView - or both?

Check the data these types hold and decide.

Fork handling is only called on Intersections (in the dedicated handlers), just as the sliproad handler. Right now this is correct. We could think about how we could update this behaviour, but right no I'd say for the scope of this PR having Intersections here is correct.

daniel-j-h · 2016-12-09T16:56:17Z

src/extractor/guidance/intersection.cpp

+
+    return all_of(first, last + 1, [&](const ConnectedRoad &road) { return road.entry_allowed; });
+}
+


This comes from the messed up rebase.

MoKob · 2016-12-12T09:01:36Z

include/extractor/guidance/intersection.hpp

+// The Turn Operation indicates what is exposed to the outside of the turn analysis.
+//
+// `angle`      is given counter-clockwise:
+//              0 = uturn, 90 = right, 180 = straight, 270 = left


the intersection shape itself does not know about where you come from. It does not offer an angle property.

MoKob · 2016-12-12T09:03:39Z

include/engine/guidance/route_step.hpp

-// A representation of intermediate intersections
+// A representation of intermediate intersections:
+// An intersection is an ordered list of connected roads ordered from from sharp right
+// counter-clockwise to sharp left where `intersection[0]` is always a u-turn


the intersection in the output is a different object than the intersection in the preprocessing. Here the concept of Connected Roads is unknown. We offer available turn bearings (in all directions, not angles) and allowed entry flags.

ugh. changed this. don't know how this happened

MoKob · 2016-12-12T09:05:29Z

include/extractor/guidance/turn_handler.hpp

+        Intersection::iterator right;
+        Intersection::iterator left;
+        const std::size_t size;
+        Fork(Intersection::iterator right, Intersection::iterator left);


Fork handling is only called on Intersections (in the dedicated handlers), just as the sliproad handler. Right now this is correct. We could think about how we could update this behaviour, but right no I'd say for the scope of this PR having Intersections here is correct.

MoKob · 2016-12-12T09:06:04Z

include/extractor/guidance/turn_handler.hpp

    bool isObviousOfTwo(const EdgeID via_edge,
                        const ConnectedRoad &road,
                        const ConnectedRoad &other) const;
+
+    bool hasObvious(const EdgeID &via_edge, const Fork) const;


is this the replacement of the above function? Also Fork could probably benefit from a reference, due to its size.

No this is not a replacement, but yes reference sounds good.

MoKob · 2016-12-12T09:09:22Z

include/extractor/guidance/turn_handler.hpp

+    struct Fork
+    {
+        Intersection::iterator right;
+        Intersection::iterator left;


since we are using iterators, wouldn't it be better to follow the stdlib paradigm for begin, end here? I'd prefer a begin / end approach at least, but given that Fork is wrapped into an optional, I'd be alright with left/right. However, left and right requires implicit knowledge about the order. So we could maybe use ccw_right and ccw_left to indicate the order here as well? This way you know which direction to iterate over.

moving to new issue

MoKob · 2016-12-12T09:10:54Z

include/extractor/guidance/turn_handler.hpp

@@ -68,8 +85,7 @@ class TurnHandler : public IntersectionHandler
    handleDistinctConflict(const EdgeID via_edge, ConnectedRoad &left, ConnectedRoad &right) const;

    // Classification
-    std::pair<std::size_t, std::size_t> findFork(const EdgeID via_edge,
-                                                 const Intersection &intersection) const;
+    boost::optional<Fork> findFork(const EdgeID via_edge, Intersection &intersection) const;


could you maybe add documentation on what the difference between findFork and findLeftmost... versions are?

good point. I also renamed findLeftAndRightmostForkCandidates to findForkCandidatesByGeometry which is hopefully a better name because we're just looking at angles and the geometric relations between the roads here.

MoKob · 2016-12-12T09:17:41Z

src/extractor/guidance/turn_handler.cpp

+using namespace osrm::extractor::guidance;
+// given two adjacent roads and `road1` being a candidate for a fork,
+// return false, if next road `road2` is also a fork candidate or
+// return true, if `road2` is not a suitable fork candidate and thus, `road1` the outermost fork


adjacent == any specific order? I'd assume they are still in ccw order? Would be nice to specify this.

MoKob · 2016-12-12T09:19:12Z

src/extractor/guidance/turn_handler.cpp

+    // if all roads are part of a fork, set `candidate` to the last road
+    else
+    {
+        return outermost - 1;


If we would use the begin/end paradigm I mentioned above, we wouldn't need these distinctions here.

moving to new issue

MoKob · 2016-12-12T09:21:19Z

src/extractor/guidance/turn_handler.cpp

+    }
+}
+
+struct StraightestTurnAtIntersection


adding a dedicated struct here seems wrong to me. the straightmost turn at an instersection can be found with getClosestTurn(STRAIGHT_ANGLE) for any intersection and is an iterator into the intersection. It's deviation can easily be computed whenever necessary by angularDeviation(itr.angle,STRAIGHT_ANGLE).

The dedicated struct is overkill in my personal opinion. Is it used often enough to justify the definition?

After checking: given that it is used twice, I feel that introducing a dedicated struct here only adds confusion. Having the indirection using an id instead of the iterator findClosestTurn returns does only add confusion in my view.

good catch. thanks!

MoKob · 2016-12-12T09:21:53Z

src/extractor/guidance/turn_handler.cpp

+// returns a tuple {i, angle_dev} of the road at the intersection that is closest to going straight:
+//     `intersection[i]` is the actual road
+//     `angle_dev` is the angle between going straight and taking the road `intersection[i]`
+StraightestTurnAtIntersection findClosestToStraight(const Intersection &intersection)


can be removed completely and replaced with findClosestTurn(STRAIGHT_ANGLE) in intersection

good catch. thanks!

MoKob

one tiny item left. otherwise 👍

MoKob · 2016-12-15T15:33:13Z

include/extractor/guidance/intersection.hpp

+    {
+        BOOST_ASSERT(end < self()->end());
+
+        return all_of(begin, end, [](const ConnectedRoad &road) { return road.entry_allowed; });


this should use type auto, since we can also call it on an intersectionView which does not have a connected road.

daniel-j-h

A single Travis build is failing with

Disallowing sources: ubuntu-toolchain-r-test

Might be a blip. Please restart and check again.

daniel-j-h · 2016-12-16T16:18:38Z

include/extractor/guidance/intersection.hpp

+    {
+        auto comp = makeCompareAngularDeviation(angle);
+        return std::min_element(self()->begin(), self()->end(), comp);
+    }


Non-const overload for the function above?

There is a trick involving a const-cast so you don't have to duplicate code.
Search around for how to implement const / non-const overloads.

Not sure if we need it here ..

daniel-j-h · 2016-12-16T16:26:55Z

include/extractor/guidance/intersection.hpp

+    bool hasAllValidEntries(const iteratorT begin, const iteratorT end) const
+    {
+        return all_of(begin, end, [](const ConnectedRoad &road) { return road.entry_allowed; });
+    }


std::all_of and not all_of - the reason the unqualified all_of works is argument-dependant lookup (ADL, great to know, read up a bit about it): if the iterators are in the std namespace all_of is searched in the std namespace, too.

Check std::all_ofs docs: it requires an InputIt (make yourself comfortable with the different iterator concepts), which basically means you can run over it once. Because you are using std::all_of your iterators now also have to be (at least) InputIterators. You could assert that at compile time via

static_assert(std::is_base_of<std::input_iterator_tag, typename std::iterator_traits<Iter>::iterator_category>::value, "")

but what you usually do is to make the template's type just InputIt, as in

template <typename InputIt> void fn(InputIt first, InputIt last);

(C++ Concepts will change this in the future and enforce the static assert automatically)

Finally, don't hard code ConnectedRoad as parameter here: the reason we're having the EnableOps CRTP struct is because you don't know if it's a ConnectedRoad or a ViewData. Make it generic.

Thanks 🙇

daniel-j-h · 2016-12-16T16:30:46Z

src/extractor/guidance/turn_handler.cpp

+template <typename Iterator>
+Iterator findOutermostForkCandidate(const Iterator start, const Iterator end)
+{
+    const auto outermost = std::adjacent_find(start, end, isOutermostForkCandidate);


ForwardIt requirement, as explained above

daniel-j-h · 2016-12-16T16:32:07Z

src/extractor/guidance/turn_handler.cpp

+
+// a wrapper to handle road indices of forks at intersections
+TurnHandler::Fork::Fork(Intersection::iterator right, Intersection::iterator left)
+    : right(right), left(left), size((int)(left - right) + 1)


Why int? And why signed at all?

daniel-j-h · 2016-12-16T16:34:35Z

src/extractor/guidance/turn_handler.cpp

-    if (turn_is_perfectly_straight && in_data.name_id != EMPTY_NAMEID &&
-        road_data.name_id != EMPTY_NAMEID && same_name)
+        via_data.name_id, road_data.name_id, name_table, street_name_suffix_table);
+    if (turn_is_perfectly_straight && via_data.name_id != EMPTY_NAMEID && same_name)


Wait, the empty name id check has to happen before the requiresNameAnnounced above

daniel-j-h · 2016-12-16T16:35:27Z

src/extractor/guidance/turn_handler.cpp

+    for (auto road = fork.right; road < fork.left; ++road)
+    {
+        if (isObviousOfTwo(via_edge, *road, *(road + 1)) ||
+            isObviousOfTwo(via_edge, *(road + 1), *road))


What is +1 here? Is this always safe to do?

It is, and it is ugly and I tagged it in another issue to fix this #3457 😞

daniel-j-h · 2016-12-16T16:36:47Z

src/extractor/guidance/turn_handler.cpp

-                                                          const Intersection &intersection) const
+// finds a fork candidate by just looking at the geometry and angle of an intersection
+boost::optional<TurnHandler::Fork>
+TurnHandler::findForkCandidatesByGeometry(Intersection &intersection) const


Do you ever modify the intersection? If yes, why? If no => const!

Oh, maybe you can help me here cause I was having trouble with this part anyways: If I make intersection const, all iterators I can get are const-iterators. The function is supposed to return a Fork struct with iterators to the beginning and end of the fork end those iterators cannot be const-iterators cause later on I want to modify their instructions. The only solution I got working was to make the intersection non-const, thus resulting in that code.

you have any idea how to fix that?

daniel-j-h · 2016-12-16T16:38:41Z

src/extractor/guidance/turn_handler.cpp

+    return std::all_of(fork.right, fork.left + 1, [&](ConnectedRoad &base) {
+        const auto base_class = node_based_graph.GetEdgeData(base.eid).road_classification;
+        // check that there is no turn obvious == check that all turns are non-onvious
+        return std::all_of(fork.right, fork.left + 1, [&](ConnectedRoad &compare) {


Does the second loop have to exclude the road from the first road? Otherwise you're doing a self-comparison?

if it's a self comparison, the inner loop will evaluate return [...] || compare.eid == base.eid;

daniel-j-h · 2016-12-16T16:38:57Z

src/extractor/guidance/turn_handler.cpp

+        // makes sure that the fork is isolated from other neighbouring streets on the left and
+        // right side
+        const auto next =
+            (fork->left + 1) == intersection.end() ? intersection.begin() : (fork->left + 1);


+1 safe to do?

it's safe but will also be fixed by #3457

daniel-j-h · 2016-12-16T16:39:01Z

src/extractor/guidance/turn_handler.cpp

-            right > 0 &&
-            angularDeviation(intersection[right].angle, intersection[right - 1].angle) >=
-                GROUP_ANGLE;
+            angularDeviation(fork->right->angle, (fork->right - 1)->angle) >= GROUP_ANGLE;


… forks

chaupow added the Work In Progress label Nov 8, 2016

chaupow force-pushed the refactor/guidance_fork_handling branch from 5287c76 to 58537a1 Compare November 8, 2016 15:46

chaupow changed the title ~~Refactor/guidance fork handling~~ Refactor fork handling in guidance Nov 8, 2016

daniel-j-h requested changes Nov 8, 2016

View reviewed changes

daniel-j-h reviewed Nov 8, 2016

View reviewed changes

MoKob reviewed Nov 9, 2016

View reviewed changes

chaupow force-pushed the refactor/guidance_fork_handling branch from 58537a1 to 6660a73 Compare November 14, 2016 15:48

daniel-j-h requested changes Nov 15, 2016

View reviewed changes

chaupow force-pushed the refactor/guidance_fork_handling branch from daf26ee to 908809b Compare December 9, 2016 13:47

chaupow added Review Work In Progress and removed Work In Progress Review labels Dec 9, 2016

chaupow force-pushed the refactor/guidance_fork_handling branch from 908809b to 5905851 Compare December 9, 2016 16:36

chaupow added Review Bicycle and removed Work In Progress Review Bicycle labels Dec 9, 2016

chaupow requested a review from daniel-j-h December 9, 2016 16:44

chaupow requested a review from MoKob December 9, 2016 16:44

daniel-j-h requested changes Dec 9, 2016

View reviewed changes

MoKob mentioned this pull request Dec 12, 2016

fix invalid assertion in coordinate_extractor #3434

Merged

2 tasks

MoKob suggested changes Dec 12, 2016

View reviewed changes

chaupow force-pushed the refactor/guidance_fork_handling branch 2 times, most recently from aaf2397 to 32dcbb5 Compare December 15, 2016 14:57

chaupow mentioned this pull request Dec 15, 2016

Refactor struct Fork #3457

Closed

chaupow force-pushed the refactor/guidance_fork_handling branch from 32dcbb5 to 138616d Compare December 15, 2016 15:06

MoKob approved these changes Dec 15, 2016

View reviewed changes

chaupow force-pushed the refactor/guidance_fork_handling branch from 138616d to 88a568d Compare December 15, 2016 15:49

daniel-j-h requested changes Dec 16, 2016

View reviewed changes

chaupow force-pushed the refactor/guidance_fork_handling branch from 88a568d to d462ec2 Compare January 5, 2017 16:15

chaupow added 3 commits January 5, 2017 17:55

refactor findFork in turn_hander.cpp

e6d3385

refactor findFork return value

e25d9f6

use iterators instead of indices when referring to connected roads in…

635a485

… forks

chaupow force-pushed the refactor/guidance_fork_handling branch from d462ec2 to e69c905 Compare January 5, 2017 17:06

refactor fork handler

696bfdb

chaupow force-pushed the refactor/guidance_fork_handling branch from e69c905 to 696bfdb Compare January 6, 2017 12:47

daniel-j-h approved these changes Jan 6, 2017

View reviewed changes

chaupow merged commit f313cb9 into master Jan 7, 2017

chaupow deleted the refactor/guidance_fork_handling branch January 9, 2017 16:44

MoKob mentioned this pull request Jan 11, 2017

fix forks exiting a ferry, resolves #3536 #3555

Merged

3 tasks


		return all_of(first, last + 1, [&](const ConnectedRoad &road) { return road.entry_allowed; });
		}

Refactor fork handling in guidance #3264

Refactor fork handling in guidance #3264

Conversation

chaupow commented Nov 8, 2016 • edited Loading

Issue

Tasklist

Tasklist unrelated to this PR (but arose during this work)

Code Review Todolist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-j-h commented Nov 8, 2016 • edited by chaupow Loading

MoKob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaupow commented Nov 9, 2016

MoKob commented Nov 9, 2016

TheMarex commented Nov 12, 2016

chaupow commented Nov 15, 2016

daniel-j-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoKob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-j-h left a comment

chaupow commented Nov 8, 2016 •

edited

Loading

daniel-j-h commented Nov 8, 2016 •

edited by chaupow

Loading