gis-ops / valhalla-app

This is the demo web app running on https://valhalla.openstreetmap.de
https://valhalla.openstreetmap.de
MIT License
164 stars 90 forks source link

Fix: reset settings when switching profile on isochrone #202

Closed jthiard closed 10 months ago

jthiard commented 10 months ago

Hi, here a little fix for a bug I found recently.

When switching profile on Directions tabs, all settings are reset. This is not the case in Isochrones tabs, which can be an issue when two profiles share some settings (eg car and truck profiles).

👨‍💻 Changes proposed

Reset all settings when switching profile in Isochrones, exactly as in Directions

📷 Screenshots

Calling an Isochrone with the car profile. width is the car default (1.6m).

Screenshot_2023-11-10_10-34-36

Switching to the truck profile. width is still the car default (1.6m) instead of the truck default (2.6m).

Screenshot_2023-11-10_10-34-51

chrstnbwnkl commented 10 months ago

Thanks @jthiard for bringing this up! I'm not entirely sure if this is what we want though. There are a lot of costing options and sometimes it can be quite some work to adjust these exactly for your needs. So when you change the profile, they all disappear. Maybe we actually would like it the other way around (don't reset on profile switch). @nilsnolde what do you think? Bug or Feature?

jthiard commented 10 months ago

Hi @chrstnbwnkl sure, the bug was inconsistency between directions and isochrones, but the proposed fix may not be the right one.

For what it's worth, the problem I see now when not resetting settings is that someone :

  1. go to the car profile
  2. don't change any setting from its default value
  3. switch to the truck profile
  4. have now a truck with a 1.6m width and a 1.9m height which is a very very small truck. And probably have no idea about this.

Which is obviously very confusing and error prone.

I understand the problem with "resetting all settings when changing profile", but fixing it looks like a lightly more complicated feature. I think we may at least need:

Not sure I have the time for this in the next few days, but we'll be happy to work on it later if you find worth it.

chrstnbwnkl commented 10 months ago

Thanks for the clarification, that makes sense (wasn't aware that not resetting could lead to changed defaults which is obviously misleading). I think then this is good to merge for now, and we would be happy to accept a PR implementing a more sensitive solution that allows the user to retain changed values across profiles. Another alternative would be – instead of carrying over values across profiles – to simply not discard changed costing options when profiles are changed. That means common options like "use_hills" would not be carried over, but when the user adjusts it in the pedestrian profile, switches to cycling, and then switches back to ped, that initially changed value would still be there (and "use_hills" in ped and cycling would be decoupled from each other). But maybe we can have this discussion in a separate issue instead.

Still would like to get @nilsnolde|s 2 cents before final approval

jthiard commented 10 months ago

That means common options like "use_hills" would not be carried over, but when the user adjusts it in the pedestrian profile, switches to cycling, and then switches back to ped, that initially changed value would still be there (and "use_hills" in ped and cycling would be decoupled from each other).

Yeah that sounds very good to me. One set of settings values per profile. You will then be able to fine-tune a profile, don't loose your config when switching, and won't be confused by bad default values.

But maybe we can have this discussion in a separate issue instead.

Sure.

nilsnolde commented 10 months ago

Yeah, what @jthiard is describing was an issue before and is why we went the way of simply resetting to profile defaults when switching. And apparently that's only the case for routing, not isochrones..

Also agree that there could be a more elaborate way of handling things. Though I'm not sure I fully understand what you're proposing @chrstnbwnkl . From what I understand you'd keep track of the state of each option per profile? i.e. user changes option A in ped to 0.1, switching to bike you'd get the bike's default for option A, but going back to ped you'd get 0.1 again? Hm, not so sure.

IMO both approaches are more or less equally good (and bad). For things like maneuver_penalty or border_penalty and quite a few others, it's more comfortable/convenient if the changed values carry over when changing profiles (@jthiard initial suggestion). But options like height (all dimenstions), top_speed and quite a few others are pretty pointless to carry over (@chrstnbwnkl suggestion), no matter if the user changed them or not.

Of course we could add more complexity by allowing us to decide on a per option basis if we'd like to use one approach over the other, basically implementing both. But meeh, that's also confusing for a user, which option resets not matter what and which doesn't.

TLDR: I don't really care which way we go haha maybe @jthiard is a little bit less code, but maybe not. Someone else needs to decide that, I'm literally 50:50 :D For now I'll merge here, it's still good to be consistent, even it's not ideal yet:) thanks @jthiard !