Adjust speed penalty and add dimensions to autocost by johannes-no · Pull Request #5627 · valhalla/valhalla

johannes-no · 2025-10-21T08:09:24Z

As discussed in #5595, this pull request extends autocosting to include the dimensions of length and weight.
In addition, the speed penalty for set top speeds has been adjusted. This can now be controlled via the input parameter “speed_penalty_factor” so it can be ignored entirely by setting it to 0.

I am open for suggestions and also nitpicks as I am looking to improve my C++ coding.

Johannes Nonnenmacher on behalf of Mercedes-Benz Tech Innovation GmbH.
Provider Information

…put parameter

chrstnbwnkl · 2025-10-21T10:03:27Z


+// Default speed penalty factor (increase the cost of an edge if the edge speed is faster than the top speed)
+constexpr float kDefaultSpeedPenaltyFactor = 0.05f;
+constexpr ranged_default_t<float> kSpeedPenaltyFactorRange{0.0f, kDefaultSpeedPenaltyFactor, 1.0f};


the lower bound needs to be kMinFactor to avoid zero edge cost

Ah nevermind, I was under the impression this was multiplied with the edge cost directly, but in this case we want to be able to set the speed penalty to 0.

chrstnbwnkl · 2025-10-21T10:07:40Z

  midgard::ranged_default_t<float> height_;
  midgard::ranged_default_t<float> width_;
+  midgard::ranged_default_t<float> length_;
+  midgard::ranged_default_t<float> weight_;


If these are now present in the BaseCostingOptions, shouldn't they be removed from TruckCost, and then TruckCost would simply override the defaults?

valhalla/src/sif/truckcost.cc

Lines 107 to 114 in 8a0ae9d

BaseCostingOptionsConfig GetBaseCostOptsConfig() {

BaseCostingOptionsConfig cfg{};

// override defaults

cfg.service_penalty_.def = kDefaultServicePenalty;

cfg.use_tracks_.def = kDefaultUseTracks;

cfg.use_living_streets_.def = kDefaultUseLivingStreets;

return cfg;

}

Wouldn't this prevent us from having different ranges? For example, TruckLength currenlty has a range of 0 to 50 m, while the range for cars is 0 to 20 m.
If this is not needed i can change it.

Mh, I didn't think about that. But generally I favor opening up ranges, because it's hard to make assumptions about edge cases (makes me think of Falsehoods Programmers Believe About Names) . Also I'd say widening these ranges comes with hardly any performance penalty.

chrstnbwnkl · 2025-10-21T10:12:26Z

 constexpr float kDefaultAutoHeight = 1.6f; // Meters (62.9921 inches)
 constexpr float kDefaultAutoWidth = 1.9f;  // Meters (74.8031 inches)
+constexpr float kDefaultAutoLength = 5.3f; // Meters (208,661 inches)
+constexpr float kDefaultAutoWeight = 3.0f; // Metric Tons (6613,87 lbs)


The default weight seems a little high maybe? I don't know too much about cars, but this looks more like the weight of a SUV. I think we'd be better off by setting it to an optimistically low value in order not to have routes failing that previously wouldn't have failed.

Yeah agree. IMO we don't even need defaults at all (or very low ones, passing the smallest car). That'd keep it as it currently is. Roads with car access are 99.99% built with cars in mind. The main point is that now you can modify that for costing. If anyone drives a monster like the dimensions here, they'll need to pass them.

Agree 100% ill change it to the size of a smaller car.

Co-authored-by: Nils <nilsnolde@proton.me>

chrstnbwnkl · 2025-10-24T13:04:29Z

For me this is good to go if you just format the code again and resolve the changelog conflicts (we just released 3.6.0, so this needs to go in the new section now)

nilsnolde · 2025-10-27T16:53:34Z

+constexpr float kDefaultHeight = 1.6f; // Meters (62.9921 inches)
+constexpr float kDefaultWidth = 1.9f;  // Meters (74.8031 inches)
+constexpr float kDefaultLength = 2.7f; // Meters (208,661 inches)
+constexpr float kDefaultWeight = 0.8f; // Metric Tons (6613,87 lbs)


just skimming again: is this the same default for truck? that'd be a change we probably shouldn't do. not that I really agree with the defaults there, but default truck should stay the same regardless

The truck profile overwrites the defaults with the truck defaults, so the default values for trucks remain the same.

nilsnolde · 2025-11-04T13:05:41Z

@johannes-no fyi, CI is failing, smth relevant to this PR on osx

nilsnolde · 2025-11-04T13:05:59Z

and you'll need to run format.sh

…tocost # Conflicts: # valhalla/sif/dynamiccost.h

johannes-no · 2025-11-11T07:49:36Z

Hello, sorry, due to vacation, this has taken an unnecessarily long time.
I also had to talk to our FOSS representative again. We now have a new fork mercedes-employees-and-valhalla-maintainer and will contribute everything from there now. I have authorized @nilsnolde, @chrstnbwnkl & @kevinkreiser such that you can merge and adjust stuff more easy in your Thursday round.
Should we also move 5596 and 5679 to the new repo or would this be to much confussion for now?

nilsnolde · 2025-11-11T08:40:28Z

Cool, thanks! For me it'd be easier to keep the other PRs on the old fork, so comments etc are preserved.

# Conflicts: # proto/options.proto

johannes-no · 2025-11-20T08:42:03Z

@chrstnbwnkl do you guys have your sync today? Maybe we can merg this

chrstnbwnkl

I think the code looks good now, could you also move the costing parameters description from the truck section to the table above it ("Vehicle Options")?

chrstnbwnkl · 2025-11-20T11:25:15Z

Can you update it as well (replace truck with vehicle and add diverging defaults for truck)?

chrstnbwnkl

I just realize I have edit privileges on this branch, so could've done this myself 😄 thanks!

johannes-no and others added 6 commits October 14, 2025 09:08

Add length and weight to auto costing and make speed penalty as an in…

583ce1f

…put parameter

add missing parser

40432e5

typo

0dcbf47

Merge branch 'valhalla:master' into jn-dimensions-to-autocost

8c3e27a

Merge branch 'valhalla:master' into jn-dimensions-to-autocost

4bf3624

adjust CHANGELOG

15569ec

chrstnbwnkl reviewed Oct 21, 2025

View reviewed changes

johannes-no added 4 commits October 22, 2025 14:58

set default dimensions to those of a "Smart"

69c0d7b

adjust defaults and ranges of dimensions

8cdb2fd

Merge branch 'master' into jn-dimensions-to-autocost

5c3a987

adjust CHANGELOG.md

ef5ff6a

nilsnolde reviewed Oct 24, 2025

View reviewed changes

Comment thread docs/docs/api/turn-by-turn/api-reference.md Outdated

johannes-no and others added 2 commits October 24, 2025 14:44

Update docs/docs/api/turn-by-turn/api-reference.md

48cd2a7

Co-authored-by: Nils <nilsnolde@proton.me>

typo

17d80b3

johannes-no added 2 commits October 24, 2025 16:03

Merge branch 'master' into jn-dimensions-to-autocost

69a8cd9

adjust CHANGELOG.md

fda5306

nilsnolde reviewed Oct 27, 2025

View reviewed changes

johannes-no added 3 commits November 3, 2025 09:20

add vehicle dimensions

a93d6b6

Merge branch 'master' into jn-dimensions-to-autocost

83d8d28

update CHANGELOG.md

15e1abc

meijie-xiang and others added 5 commits November 4, 2025 23:05

clang-tidy

e5e552f

fix gurka test

739952f

Merge remote-tracking branch 'origin/master' into jn-dimensions-to-au…

526b893

…tocost # Conflicts: # valhalla/sif/dynamiccost.h

update CHANGELOG.md

26ffd4a

typo and order BaseCostingOptionsConfig

51b940b

Merge branch 'valhalla:master' into jn-dimensions-to-autocost

5b6e2bb

johannes-no added 9 commits November 12, 2025 08:30

Merge branch 'master' into jn-dimensions-to-autocost

1469368

update CHANGELOG.md

bf979f4

Merge branch 'master' into jn-dimensions-to-autocost

5cf625a

update CHANGELOG.md

7edef3c

Merge branch 'master' into jn-dimensions-to-autocost

9e5e650

update CHANGELOG.md

ac40eab

Merge branch 'master' into jn-dimensions-to-autocost

24dd99f

# Conflicts: # proto/options.proto

Merge branch 'master' into jn-dimensions-to-autocost

cf289a8

update CHANGELOG.md

9669af4

chrstnbwnkl requested changes Nov 20, 2025

View reviewed changes

adjust api reference

0247c01

johannes-no and others added 2 commits November 24, 2025 10:18

Merge branch 'valhalla:master' into jn-dimensions-to-autocost

652d780

adjust defaults in api reference

57fa919

chrstnbwnkl approved these changes Nov 24, 2025

View reviewed changes

chrstnbwnkl enabled auto-merge (squash) November 24, 2025 09:46

Merge branch 'valhalla:master' into jn-dimensions-to-autocost

4855060

chrstnbwnkl merged commit 89a1d1b into valhalla:master Nov 24, 2025
18 checks passed

johannes-no deleted the jn-dimensions-to-autocost branch November 25, 2025 09:35

	BaseCostingOptionsConfig GetBaseCostOptsConfig() {
	BaseCostingOptionsConfig cfg{};
	// override defaults
	cfg.service_penalty_.def = kDefaultServicePenalty;
	cfg.use_tracks_.def = kDefaultUseTracks;
	cfg.use_living_streets_.def = kDefaultUseLivingStreets;
	return cfg;
	}

Conversation

johannes-no commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrstnbwnkl commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsnolde commented Nov 4, 2025

Uh oh!

nilsnolde commented Nov 4, 2025

Uh oh!

johannes-no commented Nov 11, 2025

Uh oh!

nilsnolde commented Nov 11, 2025

Uh oh!

johannes-no commented Nov 20, 2025

Uh oh!

chrstnbwnkl left a comment

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl commented Nov 20, 2025

Uh oh!

chrstnbwnkl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrstnbwnkl Oct 21, 2025 •

edited

Loading