-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conditional turn restriction support #3841
Conversation
d1e39b1
to
d0d296a
Compare
ConditionalSpeedLimit speed_limit; | ||
}; | ||
|
||
auto ParseGlobalArguments(int argc, char *argv[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably not be exposed globally, because they are just there to parse CLI parameters. This is important for the conditionals tool, but not for anything just using the functions.
535b4c3
to
488943f
Compare
@oxidase @daniel-j-h If either of you have a time to look at what's going on with inclusion of In my last commit I made a flimsy pass at pulling out the |
CMakeLists.txt
Outdated
if(SHAPEFILE_FOUND) | ||
add_dependency_includes(${LIBSHAPEFILE_LIBRARY}) | ||
if(NOT SHAPEFILE_FOUND) | ||
message(FATAL_ERROR "Shapefile library (libshp-dev) was not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as find_package(P REQUIRED) (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but i don't think it must be a requirement. better to have a warning and conditional compilation without time-based conditionals
CMakeLists.txt
Outdated
@@ -682,7 +684,7 @@ target_link_libraries(osrm_store ${STORAGE_LIBRARIES}) | |||
|
|||
# BUILD_COMPONENTS | |||
add_executable(osrm-components src/tools/components.cpp $<TARGET_OBJECTS:UTIL>) | |||
target_link_libraries(osrm-components ${TBB_LIBRARIES} ${BOOST_BASE_LIBRARIES}) | |||
target_link_libraries(osrm-components ${TBB_LIBRARIES} ${BOOST_BASE_LIBRARIES} ${LIBSHAPEFILE_LIBRARY}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does components need the shapefile lib? transitive dep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should come from UTIL_LIBARIES
eventually.
CMakeLists.txt
Outdated
target_link_libraries(osrm-extract osrm_extract ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-partition osrm_partition ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-customize osrm_customize ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-contract osrm_contract ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-routed osrm ${Boost_PROGRAM_OPTIONS_LIBRARY} ${OPTIONAL_SOCKET_LIBS} ${ZLIB_LIBRARY}) | ||
target_link_libraries(osrm-routed osrm ${Boost_PROGRAM_OPTIONS_LIBRARY} ${LIBSHAPEFILE_LIBRARY} ${OPTIONAL_SOCKET_LIBS} ${ZLIB_LIBRARY}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, why does routed need shapefile lib? and datastore above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util lib is not a library, so it is needed everywhere with $<TARGET_OBJECTS:UTIL>
it must be changed for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, this is a transitive dependency and should come from the UTIL_LIBRARIES
variable.
@karenzshea i have pushed changes to Also there is a problem with You can kick out my commit, it makes all build jobs red because libshp-dev is not installed. Only mason build should be green. |
4a5766d
to
4b85dcf
Compare
CMakeLists.txt
Outdated
@@ -630,11 +635,15 @@ set(CUSTOMIZER_LIBRARIES | |||
${MAYBE_RT_LIBRARY} | |||
${MAYBE_COVERAGE_LIBRARIES}) | |||
set(UPDATER_LIBRARIES | |||
${BZIP2_LIBRARIES} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BZIP2 and EXPAT should only be needed in EXTRACTOR
after the header refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to remove BZIP2 and EXPAND and see if it still compiles?
CMakeLists.txt
Outdated
${TBB_LIBRARIES} | ||
${LIBSHAPEFILE_LIBRARY} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to UTIL_LIBRARIES
CMakeLists.txt
Outdated
@@ -682,7 +684,7 @@ target_link_libraries(osrm_store ${STORAGE_LIBRARIES}) | |||
|
|||
# BUILD_COMPONENTS | |||
add_executable(osrm-components src/tools/components.cpp $<TARGET_OBJECTS:UTIL>) | |||
target_link_libraries(osrm-components ${TBB_LIBRARIES} ${BOOST_BASE_LIBRARIES}) | |||
target_link_libraries(osrm-components ${TBB_LIBRARIES} ${BOOST_BASE_LIBRARIES} ${LIBSHAPEFILE_LIBRARY}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should come from UTIL_LIBARIES
eventually.
CMakeLists.txt
Outdated
target_link_libraries(osrm-extract osrm_extract ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-partition osrm_partition ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-customize osrm_customize ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-contract osrm_contract ${Boost_PROGRAM_OPTIONS_LIBRARY}) | ||
target_link_libraries(osrm-routed osrm ${Boost_PROGRAM_OPTIONS_LIBRARY} ${OPTIONAL_SOCKET_LIBS} ${ZLIB_LIBRARY}) | ||
target_link_libraries(osrm-routed osrm ${Boost_PROGRAM_OPTIONS_LIBRARY} ${LIBSHAPEFILE_LIBRARY} ${OPTIONAL_SOCKET_LIBS} ${ZLIB_LIBRARY}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, this is a transitive dependency and should come from the UTIL_LIBRARIES
variable.
CMakeLists.txt
Outdated
@@ -521,6 +521,11 @@ else() | |||
ENDIF() | |||
ENDIF() | |||
|
|||
find_package(Shapefile) # package libshp-dev | |||
if(NOT SHAPEFILE_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to make the shapefile dependency optional. For this we need to ensure a few things:
- We don't try to link against a missing library when it's not there
- The code that uses shapefile is not compiled when the library is not found.
We get 1. by making the linker flags for shapefiles optional:
find_package(Shapefile)
if (SHAPEFILE_FOUND)
set(MAYBE_SHAPEFILE_LIBRARIES "${SHAPEFILE_LIBRARIES}")
target_include_directories(UTIL PRIVATE ${LIBSHAPEFILE_INCLUDE_DIR})
else()
set(MAYBE_SHAPEFILE_LIBRARIES "")
endif()
This creates a variable that is just empty in case there is no shapefile library found, and only add the header include in case we find the library.
To get 2. we need to tell the C++ code whether it is allowed to use anything that references shapefiles. The typical technique to do that is use defines like #define FOO_BAR
. Luckily we can pass these options straight to the compiler like:
g++ -D FOO_BAR test.cpp -o test
Conveniently cmake can generate those flags for us. Here is an example of how we do this to pass C++ the location of the test directory.
In our case that would be something like:
target_compile_definitions(UTIL PRIVATE COMPILE_DEFINITIONS ENABLE_SHAPEFILE)
2c73307
to
2542b38
Compare
@@ -0,0 +1,384 @@ | |||
#include "util/conditional_restrictions.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include guard missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
}; | ||
#pragma pack(pop) | ||
static_assert(std::is_trivial<TurnIndexBlock>::value, "TurnIndexBlock is not trivial"); | ||
static_assert(sizeof(TurnIndexBlock) == 24, "TurnIndexBlock is not packed correctly"); | ||
static_assert(sizeof(TurnIndexBlock) < 24, "TurnIndexBlock is not packed correctly"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change here? Byte size checks here should always check for equality.
@@ -56,14 +56,18 @@ class Extractor | |||
private: | |||
ExtractorConfig config; | |||
|
|||
std::vector<TurnRestriction> ParseOSMData(ScriptingEnvironment &scripting_environment, | |||
const unsigned number_of_threads); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can pull out the number of threads from the type's extractor config, no need to pass it in here
unsigned requested_num_threads; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a separate parameter is used here because at the call site extractor takes the min of a recommended_num_threads
and the config provided num of threads.
Do we want to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay then we're fine here!
include/extractor/io.hpp
Outdated
const auto fingerprint = storage::io::FileReader::VerifyFingerprint; | ||
storage::io::FileReader reader{path, fingerprint}; | ||
|
||
auto num_indices = reader.ReadElementCount32(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make all counts 64 bits in your new code already; see also #3848
include/extractor/io.hpp
Outdated
reader.DeserializeVector(c.monthdays); | ||
} | ||
res.restriction.flags.is_only = is_only; | ||
restrictions.push_back(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above the code resizes the restrictions
vector to size num_indices
, but here you push_back
into it.
I think what we want is either
- To
.reserve(num_indices)
above (tells the vector we expect roughlynum_incices
elements) and thenpush_back
here or - To
.resize(num_indices)
above and assign torestrictions[i]
here
Think of reserve
as a hint for vector to allocate memory more efficiently. In contrast, resize(n)
adds n
elements to the vector (default constructed).
include/extractor/io.hpp
Outdated
reader.DeserializeVector(c.monthdays); | ||
} | ||
res.restriction.flags.is_only = is_only; | ||
restrictions.push_back(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can std::move(res)
here to move the restriction container into the vector. At the moment we make a copy of res and then destroy res immediately after. Similar, you could put the InputRestrictionContainer
above outside of the loop, otherwise we create an instance of it for every iteration.
include/extractor/io.hpp
Outdated
reader.ReadInto(res.restriction.from); | ||
reader.ReadInto(res.restriction.to); | ||
reader.ReadInto(is_only); | ||
for (auto &c : res.restriction.condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never read into res.restriction.condition
(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
include/storage/io.hpp
Outdated
@@ -104,6 +104,13 @@ class FileReader | |||
} | |||
} | |||
|
|||
void ReadInto(std::string &target) | |||
{ | |||
auto len = ReadElementCount32(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. I would say 64 bit here for consistency. But this would also mean we require 8 bytes per string. These strings are osm strings, no? So it should be bound by 255 * 4 bytes (max. 4 bytes in utf-8 coded code point), and 16 bit should suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using this particular method for osm strings, but it's not unlikely that this method could be reused in the future for strings other than osm strings. Would a 16 bit size still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends how large the strings will be.
numeric_limits<uint8_t>::max(); // -> 255
numeric_limits<uint16_t>::max(); // -> 65535
numeric_limits<uint32_t>::max(); // -> 4294967295
OSM guarantees for a max of 256*4 bytes per string, so a uint16_t
would be fine here. Other strings not coming from osm might need more.
include/util/timezones.hpp
Outdated
using local_time_t = std::pair<polygon_t, struct tm>; | ||
|
||
std::function<struct tm(const point_t &)> LoadLocalTimesRTree(const std::string &tz_shapes_filename, | ||
std::time_t utc_time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dup. with timezoner class above?
98cf68d
to
9bdbdcd
Compare
9bdbdcd
to
5771d99
Compare
CMakeLists.txt
Outdated
@@ -524,6 +524,11 @@ else() | |||
ENDIF() | |||
ENDIF() | |||
|
|||
find_package(Shapefile) # package libshp-dev | |||
if(NOT SHAPEFILE_FOUND) | |||
message(FATAL_ERROR "Shapefile library (libshp-dev) was not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still on the todo list @karenzshea ?
include/extractor/serialization.hpp
Outdated
} | ||
restriction.flags.is_only = is_only; | ||
|
||
restrictions.push_back(restriction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you take a look at the docs for push_back
http://en.cppreference.com/w/cpp/container/vector/push_back
you will see there are two overloads - one taking a const T&
and the second taking a T&&
. The first is what you call here: it will make a copy. But you know you don't need the restriction
's contents anymore. In the next iteration loop it will be gone.
What you can do then, is to call std::move(restriction)
. The trick about move is, all it does is casting a T
to a T&&
. So what does it do? It allows the push_back(T&&)
overload to be called instead, and this one is free to "steal" the restriction
's content.
^ this is called move semantics; you can transfer "ownership" via std::move
and avoid a copy this way.
(Note: a moved-from object is more or less empty, you must not access it again, as in: you can't e.g. move twice)
(Also please tell me if comments like this are helpful, or if I should just point out a move is missing here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arghhh sorry comments like these are very helpful 🙏 You already explained this to me once though, I was using an std::move
before and then I must have rebased it away. Thanks for pointing out again!
include/extractor/serialization.hpp
Outdated
writer.WriteOne(restriction.to); | ||
writer.WriteOne(restriction.flags.is_only); | ||
writer.WriteElementCount64(restriction.condition.size()); | ||
for (auto &c : restriction.condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can c
be a const&
? We write from it to a file and the restriction
object is already const
.
include/util/timezones.hpp
Outdated
@@ -0,0 +1,21 @@ | |||
#ifndef TIMEZONES_HPP | |||
#define TIMEZONES_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for adding the include guard; for these generic'ish names maybe prefix with OSRM_
- the problem is, if we include a completely independent project and it also has a TIMEZONES_HPP
guard, we will clash here.
else | ||
{ | ||
// save unconditional turn restriction to memory, for use in ebg later | ||
unconditional_turn_restrictions.push_back(restriction_container.restriction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move here and above, too?
src/extractor/restriction_parser.cpp
Outdated
} | ||
|
||
// push back a copy of turn restriction | ||
parsed_restrictions.push_back(boost::make_optional(restriction_container)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just about to say, can we make_optional(move(resr_cont))
, and then checked optional's source:
http://www.boost.org/doc/libs/1_63_0/boost/optional/optional.hpp
Looks like the overload for make_optional(Args&& ...)
is missing. It's in C++17's stdlib, though:
http://en.cppreference.com/w/cpp/utility/optional/make_optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, bug, ticketed boostorg/optional 30
if you want to avoid a copy here you can do
optional<YourType> tmp{std::move(v)};
r.push_back(std::move(tmp));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this, but requires removing TryParse
's constness as a function
src/extractor/restriction_parser.cpp
Outdated
// push back a copy of turn restriction | ||
parsed_restrictions.push_back(boost::make_optional(restriction_container)); | ||
|
||
return std::move(parsed_restrictions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C++ guarantees for function-local variables to be moved out by default. If you add a move you penalize further optimization.
By which I mean
T fn() {
T tmp;
..
return tmp; // will be moved out if possible
}
return move(tmp)
would be bad for optimization here. Also there are situations in which you have to move:
struct S {
T gn() {
return tmp; // will _not_ move tmp out, as tmp is not a function-local variable
}
T tmp;
};
(will crash if you cann gn() more than once, since a moved-from object is in a valid but unspecified state)
There are some articles out there, search for penalizing move, RVO, e.g.
{ | ||
for (boost::optional<InputRestrictionContainer> r : result_res) | ||
{ | ||
resulting_restrictions.push_back(r); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (T t : res)
will make a copy for every r in res; which is fine if you want to make a move(r)
here
{ | ||
return hash_val(std::get<0>(t), std::get<1>(t)); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does hash_val come from? Also can you just use boost::hash
they properly do the hash mixing part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comes from our own implementation of std_hash
. Boost::hash implementation for tuples doesn't work across all the systems we test on #3962 (comment)
// TODO make this into a function | ||
LookupTable<std::tuple<NodeID, NodeID>, NodeID> is_only_lookup; | ||
std::unordered_set<std::tuple<NodeID, NodeID, NodeID>, | ||
std::hash<std::tuple<NodeID, NodeID, NodeID>>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you'd need to change to boost::hash
and it should magically just work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work, boost::hash implementation for tuples doesn't work across all the systems we test on #3962 (comment)
314da31
to
b2bce1b
Compare
@karenzshea i have pushed a fix for msvs. please don't split tests into conditionals and non-conditionals: if there is no TZ information EDIT: smth like
EDIT2: kicked out my commit: still |
c7a84e6
to
edca66e
Compare
edca66e
to
cb7e7a4
Compare
c83e9de
to
bbd2107
Compare
fa219ce
to
cbcbaf2
Compare
CMakeLists.txt
Outdated
@@ -337,6 +337,7 @@ elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "MSVC") | |||
# using Visual Studio C++ | |||
set(BOOST_COMPONENTS ${BOOST_COMPONENTS} zlib) | |||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj") # avoid compiler error C1128 from scripting_environment_lua.cpp | |||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zm200") # avoid compiler error C1060 due to Boost.Spirit on AppVeyor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O, this is leftover from msvc checks, please remove the line
@oxidase @karenzshea I really disagree about removing testing with a shapefile here. There should be no silent fallback to UTC, a shapefile should be a hard requirement for conditional turns. If shipping the original shapefile is a problem we need to ship our own. Splitting the testing is ugly but can't be avoided with optional dependencies. |
@TheMarex i don't see any problem in including dummy TZ shapefile, but keeping 50Mb binary data is not good anyway because just shapefile loading in debug takes more than 5 seconds. |
4da0582
to
a520cb0
Compare
a520cb0
to
a3e7d85
Compare
Does this introduce requirement of GeoJSON timezone file during contraction step of the processing flow? Does this look like a valid way to disable the updater step during contraction?
Otherwise, it throws |
@mloskot This introduced an optional requirement of a GeoJSON timezone file. Whether or not conditional turn restriction updating is applied during contraction/customization depends onthe presence of the You can disable conditional turn restriction application by disabling the |
Issue
#3414
Initial pass at this will only support conditional turns not conditional speed limits.
So far, this branch contains updates for
Tasklist
internal_to_external_node_map
toUpdateTurnPenalties
functionUpdateTurnPenalties
for restricted turnsRequirements / Relations
After this first pass is done, clean up tasks:
cc @TheMarex @oxidase