-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Valhalla costing options #104
Introduce Valhalla costing options #104
Conversation
Thanks for this PR @Max-Leopold! This is actually something that's been on my list of things to do for a while. I'll give a bit more thought to this tomorrow. The wall that I hit previously (which made me punt on it till later) is that the values can actually be more than strings, and I wasn't sure the best way to surface this in a way that crosses the FFI boundary well (JSON doesn't actually transfer directly). Here's a stream of consciousness brain dump of the approaches I've considered so far:
The last one might actually be the last practical in the interest of moving quickly... curious if you have any opinions. |
We had a discussion about this on our weekly call with the core devs and we're all leaning toward the last option. I'll give it a pass (probably this weekend) to see if it works out later and, if it works out, will make the modifications on your branch. |
Hey @ianthetechie, sorry for only answering now. Thanks for having a look.
Using a I won't have much time to look into this further over the next couple of days so please feel free to poke around. Otherwise I might pick this up again end of next week. |
I just had a bit of time in my lunch break so did a first try of using a JSON string to pass FFI instead of the String map. Let me know if this is what you had in mind. As I said, I won't have time the coming days so please feel free to modify the PR. |
Wow, thanks @Max-Leopold! Yes, this is what I had in mind! I'll review properly over the weekend to make sure I haven't missed anything, but I'm quite optimistic about getting this merged quickly :) And also quite happy to have the feature for my own app, as I have recently realized that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I got a chance to try this out a bit more deeply and am cleaning a few things up. Leaving a note here for posterity to explain what changed and why (aside from minor cleanup, docstrings, and the like).
- Adding some tests to the platform layer; this wasn't obvious, and the existing unit tests were pretty OK but I added a few platform-level sanity tests.
- Making the costing options an optional input at the Rust layer via a convenience constructor (users shouldn't have to guess what to put as a safe/sentinel value when they want the defaults).
- Moved the parsing and failure potential to init time in the core. This is faster (don't need to re-parse the same JSON string on subsequent requests; ex: re-routing) and communicates failure immediately rather than at route request time (which is somewhat unexpected).
- Surfaced an error type for these general "instantiation errors" for lack of a better term. Can refine later.
- Switched Kotlin from kotlinx serialization to moshi. When writing integration tests for the platform layer, I realized that kotlinx serialization doesn't work with
Any
😂 So, we have no choice but to use another library that deals with the dynamism of JSON a bit better.
I was looking into how to add some custom costing options for Valhalla routing and noticed a TODO comment to add more tunable parameters so I decided to give it a shot.
I'm a bit rusty in Rust and normally don't work with Kotlin so please excuse any obvious errors.