Adding further engine bindings #6
Conversation
|
I've attached the MatchParameters file as reference here, but the gist of it is that I can't find the proper parameter combination to have it link. It seems to not like that waypoints is explicitly specified in MatchParameters, while it's also handled in RouteParameters, so it expects the waypoints parameters again, which also throws off the va starting point for BaseParameters. |
|
Thanks for the further work! It's quite a lot to review though;) It's ok for now, and let's merge once reviewed and got your small problem out of the way. After that, we should concentrate on testing IMO, which will also allow us to see how the API currently works for a user (it's hard to understand just looking at the code). |
src/parameters/matchparameter_nb.cpp
Outdated
| std::vector<std::string> exclude, | ||
| const BaseParameters::SnappingType snapping | ||
| ) { | ||
| new (t) MatchParameters( |
There was a problem hiding this comment.
I've done the MatchParameters in the "assigning" way, but I'm not sure if it would be clearer to just fully assign all values manually, or use the constructor for MatchParameters and RouteParameters, and then manually assign for BaseParameters.
I believe the NodeJS bindings do it in the inverse way instead (ie. assigning BaseParameters, and then everything else is explicitly assigned), but I think that would be more complicated to replicate here.
There was a problem hiding this comment.
but I'm not sure if it would be clearer to just fully assign all values manually
Yeah I agree here
There was a problem hiding this comment.
To clarify, do you mean that you agree with manually assigning all values over "hybrid" instantiation?
There was a problem hiding this comment.
you agree with manually assigning all values
exactly that:)
There was a problem hiding this comment.
you want to integrate that change before merging or make an issue out of it?
There was a problem hiding this comment.
The part that I didn't fully understand about the NodeJS annotations_type part there is that it seems to check what enum type was sent for annotations to assign the annotations_type value (in an array), but for the C++ side of things, it seems to base it off which RouteParameters constructor is called. If I'm understanding this part correctly, the user is able to send in an array of AnnotationsTypes, which gets calculated up? In that case, it would make sense to apply that same abstraction to the Python bindings.
Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side ["nodes", "distance", "speed"] to smth like this on C++ side std::vector<AnnotationType>{AnnotationType::Nodes, AnnotationType::Distance, AnnotationType::Speed}. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.
There was a problem hiding this comment.
The part that I didn't fully understand about the NodeJS annotations_type part there is that it seems to check what enum type was sent for annotations to assign the annotations_type value (in an array), but for the C++ side of things, it seems to base it off which RouteParameters constructor is called. If I'm understanding this part correctly, the user is able to send in an array of AnnotationsTypes, which gets calculated up? In that case, it would make sense to apply that same abstraction to the Python bindings.
Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side
["nodes", "distance", "speed"]to smth like this on C++ sidestd::vector<AnnotationType>{AnnotationType::Nodes, AnnotationType::Distance, AnnotationType::Speed}. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.
It's definitely possible, but my confusion stemmed from the C++ side of things not accepting a vector of AnnotationsType - only an AnnotationsType enum. On the C++ end, is the user expected to perform that calculation prior to passing it in? Otherwise I can create an abstraction for that, like on the NodeJS bindings
There was a problem hiding this comment.
ahhh, I got it. annotations_type is not vector indeed, it is bitmask. Then you indeed can follow NodeJS path and just transform Python's list to this bitmask just like NodeJS bindings do.
So I would rephrase what I said above:
Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side
["nodes", "distance", "speed"]to smth like this on C++ sideAnnotationType::Nodes | AnnotationType::Distance | AnnotationType::Speed. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.
There was a problem hiding this comment.
Okay, that sounds doable, thanks for the clarifications!
There was a problem hiding this comment.
I've made adjustments to accommodate the requested changes. In the process, I've also discovered a bug in osrm-backend's RouteParameter's AnnotationsType |= operator overload that renders it nonfunctional, so I've also put in a PR over there to address that.
Yes, it works both ways, and basic benchmarking seemed to show negligible difference between the two methods, so I went with the return visitor for code clarity. On the topic of mapbox recursive types, it seems it works without an explicit |
|
LGTM, thanks! |
Added more bindings for the other engine calls. However, I did run into an issue with the MatchParameters bindings, which I suspect has something to do with the way the argument forwarding is set up.
Also added
__repr__outputs for the custom JSON types, so they print out as expected.