Grand Unified Obstacle Treatment #7130

MarcelloPerathoner · 2025-03-15T10:55:37Z

Issue

Currently OSRM:

does not take into account stop signs, give-way signs, and crossings,
does not take into account traffic calming,
does not take into account designated turning points,
uses separate code-paths for traffic signals and barriers.

This patch

implements stop signs and give-way signs,
- understands 'minor' stop signs,
- can infer stop direction from adjacent edge lengths,
implements designated turning points,
allows for multiple obstacles at a single node,
implements a single code-path for all kinds of obstacles like: traffic signals, stop
signs, barriers, traffic calming, and turning points,
- reduces the (already crowded) signature of many functions.

Related work

Similar to pull requests: #6426, #6153, #4828. Thanks to their authors, who gave many ideas.

Fixes issue: #7089

Implementation description

A class ObstacleMap singleton is injected globally into the LUA scripting environment.
The ObstacleMap offers functions to add and query different kinds of 'obstacle' nodes.
(For want of a better name since 'waypoint' was already taken.)

The LUA process_node() function (repeatedly) calls ObstacleMap::add() to register
discovered obstacles. This function is thread safe. The data is directly stored in the
final place and not moved again after.

A compatibilty layer for process_node() has been provided to emulate the current
behaviour: to return result.barrier and result.traffic_signal.

The LUA process_turn() function can call ObstacleMap::any() and ObstacleMap::get() to
retrieve registered obstacles.

The length of the road has been added to the ExtractionTurnLeg LUA table and legs for
the start and target roads have been added to ExtractionTurn (a parameter of process_turn()).

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments

DennisOSRM · 2025-03-15T18:26:52Z

Thanks so much for the pull request. This looks very nice, indeed. I love the simplification of the code, the documentation, and the added tests look well written at first glance. Once CI passes, I will do a deeper review.

From the first impression, this patch has a high chance of getting merged. Well done. 👍

DennisOSRM

Looks pretty good to me. Left some minor comments that should be easy to address. The only bigger question is around the use of the extern keyword which should be avoided. Again, thanks so much for the contribution.

docs/src/content.js

DennisOSRM · 2025-03-16T12:58:52Z

include/extractor/extraction_turn.hpp

    RoadPriorityClass::Enum priority_class;
    bool is_incoming;
    bool is_outgoing;
 };

+extern const std::vector<ExtractionTurnLeg> NO_OTHER_ROADS;


Is the extern keyword needed here? We should avoid it, if possible, since it implies a global variable.

DennisOSRM · 2025-03-16T13:02:58Z

include/extractor/obstacles.hpp

+//
+// This may be a completely impassable obstacle like a barrier, a temporary obstacle
+// like a traffic light or a stop sign, or an obstacle that just slows you down like a
+// traffic calming bump.  The obstacle may be present in both directions or in one


Suggested change

// traffic calming bump. The obstacle may be present in both directions or in one

// traffic calming bump. The obstacle may be present in both directions or in one

DennisOSRM · 2025-03-16T13:03:53Z

include/extractor/obstacles.hpp

+//
+struct Obstacle
+{
+    // clang-format off


How bad does it look with formatting on?

DennisOSRM · 2025-03-16T13:04:28Z

include/extractor/obstacles.hpp

+
+    // The type of an obstacle
+    // Note: must be kept in sync with scripting_environment_lua.cpp
+    enum class Type : uint16_t


Any chance to factor this out into the same header?

I can try. But then it is the same story with all enums passed to sol / LUA.

yeah, then let's not overthink it. We can leave the enum as is

I was able to pull it into the obstacles.cpp file but not into the header. In the header I had to use a constexpr, but the Microsoft compiler choked on it.

DennisOSRM · 2025-03-16T17:44:09Z

include/extractor/obstacles.hpp

+    // pass SPECIAL_NODEID as 'from' to get all obstacles at 'to'
+    // 'type' can be a bitwise-or combination of Obstacle::Type
+    using GetType =
+        boost::any_range<Obstacle, boost::forward_traversal_tag, Obstacle, std::ptrdiff_t>;


Any chance to implement this without boost?

sol does not recognize a std::ranges::subrange as a container. It did recognize a boost::any_range though. As a compromise I used a std::vector hoping the performance hit is not too bad.

DennisOSRM · 2025-03-16T17:45:40Z

profiles/lib/obstacles.lua

@@ -0,0 +1,53 @@
+-- Assigns obstacle value to node as defined by


Suggested change

-- Assigns obstacle value to node as defined by

-- Assigns obstacle value to nodes as defined by

DennisOSRM · 2025-03-16T17:46:46Z

profiles/lib/obstacles.lua

+function Obstacles.process_node(profile, node)
+    local highway = node:get_value_by_key("highway")
+    if highway then
+        local typ = obstacle_type[highway]


should the variable name be type?

I thought type was reserved in LUA, but it is not. Will change.

DennisOSRM · 2025-03-16T17:50:31Z

src/extractor/obstacles.cpp

+
+void ObstacleMap::fixupNodes(const NodeIDVector &node_ids)
+{
+    const NodeIDVector::const_iterator begin = node_ids.cbegin();


perhaps use the auto keyword?

src/extractor/obstacles.cpp

DennisOSRM

👍 well done

MarcelloPerathoner · 2025-03-18T22:40:31Z

src/extractor/scripting_environment_lua.cpp

@@ -99,11 +99,12 @@ void handle_lua_error(const sol::protected_function_result &luares)
    const auto msg = luaerr.what();
    if (msg != nullptr)
    {
-        std::cerr << msg << "\n";


I think this is why some tests are flaky. std::cerr is not thread-safe.

* Grand Unified Obstacle Treatment * add changelog * Address comments * fix formatting * fix ci * fix ci * fix ci * simplify ObstacleMap::get

MarcelloPerathoner added 2 commits March 15, 2025 10:03

Grand Unified Obstacle Treatment

9a68fac

add changelog

9752526

DennisOSRM reviewed Mar 16, 2025

View reviewed changes

MarcelloPerathoner added 3 commits March 18, 2025 12:04

Address comments

d5126db

fix formatting

cc56054

fix ci

8a96cdb

DennisOSRM approved these changes Mar 18, 2025

View reviewed changes

MarcelloPerathoner added 2 commits March 18, 2025 19:18

fix ci

0d1cdef

fix ci

ea9c912

MarcelloPerathoner commented Mar 18, 2025

View reviewed changes

simplify ObstacleMap::get

8ca9959

DennisOSRM merged commit 7ee6245 into Project-OSRM:master Mar 19, 2025
21 checks passed

bevanjkay mentioned this pull request Apr 21, 2025

osrm-backend 6.0.0 Homebrew/homebrew-core#220847

Merged

	// traffic calming bump. The obstacle may be present in both directions or in one
	// traffic calming bump. The obstacle may be present in both directions or in one

		@@ -0,0 +1,53 @@
		-- Assigns obstacle value to node as defined by

	-- Assigns obstacle value to node as defined by
	-- Assigns obstacle value to nodes as defined by

Grand Unified Obstacle Treatment #7130

Grand Unified Obstacle Treatment #7130

Uh oh!

Conversation

MarcelloPerathoner commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

This patch

Related work

Implementation description

Tasklist

Uh oh!

DennisOSRM commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DennisOSRM left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

DennisOSRM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarcelloPerathoner commented Mar 15, 2025 •

edited

Loading

DennisOSRM commented Mar 15, 2025 •

edited

Loading