Configurable livespeed fading #4340
Conversation
valhalla/baldr/graphtile.h
Outdated
| @@ -640,12 +640,12 @@ class GraphTile { | |||
| // TODO(danpat): for short-ish durations along the route, we should fade live | |||
| // speeds into any historic/predictive/average value we'd normally use | |||
|
|
|||
| constexpr double LIVE_SPEED_FADE = 1. / 3600.; | |||
| double live_speed_fade = 1. / live_speed_fading_sec_; | |||
There was a problem hiding this comment.
instead of doing it every time we load a graph tile and adding it as a member why not just do it once right here:
| double live_speed_fade = 1. / live_speed_fading_sec_; | |
| static double live_speed_fade = 1. / config().get<double>("baldr.live_speed_fading_sec", 3600); |
we dont need to catch the unconfigured exception because we are screwed anyway if we havent already configured the library (cant even get access to tiles without config)
There was a problem hiding this comment.
@johannes-no have you considered simplifying to the above?
|
this looks good to me, just wondering your thoughts on my comment @johannes-no |
valhalla/baldr/graphtile.h
Outdated
| @@ -652,12 +653,12 @@ class GraphTile { | |||
| // TODO(danpat): for short-ish durations along the route, we should fade live | |||
| // speeds into any historic/predictive/average value we'd normally use | |||
|
|
|||
| constexpr double LIVE_SPEED_FADE = 1. / 3600.; | |||
| double live_speed_fade = 1. / config().get<float>("baldr.live_speed_fading_sec", 3600); | |||
There was a problem hiding this comment.
you're still missing a static here, currently it'll still be computed every single time.
johannes-no
force-pushed
the
configurable_fading
branch
from
a2c8956
to
ab200cb
Compare
| @@ -270,6 +270,7 @@ config = { | |||
| 'max_exclude_polygons_length': 10000, | |||
| 'max_distance_disable_hierarchy_culling': 0, | |||
| }, | |||
| 'baldr': {'live_speed_fading_sec': 3600}, | |||
There was a problem hiding this comment.
I'd actually put this into thor instead of baldr, which is actually only hosting data structures and thor is the only one really using this
| #include "config.h" | ||
| #include <mutex> | ||
| namespace valhalla { | ||
| const boost::property_tree::ptree& config(const std::string& config_file_or_inline) { | ||
| static config_singleton_t instance(config_file_or_inline); | ||
| return instance.config_; | ||
| } | ||
| }; // namespace valhalla |
There was a problem hiding this comment.
why break this out into a .cc file? it's sufficiently small for a header function no? also mutex shouldn't be needed as include?
| @@ -652,12 +653,13 @@ class GraphTile { | |||
| // TODO(danpat): for short-ish durations along the route, we should fade live | |||
| // speeds into any historic/predictive/average value we'd normally use | |||
|
|
|||
| constexpr double LIVE_SPEED_FADE = 1. / 3600.; | |||
| static double live_speed_fade = 1. / config().get<float>("baldr.live_speed_fading_sec", 3600); | |||
There was a problem hiding this comment.
I'd keep the all caps for the variable, it's still pretty much a constant, even if not a compile-time constant
Fixes #4177
with this PR we would like to make livespeed fading time configurable in valhalla.json. For this we use the config singelton.
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