Add support for closure annotations #2816

mandeepsandhu · 2021-01-28T01:21:56Z

Adds a new shape attribute filter for getting closure locations in the
directions response. Similar to incidents, each closure annotation has
a start & end geometry index, which indexes into the coordinates list.

purew · 2021-02-18T19:25:09Z

src/thor/triplegbuilder.cc

+        // is still within an existing closure)
+        ::valhalla::TripLeg_Closure* closure = fetch_or_create_closure_annotation(leg);
+        if (!closure->has_begin_shape_index()) {
+          closure->set_begin_shape_index(i - 1);


How come it's i - 1 and not just i?

Since this loops starts from the 2nd shape index. It does that since we create cuts only if speed changes in subsequent shape segments.

purew · 2021-02-18T19:30:55Z

test/gurka/test_closure_annotations.cc

+
+namespace {
+
+inline void SetLiveSpeedFrom(baldr::TrafficSpeed* live_speed, uint64_t speed, uint8_t subsegment) {


Any chance subsegment argument can be renamed to breakpoint1 just to decrease number of terms describing the same thing?

Also, you're not setting the speed of the first subsegment here even though you do set the breakpoint1, is that intended? You may want to set speed1 to either unknown or 0 (or something else) for clarity

Any chance subsegment argument can be renamed to breakpoint1 just to decrease number of terms describing the same thing?

Yeah, I can change subsegment to breakpoint1 (this was just me trying to reverse engg the TrafficSpeed struct and trying to make sense of what breakpoint meant 😛 ).

Also, you're not setting the speed of the first subsegment here even though you do set the breakpoint1, is that intended? You may want to set speed1 to either unknown or 0 (or something else) for clarity

So this function intends to set live speed to 0 starting from a particular subsegment/breakpoint of the edge. Anything before that should remain whatever value it previously had. TBH I arrived at the vlaues being set after a few iterations. Let me know if there's a better/correct/simpler way to do it. I want to partially close edges upto/from a certain percentage of the edge.

purew · 2021-02-18T19:31:31Z

test/gurka/test_closure_annotations.cc

+  live_speed->speed3 = UNKNOWN_TRAFFIC_SPEED_RAW;
+}
+
+inline void SetLiveSpeedUpto(baldr::TrafficSpeed* live_speed, uint64_t speed, uint8_t subsegment) {


same note about name here

The speed datatype should probably be uint8_t too

How about overall_speed?

SetLiveSpeed below can probably be collapsed into this function if you set subsegment/breakpoint1 argument to default to 255?

I'm not sure what overall_speed should be set to if its already populated (I do that when constructing the gurka map in the setup function) and we want to close only part of the edge. Thats why I left it as is, and also why I have a separate function for setting speed for the entire edge as it sets the overall_speed too.

The most important is probably to set overall_speed to 0 if any of the subsegments have zero speed. That marks the edge as closed if a subsegment is closed. Apart from that, it's not used if you have any unknown subsegment.

purew · 2021-02-18T22:56:37Z

test/gurka/test_closure_annotations.cc

+  }
+}
+
+void close_partial_dir_edge_upto(baldr::GraphReader& reader,


Just an FYI:

There are some utility functions already for customizing live traffic like test::customize_live_traffic_data(map.config, cb_setter_max); as used in https://github.com/valhalla/valhalla/blob/master/test/gurka/test_traffic.cc#L56

Ohh I see that you are in fact using that below.

yup 🙂

I added these functions here since I wanted partial closures. If this is reused elsewhere in the future, we could move it to the gurka module itself.

purew · 2021-02-18T22:58:58Z

test/gurka/test_closure_annotations.cc

@@ -0,0 +1,327 @@
+#include "gurka.h"


Could we apply more compiler warnings to this file at https://github.com/valhalla/valhalla/blob/master/test/gurka/CMakeLists.txt#L14 ?

totally! will add it

Adds a new shape attribute filter for getting closure locations in the directions response. Similar to incidents, each closure annotation has a start & end geometry index, which indexes into the coordinates list.

mandeepsandhu changed the title ~~Add support for closure annotations~~ DNM: Add support for closure annotations Jan 28, 2021

mandeepsandhu marked this pull request as draft January 28, 2021 01:22

mandeepsandhu changed the title ~~DNM: Add support for closure annotations~~ Add support for closure annotations Jan 28, 2021

mandeepsandhu requested review from kevinkreiser, danpat and purew January 28, 2021 07:58

mandeepsandhu marked this pull request as ready for review January 28, 2021 08:01

mandeepsandhu force-pushed the closure_annotations branch from 7082239 to 55278a9 Compare January 28, 2021 08:01

mandeepsandhu force-pushed the closure_annotations branch from 55278a9 to fff6c51 Compare February 18, 2021 01:42

purew reviewed Feb 18, 2021

View reviewed changes

mandeepsandhu force-pushed the closure_annotations branch from a1846b1 to daef98d Compare February 22, 2021 22:39

purew previously approved these changes Feb 23, 2021

View reviewed changes

mandeepsandhu dismissed purew’s stale review via 47bd9ec February 23, 2021 21:12

mandeepsandhu added 7 commits February 23, 2021 13:12

Add support for closure annotations

ace168d

Adds a new shape attribute filter for getting closure locations in the directions response. Similar to incidents, each closure annotation has a start & end geometry index, which indexes into the coordinates list.

set closure in fwd direction

faee2ff

Add tests for closure annotations

be0fafd

update changelog

3947bff

Address review comments

e5c3f6d

lint fix

64e8824

remove debug function

845223e

mandeepsandhu force-pushed the closure_annotations branch from 47bd9ec to 845223e Compare February 23, 2021 21:12

mandeepsandhu requested a review from purew February 23, 2021 21:12

purew approved these changes Feb 23, 2021

View reviewed changes

mandeepsandhu merged commit 1ff1856 into master Feb 23, 2021

purew deleted the closure_annotations branch February 24, 2021 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for closure annotations #2816

Add support for closure annotations #2816

mandeepsandhu commented Jan 28, 2021

purew Feb 18, 2021

mandeepsandhu Feb 19, 2021

purew Feb 18, 2021

mandeepsandhu Feb 22, 2021

purew Feb 18, 2021

purew Feb 18, 2021

purew Feb 18, 2021

purew Feb 18, 2021

mandeepsandhu Feb 22, 2021 •

edited

Loading

purew Feb 23, 2021 •

edited

Loading

purew Feb 18, 2021 •

edited

Loading

purew Feb 18, 2021

mandeepsandhu Feb 22, 2021

purew Feb 18, 2021

mandeepsandhu Feb 22, 2021


		namespace {

		inline void SetLiveSpeedFrom(baldr::TrafficSpeed* live_speed, uint64_t speed, uint8_t subsegment) {

Add support for closure annotations #2816

Add support for closure annotations #2816

Conversation

mandeepsandhu commented Jan 28, 2021

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

mandeepsandhu Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

purew Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

purew Feb 18, 2021 • edited Loading

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

mandeepsandhu Feb 22, 2021 •

edited

Loading

purew Feb 23, 2021 •

edited

Loading

purew Feb 18, 2021 •

edited

Loading