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

Let app and consumers of profiles edit variables #189

Merged
merged 12 commits into from
Oct 5, 2019

Conversation

Phyks
Copy link
Contributor

@Phyks Phyks commented Sep 3, 2019

Hi,

There has been a discussion in BRouter-web recently about letting users customize easily the main features of a given profile (and avoid resorting to multiple different profiles differing by just a boolean value), nrenner/brouter-web#223. This discussion also happened in Locus Maps about two years ago https://help.locusmap.eu/topic/dynamic-brouter-profile-configuration.

We (in BRouter-web) came up with a syntax which is both convenient to build a nice UI on top and retro-compatible with the syntax currently in use by Locus. That is, adding specific comments for the meaningful variables (see nrenner/brouter-web#223 (comment)) of the form

assign <parameter> = <value> # %<parameter>% | Some description | <type>

where type can be (for the moment) one of boolean, number or an option list ([0=none, 1=auto-choose, 2=locus-style, 3=osmand-style] for instance).

I updated the trekking.brf profile as an example, including comments and details for all the relevant variables in this profile. This might also partially address #184. What do you think about this? If you think this syntax is nice, I can proceed with the rest of the profiles, to make them easily consumable/editable by third parties, and edit the developper guides to account for this new syntax.

The main points left to discuss in my opinion are:

  • If we want an (internal) variable to be configurable and editable, it should appear in the profile. However, this can lead to duplication of default value (see assign totalMass = 90 for instance). What about adding a new keyword (DEFAULT ?) to be able to list a variable in a profile, while ensuring it will have the default value from the BRouter-core code (something like assign totalMass = DEFAULT)?

  • There currently are some almost duplicate profiles in BRouter code. This is typically the case of the car-eco.brf and the car-fast.brf profiles where only the vmax variable is changing. I think this makes editing BRouter profiles more difficult as one should carefully maintain multiple files in sync. What about replacing this with a single brf file in this repository and have a script to generate all the useful variants (same for trekking and safety on http://brouter.de/)?

Thanks!

@Phyks
Copy link
Contributor Author

Phyks commented Oct 3, 2019

I did a pass today on the second point mentionned in my first message. With these commits, the profiles from http://brouter.de/brouter/profiles2/ should be available easily inside the repository and I tried to remove and factor out variants as much as possible.

Therefore, I introduced a new script in misc/script/generate_profile_variants.sh to generate the pre-existing variants in the repository (as well as some variants which were available at http://brouter.de/brouter/profiles2).

I'll try to finish this PR tomorrow by completing the required comments for easy editing of the profiles.

Fixes #182.

@Phyks Phyks marked this pull request as ready for review October 4, 2019 11:12
@Phyks
Copy link
Contributor Author

Phyks commented Oct 4, 2019

Done. I should not have changed any behavior of the current profile, the diff is purely rewriting them for generating from a single source with configurable options and variants.

https://github.com/abrensch/brouter/pull/189/files#diff-fb72c54998108aa446b5956c7b615f16 should result in all the previously existing profiles being generated from replacements of variables of the initial profile.

I think further factoring could be done between fastbike, trekking and fastbike-verylowtraffic but I'll leave it for a future pull request, this one is already large enough!

Best,

@poutnikl
Copy link
Contributor

poutnikl commented Oct 4, 2019

Hmm, AFAIK, Menion ( LocusMap developer ) talked about 10 parameters as a reasonable count limit, and was quite strongly against numerical parameters.

I see a lot of %parameter%s in the fastbike update.

But it may still be applicable on BRouter-web, may not it ?

@Phyks
Copy link
Contributor Author

Phyks commented Oct 4, 2019

The conclusion of the discussion with Menion was that BRouter is quite agnostic with respect to the app. There are a lot of variables and configuration options and it's worth exposing them. Then, the app can decide the strategy they want to include them. That's why we use the %parameter% key, for easy filtering (and eventually translating the comments!).

They currently filter to keep only three of them (allow_motorways, avoid_toll etc). They might extend it a bit to include new variables but it is up to them to decide whether they want only boolean value or numbers etc.

BRouter-web might choose a different strategy. Currently, with my PR, they would expose everything, but there will probably be some sorting / ordering / filtering in the future.

BRouter power users will have detailed comments for all the possible config options in the profiles and they can tweak it more easily, rather than digging in the code. :)

@poutnikl
Copy link
Contributor

poutnikl commented Oct 4, 2019

4 of them, 3 for cars and 1 for bikes/pedestrians. You know it was originally implemented in my profiles to adapt them as internal ones in LocusMap, being now more than 2 years old, don't you ? :-)

@Phyks
Copy link
Contributor Author

Phyks commented Oct 4, 2019

I did not know this :) My idea is basically to have something similar to your profiles for internal (default) BRouter profiles :) Hope this can be useful to ease management of yours as well!

@poutnikl
Copy link
Contributor

poutnikl commented Oct 4, 2019

Yes, I suppose it will be useful.

The coming trekking template v2.7 has implemented the 3rd numerical parameter UnivFactorAdjust as addition to MTB_factor and SmallRoads_factor, shifting the neutral profile position.

I may will include few more pictures to Trekking-poutnik wiki :-), perhaps with link to online excel document for illustrative interactive profile tweaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants