Issue 6171 Make MAX_COLLAPSE_DISTANCE configurable in the lua profile#7252
Issue 6171 Make MAX_COLLAPSE_DISTANCE configurable in the lua profile#7252kaligrafy wants to merge 1 commit intoProject-OSRM:masterfrom
Conversation
9a86d39 to
0283967
Compare
|
Thanks for your contribution, I have noticed that CHANGELOG.md is missing |
|
Hi! Can any maintainer check this PR, it has been a month with no review. Thanks! |
Implements configurable max_collapse_distance parameter to address issue Project-OSRM#6171. This allows precise control over turn instruction collapsing in guidance, particularly important for pedestrian routing where short crossings need to be preserved for safety analysis. Changes: * Add max_collapse_distance property to ProfileProperties with getter/setter * Expose parameter to Lua scripting environment for profile configuration * Add GetMaxCollapseDistance() method to DataFacade interface * Replace hardcoded MAX_COLLAPSE_DISTANCE constant with thread-local variable * Update guidance collapse functions to use configurable parameter * Add documentation and example profile demonstrating usage The parameter defaults to 30.0 meters (preserving existing behavior) but can now be customized in Lua profiles: ```lua function setup() return { properties = { max_collapse_distance = 10.0, -- Custom value in meters }, } end ``` This enables sepcific usages who requires precise pedestrian routing instructions by preventing short road crossings from being collapsed into longer segments. Fixes Project-OSRM#6171 Developed with the help of Claude Sonnet 4 from Anthropic AI.
|
I added the changelog entry as requested. Thanks! |
|
Bump! Is there some timeline for review or commenting PRs? Thanks! |
TheMarex
left a comment
There was a problem hiding this comment.
I'm guessing this was mostly done by an LLM without real supervision? Look it probably took me longer to review this then it took you to generate this. None of this makes sense and it is a super hacky implementation that no sane human would allow. If you can not review your contributions yourself, then do not submit them here please. I know it is frustrating when nobody fixes your issues, but slop is not the answer and only drains more resources from open source projects.
| @@ -0,0 +1,272 @@ | |||
| -- Test Foot profile with configurable max_collapse_distance | |||
There was a problem hiding this comment.
Feel free to modify the foot profile directly, no need to create a separate profile. 10m makes sense in general, the values was intended for cars.
| using namespace osrm::guidance; | ||
|
|
||
| // Thread-local storage for configurable max collapse distance | ||
| thread_local double current_max_collapse_distance = DEFAULT_MAX_COLLAPSE_DISTANCE; |
There was a problem hiding this comment.
So this looks like a hack introduced by an LLM to me? The correct fix would simply pass the max_collapse_distance to the functions as parameter.
|
|
||
| // OTHER IMPLEMENTATIONS | ||
| [[nodiscard]] RouteSteps collapseTurnInstructions(RouteSteps steps) | ||
| [[nodiscard]] RouteSteps collapseTurnInstructions(const datafacade::BaseDataFacade &facade, RouteSteps steps) |
There was a problem hiding this comment.
max_collapse_distance should also be a parameter here.
| @@ -0,0 +1,89 @@ | |||
| #include "extractor/profile_properties.hpp" | |||
There was a problem hiding this comment.
All of these tests are useless.
| @@ -0,0 +1,130 @@ | |||
| #include "engine/guidance/collapse_turns.hpp" | |||
There was a problem hiding this comment.
These tests are also useless, please remove.
I am really sorry and you are right. C++ is not one of the languages I master so I just tried using an LLM to generate the patch, thinking it was going to be very trivial and simple, but it was not (or at least the LLM made it more complicated). I will try to propose a better PR as soon as possible as this is a feature we need since a long time ago. Thanks and sorry for the inconvenience! |
|
Closing here because we have a replacement in the works. |
feat: Add configurable max_collapse_distance parameter to Lua profiles
Implements configurable max_collapse_distance parameter to address issue #6171.
This allows precise control over turn instruction collapsing in guidance,
particularly important for pedestrian routing where short crossings need
to be preserved for safety analysis.
Changes:
The parameter defaults to 30.0 meters (preserving existing behavior) but can
now be customized in Lua profiles:
This enables sepcific usages who requires precise
pedestrian routing instructions by preventing short road crossings from
being collapsed into longer segments.
#6171