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

Fix: reset settings when switching profile on isochrone #202

Merged
merged 1 commit into from
Nov 11, 2023

Conversation

jthiard
Copy link
Contributor

@jthiard jthiard commented Nov 10, 2023

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

same as when switching direction profile
@chrstnbwnkl
Copy link
Member

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
Copy link
Contributor Author

jthiard commented Nov 10, 2023

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:

  • know which settings have been changed from its default value, and reset all those which have not been touched when switching profile.
  • a way to see that a setting is differs from its default value when a setting is collapsed in the form (an icon near the help sign maybe ?)

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
Copy link
Member

chrstnbwnkl commented Nov 10, 2023

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
Copy link
Contributor Author

jthiard commented Nov 10, 2023

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
Copy link
Member

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 !

@nilsnolde nilsnolde merged commit 37cd6d5 into gis-ops:master Nov 11, 2023
1 of 2 checks passed
@jthiard jthiard deleted the fix/isochrone-reset-settings branch November 11, 2023 18:13
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.

None yet

3 participants