make some redundant proto messages/enums common #3452
Conversation
|
Nice stuff, thanks! IMO, maybe this is the time to make the switch? Couldn’t really find much related, but at some point proto2 won’t be supported anymore I guess, language bindings, and core code.. some PRs from 2017 are already talking about maintenance mode. Only fear I’d have.. with the features you’re planning, weening off proto2 sometime in the future would be a whole lot more painful than doing it right now I think.. though still shitload of work😅 I have some time on my hands these days, yay quarantine!, and if it’s any help I’d be willing to support for such a noble goal:) |
| @@ -13,61 +13,6 @@ message DirectionsLeg { | |||
| optional bool has_time_restrictions = 4; // Does the route contain any time restrictions? | |||
| } | |||
|
|
|||
| message TransitInfo { | |||
There was a problem hiding this comment.
these things all existed also in the trip proto, so i moved them all to tripcommon. this changes the namespace they are located within of course so the rest of the code changes were all about that
| @@ -1,100 +0,0 @@ | |||
| syntax = "proto2"; | |||
There was a problem hiding this comment.
i think its safe to say we will never use this. @dgearhart do you disagree?
| @@ -73,62 +73,6 @@ message TripLeg { | |||
| kImpassable = 7; | |||
| } | |||
|
|
|||
| enum TravelMode { | |||
There was a problem hiding this comment.
duplicated moved to common
| @@ -21,46 +21,6 @@ constexpr auto kMinEdgeLength = 0.001f; | |||
| namespace valhalla { | |||
| namespace odin { | |||
|
|
|||
| const std::unordered_map<int, DirectionsLeg_VehicleType> translate_vehicle_type{ | |||
There was a problem hiding this comment.
these arent needed because now directions and trip share the same definition of these things
| @@ -410,7 +370,7 @@ void DirectionsBuilder::PopulateDirectionsLeg(const Options& options, | |||
| } | |||
|
|
|||
| // Travel mode | |||
| trip_maneuver->set_travel_mode(translate_travel_mode.find(maneuver.travel_mode())->second); | |||
There was a problem hiding this comment.
everything else from here down is renaming stuff since i moved them out of directions and trip and into common
| @@ -20,6 +20,8 @@ enum class TravelMode : uint8_t { | |||
| kPublicTransit = 3, | |||
| kMaxTravelMode = 4 | |||
| }; | |||
| // To avoid collisions with pbf version | |||
| using travel_mode_t = TravelMode; | |||
There was a problem hiding this comment.
in thor we make use of sif::TravelMode a lot but since i moved the common stuff in proto up to the valhalla namespace we have to tell the code in thor its refering to the sif one not the proto one. to make this easier i made a type alias. probably we should get rid of the non protoversions of some of this stuff too to be honest but ill focus on one thing at a time 😄
Ok so this is the first step in my quest to make pbf in/output from the http api a reality.
At the very beginning of this process my idea was that I would convert all of our internal protos to proto3 (which is recommended by grpc) and then eventually be able to have a grpc service as well. I've since backed off of this goal a bit after the absolutely insane amount of work it was to move to proto3 (see here: master...proto3). Proto3 originally did not have a way to tell whether a scalar field was set or unset. You can get around it by using
oneofthough so I started implementing that. In addition to that, you cannot set default values so we would need to take the defaults we do have (that arent0) and move those to code. By themselves these two things do not seem insurmountable (and they arent) but after making a lot of the changes i decided it was too much for one go.So what im starting with here is just lining up the structures so that they are somewhat organized. the main thing that i saw was that we had some redundancies between directions and trip and so i moved those over to tripcommon so they could be shared. i've also deleted the directions_next proto as we will likely never do it.
speaking of that who has opinions on proto2 vs proto3? once we expose proto2 as an interface i feel like it will be difficult or possibly impossible to go to proto3 (since we'd need to use the
oneoftrick to get backoptionalfields, though maybe they are binary compatible and so it wouldnt matter?).