refactor: make MovementManager click limits configurable (#2013)#2116
refactor: make MovementManager click limits configurable (#2013)#2116JoeCardoso13 wants to merge 2 commits into
Conversation
MAX_CLICK_HORIZONTAL_M / MAX_CLICK_VERTICAL_M were module-level constants. Move them into MovementManagerConfig so they can be set per-run for users rendering very large maps. Defaults unchanged. Addresses the first item of dimensionalOS#2013.
Greptile SummaryThis PR moves
Confidence Score: 5/5This PR is safe to merge. It is a mechanical refactor with no logic changes at default settings, protected by a Pydantic field constraint and backed by 7 passing tests. The change is narrow and well-contained: two constants move into config, the consuming code is updated in one place, defaults are identical, and both previously flagged issues (missing lower-bound guard and missing vertical-axis test coverage) have been addressed by this revision. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[clicked_point stream] --> B[_on_click]
B --> C{all coords finite?}
C -- No --> D[log warning: invalid click\nreturn]
C -- Yes --> E{abs x or y > max_click_horizontal_m\nor abs z > max_click_vertical_m?}
E -- Yes --> F[log warning: out-of-range click\nreturn]
E -- No --> G[publish to way_point]
G --> H[publish to goal]
style D fill:#f99,stroke:#c00
style F fill:#f99,stroke:#c00
style H fill:#9f9,stroke:#090
Reviews (2): Last reviewed commit: "refactor: constrain click limits to posi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Clamp clicked goals so a stray click can't send the planner toward infinity in | ||
| # rerun. Configurable for people rendering very large maps. | ||
| max_click_horizontal_m: float = 500.0 | ||
| max_click_vertical_m: float = 50.0 |
There was a problem hiding this comment.
max_click_horizontal_m or max_click_vertical_m to 0.0 or any negative value and silently disable all click-based navigation — abs(msg.x) > 0.0 is True for every non-zero coordinate, so every click gets dropped without an obvious error. A Pydantic Field(gt=0) constraint would surface the mistake at config-parse time instead of at runtime.
| # Clamp clicked goals so a stray click can't send the planner toward infinity in | |
| # rerun. Configurable for people rendering very large maps. | |
| max_click_horizontal_m: float = 500.0 | |
| max_click_vertical_m: float = 50.0 | |
| # Clamp clicked goals so a stray click can't send the planner toward infinity in | |
| # rerun. Configurable for people rendering very large maps. | |
| max_click_horizontal_m: float = Field(default=500.0, gt=0) | |
| max_click_vertical_m: float = Field(default=50.0, gt=0) |
| def test_click_limit_rejects_when_lowered(manager_and_captured): | ||
| """A click within the default range is rejected once the limit is lowered below it.""" | ||
| manager, captured = manager_and_captured | ||
| manager.config.max_click_horizontal_m = 10.0 | ||
| manager._on_click(_click(x=50.0, y=0.0, z=0.0)) | ||
| assert captured.goal == [] | ||
| assert captured.way_point == [] | ||
|
|
||
|
|
||
| def test_click_limit_accepts_when_raised(manager_and_captured): | ||
| """A click beyond the default range is accepted once the limit is raised above it.""" | ||
| manager, captured = manager_and_captured | ||
| manager.config.max_click_horizontal_m = 1000.0 | ||
| far_click = _click(x=600.0, y=0.0, z=0.0) | ||
| manager._on_click(far_click) | ||
| assert captured.goal == [far_click] | ||
| assert captured.way_point == [far_click] |
There was a problem hiding this comment.
max_click_horizontal_m; max_click_vertical_m has no equivalent coverage for the dynamic-config path. A click like _click(x=0.0, y=0.0, z=30.0) with max_click_vertical_m lowered to 10.0 would be a symmetric counterpart and would guard against a future regression where only one field gets wired to self.config.
…imit Address review feedback on dimensionalOS#2116: add Field(gt=0) so a zero/negative config override can't silently reject every click, and add a test exercising the dynamic max_click_vertical_m path.
1 participant
Problem
MAX_CLICK_HORIZONTAL_M/MAX_CLICK_VERTICAL_MinMovementManagerweremodule-level constants, so users rendering very large maps couldn't raise the
click-to-goal clamp without editing source.
Solution
Move both into
MovementManagerConfig(defaults unchanged: 500.0 / 50.0), andread them via
self.config.*in_on_click. No behavior change at defaultsettings.
Addresses the first item of #2013. The other two bullets of #2013
(locks → async, publish-only-to
goal) are intentionally left out to keep thisPR single-purpose;
Breaking Changes
None.
How to Test
uv run pytest dimos/navigation/movement_manager/test_movement_manager.py(7 pass, including 2 new tests covering lowered/raised limits).
Contributor License Agreement