Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些高层面的反馈：
- 新增的 HasSevereTurnDrift 守卫
if (session_->current_node_idx() > 1) return false;在第二个节点之后完全禁用了自适应漂移处理；如果你的意图只是缩小其适用范围，建议把条件绑定到距离 / 阶段 / 标志位，而不是硬编码在节点索引上，这样后续路段在合适情况下仍然可以受益。 - EstimateSprintSegmentDistance 目前在找到第一个符合条件的下一路点后就直接返回，所以最多只会考虑两段（当前→下一）；如果你希望自动冲刺更好地反映较长的直线路段，可以考虑在同一分区内，跨多个连续 RUN 路点累加距离，直到满足某个停止条件。
- 修改后的自动冲刺就绪条件移除了之前的一些保护措施（偏航约束、本地驾驶员状态、严格到达排除、转弯后的提交条件等）；如果这些场景对冲刺来说仍然有问题，可能值得有选择地重新引入部分检查，或者把这些逻辑编码进 EstimateSprintSegmentDistance 里以避免回归。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new HasSevereTurnDrift guard `if (session_->current_node_idx() > 1) return false;` completely disables adaptive drift handling after the second node; if the intent is to narrow its scope, consider tying this to a distance/phase/flag instead of a hard node index cut‑off so later route segments can still benefit when appropriate.
- EstimateSprintSegmentDistance currently returns after the first eligible next waypoint, so it only ever considers at most two legs (current→next); if you want auto sprint to better reflect longer straight segments, consider accumulating distances across multiple consecutive RUN waypoints in the same zone until a stopping condition is hit.
- The revised auto-sprint readiness condition drops several previous safeguards (yaw constraint, local driver state, strict-arrival exclusion, post-turn commit, etc.); if those scenarios are still problematic for sprinting, it may be worth selectively reintroducing some of those checks or encoding them into EstimateSprintSegmentDistance to avoid regressions.
## Individual Comments
### Comment 1
<location path="agent/cpp-algo/source/MapNavigator/navigation_state_machine.cpp" line_range="834" />
<code_context>
}
else {
- action_wrapper_->ClickKeySync(kKeyW, kExactTargetMoveWaitMs);
+ action_wrapper_->ClickKeySync(kKeyW, 60); // kExactTargetMoveWaitMs
}
}
</code_context>
<issue_to_address>
**nitpick:** Hardcoded magic number with a stale constant name comment can be confusing and brittle.
The literal `60` has replaced `kExactTargetMoveWaitMs`, but the trailing comment still references the removed constant, which suggests a dangling or misleading reference. Please either introduce a new, clearly named constant for this delay or remove the stale comment so the purpose of `60` is explicit and maintainable.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new HasSevereTurnDrift guard
if (session_->current_node_idx() > 1) return false;completely disables adaptive drift handling after the second node; if the intent is to narrow its scope, consider tying this to a distance/phase/flag instead of a hard node index cut‑off so later route segments can still benefit when appropriate. - EstimateSprintSegmentDistance currently returns after the first eligible next waypoint, so it only ever considers at most two legs (current→next); if you want auto sprint to better reflect longer straight segments, consider accumulating distances across multiple consecutive RUN waypoints in the same zone until a stopping condition is hit.
- The revised auto-sprint readiness condition drops several previous safeguards (yaw constraint, local driver state, strict-arrival exclusion, post-turn commit, etc.); if those scenarios are still problematic for sprinting, it may be worth selectively reintroducing some of those checks or encoding them into EstimateSprintSegmentDistance to avoid regressions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new HasSevereTurnDrift guard `if (session_->current_node_idx() > 1) return false;` completely disables adaptive drift handling after the second node; if the intent is to narrow its scope, consider tying this to a distance/phase/flag instead of a hard node index cut‑off so later route segments can still benefit when appropriate.
- EstimateSprintSegmentDistance currently returns after the first eligible next waypoint, so it only ever considers at most two legs (current→next); if you want auto sprint to better reflect longer straight segments, consider accumulating distances across multiple consecutive RUN waypoints in the same zone until a stopping condition is hit.
- The revised auto-sprint readiness condition drops several previous safeguards (yaw constraint, local driver state, strict-arrival exclusion, post-turn commit, etc.); if those scenarios are still problematic for sprinting, it may be worth selectively reintroducing some of those checks or encoding them into EstimateSprintSegmentDistance to avoid regressions.
## Individual Comments
### Comment 1
<location path="agent/cpp-algo/source/MapNavigator/navigation_state_machine.cpp" line_range="834" />
<code_context>
}
else {
- action_wrapper_->ClickKeySync(kKeyW, kExactTargetMoveWaitMs);
+ action_wrapper_->ClickKeySync(kKeyW, 60); // kExactTargetMoveWaitMs
}
}
</code_context>
<issue_to_address>
**nitpick:** Hardcoded magic number with a stale constant name comment can be confusing and brittle.
The literal `60` has replaced `kExactTargetMoveWaitMs`, but the trailing comment still references the removed constant, which suggests a dangling or misleading reference. Please either introduce a new, clearly named constant for this delay or remove the stale comment so the purpose of `60` is explicit and maintainable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
agent/cpp-algo/source/MapNavigator/navigation_state_machine.cpp
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
优化 MapNavigator 的导航行为和诊断功能，简化终点定位逻辑，改进自动冲刺（auto-sprint）处理，并增强工具的健壮性与日志记录能力。
New Features:
Bug Fixes:
TRANSFER处理逻辑，使同一点的后续动作通过完整的航点抵达流程处理，并使用更新后的距离与传送门就绪检查。Enhancements:
ExactTargetRefine导航阶段及其相关配置，简化终点定位行为和状态管理。Original summary in English
Summary by Sourcery
Refine MapNavigator navigation behavior and diagnostics, simplifying terminal targeting, improving auto-sprint handling, and enhancing tooling robustness and logging.
New Features:
Bug Fixes:
Enhancements: