Add "DIVIDE" command and new "maxslope" and "maxslopecost" parameters #642

simdens · 2023-11-01T19:53:14Z

To extend the functionality an additional "DIV" command and two additional parameters are defined. An example instance can be tested under Brouter-Web Dev Instance with fastbike-longdistance.brf profile.

Reasoning

"DIV" command:
The profile can compute constants within the global context based on physical values like systemweight or nominal power.

"downhillmaxbuffercost" and "uphillmaxbuffercost"
These parameters allow to increase the uphill or downhill penalty if the buffer exceeds the maximum buffer value. This makes it possible to increase the penalty for very step hills. i.e. for uphills which are too steep for the given gear ratio. See the fastbike-longdistance.brf profile for reference.

…is makes it possible to penalize very steep path sections

simdens · 2023-11-02T12:51:04Z

This merge request partially solves #617 as it introduces the possibility of two different elevation penalties depending on the slope of the road.

quaelnix · 2023-11-05T14:47:14Z

I have also missed a 'div' operator a few times myself, but I assumed it was left out on purpose, so I would like to get @abrensch's opinion on this.

As for the maxbuffercost logic, I'm not yet sure this is the right way to go, but the concept is interesting!

By looking at the way you designed your profile I was wondering if it maybe was enough to also allow the usage of the elevationbufferreduce command in the way context in order to solve #617?

simdens · 2023-11-05T14:56:41Z

Yes, I assume the 'div' operator was left out on purpose for performance reasons. Nevertheless it was not possible to create a physics based profile without the operator. That's why I added it. At least on a server or normal PC, even very long routes are possible.

About the elevationbufferreduce: Can you please further elaborates how that can solve #617 or how that can implement a kind of a slope limit like in my profile?

afischerdev · 2023-11-05T16:56:17Z

I don't see a performance risk on the div command.
But I see the exception problem.

When you add something like this to your profile, it will work without exception.

assign x
  max 1
  add div 1 2
  add div 2 1
  add div 3 0
  add div 0 3
  1

It just produces an infinity variable.
But if you use this var you will run into an error. Sample

assign costfactor
  max 1
  add x
  costsum

The result in error log is:
Error (linksProcessed=0 open paths: 0): from-position not mapped in existing datafile
With a small error handling in BExpression you will get the 'real' result - for instance:

  private float div(float v1, float v2) {
    if (v2 == 0f) throw new IllegalArgumentException("div by zero");
    return v1 / v2;
  }

Result:
ParseException .\profiles2\minimal.brf at line 39: div by zero

quaelnix · 2023-11-05T17:53:27Z

About the elevationbufferreduce: Can you please further elaborates how that can solve #617 or how that can implement a kind of a slope limit like in my profile?

I thought about something along the lines of this:

assign elevationmaxbuffer     100
assign elevationbufferreduce  20
assign elevationpenaltybuffer 10

assign downhillcost   0
assign downhillcutoff 2
assign uphillcost     0
assign uphillcutoff   2

...

assign costfactor ...

assign uphillcostfactor   multiply 5 costfactor
assign downhillcostfactor multiply 5 costfactor

Here is a test profile if you want to play around with it. But it sadly does not work as well as I thought it would.

simdens · 2023-11-07T21:23:43Z

  private float div(float v1, float v2) {
    if (v2 == 0f) throw new IllegalArgumentException("div by zero");
    return v1 / v2;
  }

I have added that error handling. Thanks for suggesting!

assign elevationmaxbuffer     100
assign elevationbufferreduce  20
assign elevationpenaltybuffer 10

assign downhillcost   0
assign downhillcutoff 2
assign uphillcost     0
assign uphillcutoff   2

...

assign costfactor ...

assign uphillcostfactor   multiply 5 costfactor
assign downhillcostfactor multiply 5 costfactor

Very interesting approach. But I do not see any way to (soft) limit the maximum allowed slope of a route. At least to my understanding, one still needs the elevation height of the segment which is not directly available.

quaelnix · 2023-11-09T13:05:25Z

But I do not see any way to (soft) limit the maximum allowed slope of a route.

You do that by reducing the elevationbufferreduce to the desired maximum allowed slope.

simdens · 2023-11-09T20:25:35Z

But I do not see any way to (soft) limit the maximum allowed slope of a route.

You do that by reducing the elevationbufferreduce to the desired maximum allowed slope.

I already do that. But to actually limit to a maximum slope, the exceeding elevation must be penalized more heavily.

quaelnix · 2023-11-12T14:41:06Z

What I was trying to say is that in my test profile (which is designed to work on a regular BRouter instance without the contents of this pull request) you can also use elevationbufferreduce to soft limit the maximum allowed slope.

But to actually limit to a maximum slope, the exceeding elevation must be penalized more heavily.

My test profile does this by increasing the uphillcostfactor like so:

assign uphillcostfactor   multiply 5 costfactor

This merge request partially solves #617 as it introduces the possibility of two different elevation penalties depending on the slope of the road.

I wonder if we could add a slope penalty based directly on the incline that we already calculate in StdPath?

brouter/brouter-core/src/main/java/btools/router/StdPath.java

Line 216 in 109782d

double incline = calcIncline(dist);

quaelnix · 2023-11-16T16:58:13Z

I've been playing around with it a bit more and I have to say that your code works really well!

At least to my understanding, one still needs the elevation height of the segment which is not directly available.

To my understanding, you don't have to, because you can set elevationmaxbuffer to an arbitrarily large number.

I wonder if we could add a slope penalty based directly on the incline that we already calculate in StdPath?

Here's a little hot-rodded code snippet that illustrates the idea: quaelnix@a00d992

The syntax is straightforward. "maxupslope" and "maxdownslope" define the threshold values for the maximum permissible inclines and declines above which the additional penalties becomes effective. upslopecostfactor and downslopecostfactor optionally allow to adjust the amount of additional penalty and thus the "softness" of the limit:

assign maxupslope 9
assign maxdownslope -15
#assign upslopecostfactor multiply 10 costfactor
#assign downslopecostfactor multiply 10 costfactor

The remaining problem seems to be, that both of our ideas lack the 'fine-grained' part in order to truely solve #617.

quaelnix · 2023-11-19T20:55:26Z

@simdens, I spent some more time with this pull request and the more I think about it, the more I like it. It is simple, yet very powerful and gets us a fair bit closer to solving #617, while sticking to existing concepts and the performance overhead should be reasonably small.

@afischerdev, what do you think about it?

simdens · 2023-11-20T01:43:07Z

Sorry. Currently on a business trip. Will write some more comments in a week.

afischerdev · 2023-11-23T08:35:29Z

Looks good to perfekt for me.
My test was around here
and the profile was reduced to the bare essentials

---context:global   # following code refers to global config

assign   validForBikes        1

#assign maxupslope 9
#assign maxdownslope -15

#assign maxSpeed 35

---context:way   # following code refers to way-tags

assign turncost 0
assign initialcost 0

assign costfactor
  switch not highway=   1
  switch not route=       1
  100000

---context:node  # following code refers to node tags

assign initialcost 0

Results - forward / revers - show what was expected. Blue the route without, red with activated params

Also speed check still works .

afischerdev · 2023-11-24T09:37:29Z

One more test:
Start on a hill with all ways down less then maxdownslope brings a route down anyway.

afischerdev · 2023-11-24T09:44:50Z

And an addtional idea:
bring out the slope values in json format like the times array:

Sample: Add an incline value to message class and add it like this

"slopes": [0.0,-0.4,-1.9,-2.2,-0.1,-1.7,-5.2,-5.9,-6.2,-6.6,-7.5,-7.8,-8.0,-7.4,-6.1,-6.2,-4.9,-7.1,-5.3,-6.7,-9.6]

simdens · 2023-12-06T11:38:41Z

I've been playing around with it a bit more and I have to say that your code works really well!

At least to my understanding, one still needs the elevation height of the segment which is not directly available.

To my understanding, you don't have to, because you can set elevationmaxbuffer to an arbitrarily large number.

I wonder if we could add a slope penalty based directly on the incline that we already calculate in StdPath?

Here's a little hot-rodded code snippet that illustrates the idea: quaelnix@a00d992

The syntax is straightforward. "maxupslope" and "maxdownslope" define the threshold values for the maximum permissible inclines and declines above which the additional penalties becomes effective. upslopecostfactor and downslopecostfactor optionally allow to adjust the amount of additional penalty and thus the "softness" of the limit:
assign maxupslope 9
assign maxdownslope -15
#assign upslopecostfactor multiply 10 costfactor
#assign downslopecostfactor multiply 10 costfactor
The remaining problem seems to be, that both of our ideas lack the 'fine-grained' part in order to truely solve #617.

I finally understand. Yes, that might be a way to linearly increase cost depending on slope. Simple and easy :-)

And an addtional idea: bring out the slope values in json format like the times array:

Sample: Add an incline value to message class and add it like this
"slopes": [0.0,-0.4,-1.9,-2.2,-0.1,-1.7,-5.2,-5.9,-6.2,-6.6,-7.5,-7.8,-8.0,-7.4,-6.1,-6.2,-4.9,-7.1,-5.3,-6.7,-9.6]

What do you mean with this suggestion? And what is the advantage of this?

quaelnix · 2023-12-06T13:14:46Z

@simdens, I think the idea is to enrich the json output (sent to e.g. BRouter-Web) with incline values, but I don't think this will work well, as the average incline of a way segment, which can be several kilometers long, is not very informative.

The only remaining issue I have with this pull request is that I dislike the nomenclature. maxbuffercost is misleading in my opinion, but at the same time, I do not have a better suggestion.

sjakobi · 2023-12-06T13:40:30Z

First off: Thanks a lot for working on a (partial) solution for #617, @simdens! :)

Regarding the new div command and the new parameters, I think it would be great to have some kind of documentation on what they do.

Based on the link to #617 I was under the impression that the div command would somehow make it possible to divide elevation cost definitions into segments for different slopes. ;) I had to look at the sample profile to figure out that it's just basic mathematical division.

(Minor bikeshed: Would it be better to avoid the abbreviation and spell out divide in analogy to the existing multiply function?)

For "downhillmaxbuffercost" and "uphillmaxbuffercost", I think it would be good to see some documentation that includes an annotated example. The documentation could be added to the wiki once the PR has been merged.

simdens · 2023-12-06T14:44:43Z

Agree, documentation is missing. I will include an updated profile_developers_guide.md to the pull request ASAP.

About the naming:

"div":
I have chosen the name according to the scheme for addition ("add") and subtraction ("sub"). But yes, multiplication ("multiply") does not use the short form. So at least for me it seems to be more consistent to go for "divide" instead of "div".
"uphillmaxbuffercost" and "uphillmaxbuffercost":
I don't like the names either. But every more precice name I came up was even longer. I thout about "uphillmaxbufferexceedcost" 🙈 A bit more clear but even longer. Perhaps we start a little brainstorming (same for downhill parameter):
- "uphillmaxbufferexceedcost"
- "uphillbufferexceedcost" (currently my favorite)
- "uphillbufexcost"

@quaelnix What are your thoughts about the naming? I'd like to agree first here before changing in the code.

simdens · 2023-12-06T15:41:25Z

@quaelnix About your commit 6a84490: I like the new clean code style! Well done! Nevertheless, I would prefer some more (ok, to be honest, a lot more) comments to make it easier to understand the code. I always find it hard to understand even my own code after some time and thus add a lot of comments everywhere in the code. Until now, that helped me a lot to understand the code faster when maintaining it.

quaelnix · 2023-12-06T19:59:19Z

So at least for me it seems to be more consistent to go for "divide" instead of "div".

I like it.

A bit more clear but even longer.

The problem is that both uphillcost and uphillmaxbuffercost penalize elevation that exceeds the elevationmaxbuffer:

uphillcost is the costfactor for "excess elevation" below the elevationbufferreduce induced threshold
uphillmaxbuffercost is the costfactor for "excess elevation" above the elevationbufferreduce induced threshold

The split point is not the size of the buffer, but the slope, which is implicitly defined by the bufferreduce logic.

I would prefer some more (ok, to be honest, a lot more) comments to make it easier to understand the code.

Ok, but wouldn't it make more sense in this case to make the variable names more meaningful instead?

excess = ehbd - rc.elevationmaxbuffer;          // elevation which excesses the maxbuffer threshold

->

buffer_excess = elevation_hysteresis_buffer_downhill - rc.elevationmaxbuffer;

Additionally, when reaching elevationmaxbuffer, uphillcostfactor/ downhillcostfactor fully replaces costfactor in way cost calculation if those cost factors are defined.

This is true yet misleading. The elevationmaxbuffer does not affect the costfactor logic. Try:

Global context: assign elevationmaxbuffer 1000
Way context: assign uphillcostfactor 10

afischerdev · 2024-01-16T15:40:12Z

@quaelnix
I've tested new with the extra parameter uphillmaxslopecost anddownhillmaxslopecost and get same result as on my first test.
Merge now?

- Allow to set both the maxslope and the maxslopecost in the way context separately for uphill and downhill - New names for the new commands that better reflect what they actually do

quaelnix · 2024-01-16T19:34:45Z

@afischerdev, yes, I think it will make a lot of users very happy, and if we see the need for it, we can always expand it later.

We should probably use 'Squash and merge' to compress all the minor changes into a single commit that is properly attributed to @simdens who came up with this awesome idea.

simdens · 2024-02-04T18:08:21Z

Now had the time to test the current version. Looks good to me. Even though the downhillmaxslope and uphillmaxslope parameters appear a bit redundant to elevationbufferreduce. Can not think of any situation, were one sets elevationbufferreduce with a different value than the other two.

A small typo is still in profile_developers_guide.md: In the Operant list is still div instead of the correct divide command. I commited the fix to my dev branch.

quaelnix · 2024-02-04T18:32:29Z

The idea behind this was that you can now set the thresholds in the way context separately for uphill and downhill depending on the properties of the way. You can now create mountain bike profiles with like 4 lines of code where the profile automatically selects good (boring) paths for climbing up the hill and bad (fun) paths for racing down the hill.

simdens · 2024-02-04T18:34:32Z

Ah. Understand.

Sounds like a great idea. I have to think about the possibilities and may have to extend my profile :-D

simdens · 2024-02-04T19:03:28Z

If someone is interested about the kind of profiles possible with this change:

Thanks a lot for your work to make this possible.

quaelnix · 2024-02-05T09:36:56Z

I am still wondering if we should rename uphillmaxslopecost into something like uphillcostabovemaxslope and downhillmaxslopecost into something like downhillcostbelowmaxslope. The new parameter names would be truely ugly, but at the same time they would tell you exactly what they do. What do you think @simdens.

quaelnix · 2024-02-05T10:21:12Z

If someone is interested about the kind of profiles possible with this change:

Your profile contains some sexy shit! I'll definitely have to take a closer look at it!

PS: BRouter expects S_C_x to be the drag coefficient times reference area (m^2) times half air density (kg/m^3).

simdens · 2024-02-06T20:50:15Z

PS: BRouter expects S_C_x to be the drag coefficient times reference area (m^2) times half air density (kg/m^3).

Thanks for the hint! I've fixed it :-)

simdens · 2024-02-06T20:53:00Z

I am still wondering if we should rename uphillmaxslopecost into something like uphillcostabovemaxslope and downhillmaxslopecost into something like downhillcostbelowmaxslope. The new parameter names would be truely ugly, but at the same time they would tell you exactly what they do. What do you think @simdens.

I understand your concerns. But I don't like very long names for parameters and I do not have the creativity to find a better short one. To be honest, I'd go for the current parameter names.

simdens · 2024-02-25T16:12:50Z

With the changes from this merge request, for me it is not possible to change the downhillmaxslope variable in the way context:

Is there any other merge request I have to include into my brouter instance?

quaelnix · 2024-02-25T17:11:48Z

The downhillmaxslopecost variable will be readonly in the way context if you have already defined it in the global context.

simdens · 2024-02-25T17:13:14Z

🙈 yes. I just noticed in that second. Sorry for my stupid question and thanks for your very quick answer!

quaelnix · 2024-02-25T17:50:09Z

No worries, this also caught me in surprise a while ago.

simdens · 2024-03-04T20:27:30Z

Finally included the speed limitation to the "Long Distance" profile thanks to the configurable downhillmaxslope :-) Great idea, thanks!

LupinSun · 2024-03-28T16:18:58Z

Hi everyone, these "slope" parameters are already present in locus and Brouter, are they still under development or will they never be implemented, because they would be perfect for my needs.

quaelnix · 2024-03-29T07:20:56Z

@LupinSun, you can try them out at: https://brouter.de/brouter-test/

PS: The test area is currently limited to Germany and there are no pseudo tags on this server.

LupinSun · 2024-03-29T09:53:06Z

@LupinSun, you can try them out at: https://brouter.de/brouter-test/

PS: The test area is currently limited to Germany and there are no pseudo tags on this server.

Thank you, I will wait for the test to be extended to Italy. Where can I find news about developments and changelogs so that I can be aware of new features and future developments?

afischerdev · 2024-04-03T12:18:37Z

@LupinSun
Sorry for the delay, I was away for a while. And you are right, we haven't a high rate in updates.
I'll start with the process.

May be meanwhile the test for 1asec elevation #644 could stop and the test server could use standard data pool?

quaelnix · 2024-04-03T13:25:13Z

May be meanwhile the #644 (comment) for 1asec elevation #644 could stop and the test server could use standard data pool?

Sounds reasonable to me.

afischerdev · 2024-04-04T08:23:33Z

@quaelnix
Fine, thanks, I restarted the testserver with last lib and standard data pool

simdens added 2 commits October 25, 2023 21:00

Added 'DIV' expression for profiles

3c8fee7

Added 'uphillmaxbuffercost' and 'downhillmaxbuffercost' parameter. Th…

8f15385

…is makes it possible to penalize very steep path sections

simdens temporarily deployed to BRouter November 2, 2023 07:42 — with GitHub Actions Inactive

Added 'div by zero' check in BExpression.java DIV command

879371c

simdens temporarily deployed to BRouter November 13, 2023 20:01 — with GitHub Actions Inactive

quaelnix added the enhancement label Nov 13, 2023

Simplify maxbuffercostdiv logic

6a84490

quaelnix temporarily deployed to BRouter November 19, 2023 20:53 — with GitHub Actions Inactive

Added documentation about new features

b8b986d

Redesign the new commands

7ce187e

- Allow to set both the maxslope and the maxslopecost in the way context separately for uphill and downhill - New names for the new commands that better reflect what they actually do

quaelnix temporarily deployed to BRouter January 16, 2024 18:45 — with GitHub Actions Inactive

Adapt the profile developers guide to the latest changes

00dce2d

quaelnix temporarily deployed to BRouter January 16, 2024 19:31 — with GitHub Actions Inactive

Improve wording

826f520

quaelnix temporarily deployed to BRouter January 16, 2024 19:34 — with GitHub Actions Inactive

quaelnix changed the title ~~Add "DIVIDE" command and two additional parameters "downhillmaxbuffercost" and "uphillmaxbuffercost"~~ Add "DIVIDE" command and new "maxslope" and "maxslopecost" parameters Jan 17, 2024

afischerdev merged commit 2f14223 into abrensch:master Jan 17, 2024
1 check passed

afischerdev added this to the Version 1.7.4 milestone Jan 17, 2024

Add "DIVIDE" command and new "maxslope" and "maxslopecost" parameters #642

Add "DIVIDE" command and new "maxslope" and "maxslopecost" parameters #642

Conversation

simdens commented Nov 1, 2023 • edited Loading

Reasoning

simdens commented Nov 2, 2023

quaelnix commented Nov 5, 2023

simdens commented Nov 5, 2023 • edited Loading

afischerdev commented Nov 5, 2023

quaelnix commented Nov 5, 2023 • edited Loading

simdens commented Nov 7, 2023

quaelnix commented Nov 9, 2023 • edited Loading

simdens commented Nov 9, 2023

quaelnix commented Nov 12, 2023 • edited Loading

quaelnix commented Nov 16, 2023 • edited Loading

quaelnix commented Nov 19, 2023 • edited Loading

simdens commented Nov 20, 2023

afischerdev commented Nov 23, 2023

afischerdev commented Nov 24, 2023

afischerdev commented Nov 24, 2023

simdens commented Dec 6, 2023

quaelnix commented Dec 6, 2023

sjakobi commented Dec 6, 2023

simdens commented Dec 6, 2023

simdens commented Dec 6, 2023

quaelnix commented Dec 6, 2023

afischerdev commented Jan 16, 2024

quaelnix commented Jan 16, 2024

simdens commented Feb 4, 2024

quaelnix commented Feb 4, 2024

simdens commented Feb 4, 2024

simdens commented Feb 4, 2024

quaelnix commented Feb 5, 2024

quaelnix commented Feb 5, 2024

simdens commented Feb 6, 2024 • edited Loading

simdens commented Feb 6, 2024

simdens commented Feb 25, 2024

quaelnix commented Feb 25, 2024

simdens commented Feb 25, 2024

quaelnix commented Feb 25, 2024

simdens commented Mar 4, 2024

LupinSun commented Mar 28, 2024

quaelnix commented Mar 29, 2024 • edited Loading

LupinSun commented Mar 29, 2024

afischerdev commented Apr 3, 2024

quaelnix commented Apr 3, 2024

afischerdev commented Apr 4, 2024

simdens commented Nov 1, 2023 •

edited

Loading

simdens commented Nov 5, 2023 •

edited

Loading

quaelnix commented Nov 5, 2023 •

edited

Loading

quaelnix commented Nov 9, 2023 •

edited

Loading

quaelnix commented Nov 12, 2023 •

edited

Loading

quaelnix commented Nov 16, 2023 •

edited

Loading

quaelnix commented Nov 19, 2023 •

edited

Loading

simdens commented Feb 6, 2024 •

edited

Loading

quaelnix commented Mar 29, 2024 •

edited

Loading