stadiamaps / ferrostar

A FOSS navigation SDK built from the ground up for the future
https://stadiamaps.github.io/ferrostar/
Other
91 stars 10 forks source link

Introduce Valhalla costing options #104

Closed Max-Leopold closed 1 month ago

Max-Leopold commented 1 month ago

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.

ianthetechie commented 1 month ago

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.

ianthetechie commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.