The events/incidents in intersections #2547

purew · 2020-08-20T14:05:43Z

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

Requirements / Relations

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

…-intersections

src/thor/triplegbuilder.cc

src/tyr/route_serializer_osrm.cc

valhalla/proto_conversions.h

src/thor/triplegbuilder_utils.cc

and 'edge.show_incidents' property

src/tyr/route_serializer_osrm.cc

kevinkreiser · 2020-08-22T01:35:33Z

proto/trip.proto

@@ -226,6 +226,30 @@ message TripLeg {
    optional float default_speed = 44;       // km/h
    optional Restriction restriction = 45;
    optional bool destination_only = 46;
+
+    message Incident {


i know im a late comer here but if the incident is to happen at the node, shouldnt we put this on the node rather than nest it on the edge?

or is the idea that since the incident will cover the whole edge (because we cut the edge wherever the incidents occur both at the beginning and end of the incident that we dont need to put it on the node? what if the incident isnt linear but is a point? how do we really understand the full spatial extent of the incident. i feel like we are missing just a little bit of structure here

Yep, node makes more sense, moving it there!

danpaz · 2020-08-24T15:49:50Z

valhalla/thor/attributes_controller.h

@@ -69,6 +69,7 @@ const std::string kEdgeTruckSpeed = "edge.truck_speed";
 const std::string kEdgeTruckRoute = "edge.truck_route";
 const std::string kEdgeDefaultSpeed = "edge.default_speed";
 const std::string kEdgeDestinationOnly = "edge.destination_only";
+const std::string kEdgeShowIncidents = "edge.show_incidents";


I guess this will become node.incidents?

…-intersections

mandeepsandhu · 2020-08-24T21:20:34Z

src/odin/enhancedtrippath.cc

@@ -1760,6 +1760,16 @@ std::string EnhancedTripLeg_Admin::ToString() const {

  return str;
 }
+google::protobuf::RepeatedPtrField<valhalla::TripLeg_Node_Incident> empty;


Should this made const since we're using it as one?

mandeepsandhu · 2020-08-24T21:27:49Z

src/proto_conversions.cc

+  std::string incident_type;
+  switch (incident.type()) {
+    case TripLeg_Node_Incident_Type_ACCIDENT:
+      incident_type = "accident";


Maybe we can avoid a copy by returning directly from the case statements? (not sure if the compiler can do away with the string copy being created here, maybe it can?)

Not sure either. I'd assume the compiler could optimize it, but I can make it a direct return.

mandeepsandhu · 2020-08-24T21:52:46Z

src/tyr/route_serializer_osrm.cc

+                boost::get<std::shared_ptr<valhalla::baldr::json::Jarray>>(&existing->second)) {
+          serialized_incidents = *ptr;
+        } else {
+          throw std::logic_error("This should not be possible");


Can we put a bit more information into the error message? Like "could not get existing incidents" or some such.

mandeepsandhu · 2020-08-24T21:54:12Z

src/tyr/route_serializer_osrm.cc

+  }
+  json::ArrayPtr serialized_incidents = std::shared_ptr<json::Jarray>(new json::Jarray());
+  for (const auto& incident : incidents) {
+    {


Whats the purpose of this additional scope?

Keeping the local context small on the temporary variables used for bringing out any existing array in the json doc.

mandeepsandhu · 2020-08-24T21:56:43Z

src/tyr/route_serializer_osrm.cc

+  for (const auto& incident : incidents) {
+    {
+      // Bring up any already existing array
+      auto existing = doc.find("incidents");


We don't need this inside the loop, right?

Correct, I thought I moved this out in a previous version of the code. Anyway, moving out of the loop.

…-intersections

src/odin/enhancedtrippath.cc

kevinkreiser · 2020-08-25T13:50:18Z

src/proto_conversions.cc

+      return "weather";
+      break;
+  };
+  return "Unhandled case";


kevinkreiser · 2020-08-25T13:51:53Z

src/tyr/route_serializer_osrm.cc

+
+  {
+    uint64_t id = static_cast<uint32_t>(incident.id());
+    metadata_json->emplace("id", id);


why not cast inline like the others below?

I had a really long, opaque compiler error which didn't resolve until I did this. In hindsight, it seemed like the json document wouldn't allow both uint64 and uint32 inside.

With the increase of id to 64bits, this entire cast goes away.

kevinkreiser · 2020-08-25T14:03:51Z

proto/trip.proto

+      optional uint64 start_time = 3;     // START_TIME as seconds since epoch UTC
+      optional uint64 end_time = 4;       // END_TIME seconds since epoch UTC
+      optional uint64 creation_time = 5;  // time the incident was created/updated in seconds since epoch UTC
+      optional uint32 id = 6;


we might want to make the id some sort of a unique-ish information carrier for the important bits of the incident:

incident_id { uint64_t start_lon: 26; //int(lon * 100000) uint64_t start_lat : 26; //int(lat * 100000) uint64_t type: 4; uint64_t time: 8; //epoch second modulo 255 }

I like this idea. I'll implement this in the follow-on PR where the real incident data is implemented.

to be able to decide if type is set or not

purew added 3 commits August 19, 2020 10:38

Adds incidents to intersection-object

7f83233

Merge remote-tracking branch 'origin/master' into anders-incidents-in…

262a581

…-intersections

Removes mocking code for now

ffbd5b7

purew requested a review from danpaz August 20, 2020 14:05

danpaz reviewed Aug 20, 2020

View reviewed changes

src/thor/triplegbuilder.cc Outdated Show resolved Hide resolved

purew added 2 commits August 20, 2020 10:19

Missing triplegbuilder_utils file

fe19a33

Makes incidents a request parameter

edbf501

danpaz reviewed Aug 20, 2020

View reviewed changes

src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved

danpaz reviewed Aug 20, 2020

View reviewed changes

src/tyr/route_serializer_osrm.cc Show resolved Hide resolved

danpaz reviewed Aug 20, 2020

View reviewed changes

src/tyr/route_serializer_osrm.cc Outdated Show resolved Hide resolved

danpaz reviewed Aug 20, 2020

View reviewed changes

valhalla/proto_conversions.h Outdated Show resolved Hide resolved

danpaz reviewed Aug 20, 2020

View reviewed changes

src/thor/triplegbuilder_utils.cc Outdated Show resolved Hide resolved

purew added 5 commits August 20, 2020 14:35

Enables incidents in routeline with AttributesController

27055ed

and 'edge.show_incidents' property

nit: Removes unused option

ad75b2c

Dont insert over and over

0a2aa85

Only serialize incident properties that are set

3f8e46b

Fix test for previous commit

35b6611

purew added this to the Sprint 17 - Left Turn milestone Aug 20, 2020

purew self-assigned this Aug 20, 2020

Replace border_crossing with country_code and state_code

f7c93f0

danpaz reviewed Aug 21, 2020

View reviewed changes

src/tyr/route_serializer_osrm.cc Show resolved Hide resolved

danpaz added 3 commits August 21, 2020 16:54

Merge branch 'master' into anders-incidents-in-intersections

d751161

Remove unrelated intersections properties

8343951

Update changelog

3a865c9

danpaz previously approved these changes Aug 21, 2020

View reviewed changes

kevinkreiser reviewed Aug 22, 2020

View reviewed changes

danpaz reviewed Aug 24, 2020

View reviewed changes

Moves incidents to Node instead of Node.Edge

405e387

purew dismissed danpaz’s stale review via 405e387 August 24, 2020 18:28

Merge remote-tracking branch 'origin/master' into anders-incidents-in…

19979b2

…-intersections

mandeepsandhu reviewed Aug 24, 2020

View reviewed changes

purew added 2 commits August 25, 2020 09:11

PR Feedback

1f9e9e4

Merge remote-tracking branch 'origin/master' into anders-incidents-in…

7dfd278

…-intersections

danpaz previously approved these changes Aug 25, 2020

View reviewed changes

kevinkreiser reviewed Aug 25, 2020

View reviewed changes

src/odin/enhancedtrippath.cc Show resolved Hide resolved

kevinkreiser reviewed Aug 25, 2020

View reviewed changes

64bit incident id

08ed828

purew dismissed danpaz’s stale review via 08ed828 August 25, 2020 14:07

kevinkreiser previously approved these changes Aug 25, 2020

View reviewed changes

Adds incident_type UN_SET

6f67eb3

to be able to decide if type is set or not

purew dismissed kevinkreiser’s stale review via 6f67eb3 August 25, 2020 14:26

kevinkreiser approved these changes Aug 25, 2020

View reviewed changes

purew merged commit 791cc24 into master Aug 25, 2020

purew deleted the anders-incidents-in-intersections branch August 25, 2020 16:06

danpaz added a commit that referenced this pull request Aug 26, 2020

Remove intersection properties unintentionally added in #2547

a158fe9

danpaz mentioned this pull request Aug 26, 2020

Remove intersection properties unintentionally added in #2547 #2560

Merged

kevinkreiser pushed a commit that referenced this pull request Aug 26, 2020

Remove intersection properties unintentionally added in #2547 (#2560)

9167d2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The events/incidents in intersections #2547

The events/incidents in intersections #2547

purew commented Aug 20, 2020

kevinkreiser Aug 22, 2020

kevinkreiser Aug 22, 2020

purew Aug 24, 2020

danpaz Aug 24, 2020

mandeepsandhu Aug 24, 2020

mandeepsandhu Aug 24, 2020

purew Aug 25, 2020

mandeepsandhu Aug 24, 2020

mandeepsandhu Aug 24, 2020

purew Aug 25, 2020

mandeepsandhu Aug 24, 2020

purew Aug 25, 2020

kevinkreiser Aug 25, 2020

kevinkreiser Aug 25, 2020

purew Aug 25, 2020

kevinkreiser Aug 25, 2020 •

edited

Loading

purew Aug 25, 2020

The events/incidents in intersections #2547

The events/incidents in intersections #2547

Conversation

purew commented Aug 20, 2020

Issue

Tasklist

Requirements / Relations

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

kevinkreiser Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser Aug 25, 2020 •

edited

Loading