-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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.