Skip to content
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

Merged

Conversation

Max-Leopold
Copy link
Contributor

@Max-Leopold Max-Leopold commented May 5, 2024

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.

@Max-Leopold Max-Leopold marked this pull request as draft May 5, 2024 19:42
@Max-Leopold Max-Leopold marked this pull request as ready for review May 5, 2024 19:45
@ianthetechie ianthetechie self-requested a review May 6, 2024 08:30
@ianthetechie
Copy link
Contributor

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:

  • It's actually possible that Valhalla gracefully converts strings to a relevant type... I'll have to check into that; not super hopeful TBH, but if so, then your PR looks like it'd probably do the job!
  • Punt on the arbitrary options for now and just add the parameters you need. This is basically what we're doing so far, but it doesn't live up to my aspirations for extensibility.
  • Swinging the other direction, we have an OpenAPI spec for costing options in here: https://api.stadiamaps.com/openapi.yaml. This would give a really nice strongly typed interface for all standard parameters.
  • Some hybrid builder pattern... Internally we can represent this as a JSON object. But maybe we need to expose a few different methods to set "key paths."
  • Allow more dynamic versions like [String: Any] at the platform layer (Swift / Kotlin) and use JSON strings to cross the FFI boundary (need to think about communicating failure in these cases though).

The last one might actually be the last practical in the interest of moving quickly... curious if you have any opinions.

@ianthetechie
Copy link
Contributor

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.

@Max-Leopold
Copy link
Contributor Author

Hey @ianthetechie, sorry for only answering now. Thanks for having a look.
I had a similar thought process to you and my hope was that Valhalla would just convert the strings to the appropriate type. Thats why I just implemented it as a String: String map. However, I never actually tested that hypothesis 🤦‍♂️

Allow more dynamic versions like [String: Any] at the platform layer (Swift / Kotlin) and use JSON strings to cross the FFI boundary (need to think about communicating failure in these cases though).

Using a String: Any was what I wanted to do initially but I wasn't sure how to represent the Any type in Rust. Using a JSON string to pass the map from Swift/Kotlin to Rust makes sense. I agree that this sounds like a sensible approach 👍

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.

@Max-Leopold
Copy link
Contributor Author

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.

@ianthetechie
Copy link
Contributor

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 AVSpeechSynthesizer + Valhalla voice instructions as they are today do NOT do well with mixed-language results, so I'd rather just change the language on my requests haha.

Copy link
Contributor

@ianthetechie ianthetechie left a 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).

  1. 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.
  2. 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).
  3. 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).
  4. Surfaced an error type for these general "instantiation errors" for lack of a better term. Can refine later.
  5. 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.

@ianthetechie ianthetechie merged commit c730df7 into stadiamaps:main May 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants