Configurable fading of live speed #4497
Conversation
This reverts commit ec49e9a.
|
Okay thanks a lot @nilsnolde! We will revise our merg process again so that this will hopefully no longer happen in the future. In addition, we have encountered further problems with the Config Singleton which we are currently fixing and then contributing, so unfortunately this story had less priority. It was my mistake not to make you aware of this earlier. |
|
No worries, we're happy to have this change as it was a bit of an opinionated constant. |
| @@ -388,18 +381,18 @@ boost::property_tree::ptree make_config(const std::string& path_prefix, | |||
| // force the paths to be different | |||
| boost::replace_all(defaults, "%%", path_prefix); | |||
|
|
|||
| // make ptree and override defaults | |||
There was a problem hiding this comment.
this should be fine, nothing really changed
There was a problem hiding this comment.
yeah config singleton is a double edged sword. on one hand using makes life way easier for 99.99% of all use-cases. but then in testing, if we need to change the configs to test something and the singleton is used throughout the library it will be a gotcha. im all for crossing that bridge when we get there (i have an idea for how to make it work)
There was a problem hiding this comment.
for tests I think it's fine though right? bcs we can change things with overrides & removes when we call make_config.
There was a problem hiding this comment.
once there are calls to the config singleton embedded deep in the library code, a test wont be able to control which which one it will get. it will always get the first config it was called with. so in the future when that is a problem we'll need a way to inject a change overtop of the singletons initial copy
|
hmpf, adding the config singleton to the graphtile results in many more changes than I'd have liked.. basically anything calling GetSpeed needs to have the singleton initiated now.. |
|
Yes exactly, we would have to adapt many places. That's why we had implemented the try here in the initial merg request. How should we proceed now? |
|
I'd just bite the bullet and do all those changes. Would have to be done eventually anyways. It's a bit annoying but not the end of the world. The actual PR is just a few lines, so all those changes won't distract too much. |
| @@ -57,7 +50,7 @@ struct config_singleton_t { | |||
| config_singleton_t() = delete; | |||
| config_singleton_t(const std::string& config_file_or_inline) { | |||
| if (config_file_or_inline.empty()) { | |||
| throw valhalla::ConfigUninitializedException(); | |||
| throw std::runtime_error("Config singleton was not initialized before usage"); | |||
|
FFS I'm really sick of compiler bullshit.. with gcc 13.2.1 a Release build fails for utrecht_tiles with which of course isn't valid, at this point the singleton is configured by |
|
yeah seems like something is systemically wrong.. i wonder if we are suffering from something more exotic like the initialization fiasco: https://en.cppreference.com/w/cpp/language/siof i dont think so because we always use the "construct on first use" idiom in our singletons but this kind of gotcha seems to be whats happening to us |
supersedes #4340
@johannes-no, I had to open another PR to finish this, your fork wasn't editable. I just added the review comments so we can merge this. I'd soon like to release.