Fix/roi offset correction - #33
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Walkthrough本PR恢复并重组运行时主循环，拆分装甲可视化为 solved_pnp 与 predicted 两个可视化器，引入并传播 ROI 偏移以修正裁剪坐标，调整配置参数并更新相关单元测试与少量可视化错误处理行为。 Changes
Sequence DiagramsequenceDiagram
participant Capturer
participant Identifier
participant PoseEstimator
participant Tracker
participant FireControl
participant Visualization
participant Throttler
Capturer->>Identifier: 获取帧
Identifier->>Identifier: 检测 2D 装甲
Identifier->>PoseEstimator: 提供 2D 装甲进行 PNP 求解
PoseEstimator->>Visualization: 可视化 solved_pnp_armors
Identifier->>Visualization: 可视化 predicted_armors
Tracker->>Tracker: 应用跟踪逻辑
Tracker->>Throttler: 请求节流决策
FireControl->>FireControl: 计算开火命令
FireControl->>Capturer: 发送/执行开火指令
Visualization->>Visualization: 更新并发送可视化帧
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/module/identifier/armor_detection.cpp (1)
117-127:⚠️ Potential issue | 🟡 Minor
noexcept函数中存在潜在的异常风险。静态分析工具指出
explain函数声明为noexcept，但final_result.push_back(armor)在内存分配失败时可能抛出std::bad_alloc。虽然在实际运行中发生的概率较低，但这违反了noexcept的契约。♻️ 建议的修复方案
方案一：移除
noexcept声明（推荐）- auto explain(ov::InferRequest& finished_request) const noexcept -> Armor2Ds override { + auto explain(ov::InferRequest& finished_request) const -> Armor2Ds override {方案二：使用
reserve预分配内存减少异常风险（已存在于 line 115）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/identifier/armor_detection.cpp` around lines 117 - 127, The function explain is declared noexcept but can throw when pushing into final_result (final_result.push_back(armor)) if allocation fails; remove the noexcept specifier from the explain function declaration/definition so the function can propagate std::bad_alloc, or alternatively ensure final_result cannot allocate (e.g. pre-reserve sufficient capacity before the loop where kept_points is used), but the preferred fix is to drop noexcept on explain; locate explain and its signature and update it accordingly (also adjust any callers or exception-safety contracts if necessary).
🧹 Nitpick comments (2)
src/runtime.cpp (2)
196-203: 条件判断可以简化。
armors_3d在 187 行已经检查过一次（用于可视化），然后在 196 行再次检查。可以合并逻辑以减少重复判断。♻️ 建议的简化
- if (armors_3d) { - auto transform = control_state.odom_to_camera_transform; - pose_estimator.set_odom_to_camera_transform(transform); - - armors_3d = pose_estimator.odom_to_camera(*armors_3d); - } else { + if (!armors_3d) { continue; } + auto transform = control_state.odom_to_camera_transform; + pose_estimator.set_odom_to_camera_transform(transform); + armors_3d = pose_estimator.odom_to_camera(*armors_3d);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime.cpp` around lines 196 - 203, The code redundantly checks armors_3d twice; simplify by merging the logic so the earlier null/empty check (the one used for visualization) also performs the odom-to-camera transform step: when armors_3d is valid, grab control_state.odom_to_camera_transform, call pose_estimator.set_odom_to_camera_transform(transform) and then replace armors_3d with pose_estimator.odom_to_camera(*armors_3d); otherwise skip both visualization and transform steps. Update the block around the existing armors_3d usage to remove the second if/else and ensure only one presence check controls both visualization and the calls to set_odom_to_camera_transform and odom_to_camera.
150-155:std::ignore = stream_guard是多余的。
scope_exit对象会在作用域结束时自动执行，无需显式使用std::ignore来"使用"它。编译器警告（如果有）可以通过[[maybe_unused]]属性来抑制。♻️ 建议的简化
- auto stream_guard = std::experimental::scope_exit { [&] { + [[maybe_unused]] auto stream_guard = std::experimental::scope_exit { [&] { if (visualization.initialized()) { visualization.send_image(*image); } } }; - std::ignore = stream_guard;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime.cpp` around lines 150 - 155, Remove the redundant "std::ignore = stream_guard" statement and instead mark the scope_exit variable to avoid unused-variable warnings; replace "auto stream_guard = std::experimental::scope_exit { [&] { ... } };" with "[[maybe_unused]] auto stream_guard = std::experimental::scope_exit { [&] { if (visualization.initialized()) visualization.send_image(*image); } };" so the scope guard (stream_guard) still runs at scope exit but does not require the unnecessary std::ignore assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utility/rclcpp/visual/armor.cpp`:
- Around line 50-53: marker.scale is left uninitialized when config.device is
not kSmallArmor() or kLargeArmor(), causing unpredictable marker sizes;
initialize a safe default scale and log a warning in that branch. Update the
else branch handling config.device (where marker and config.device are used in
src/utility/rclcpp/visual/armor.cpp) to set marker.scale to a sensible default
(e.g., a fallback size) and emit a warning via the existing logging facility (or
std::cerr) so unknown devices (AERIAL, DART, RADAR, UNKNOWN) produce a
deterministic marker; alternatively initialize marker.scale to the default
before the conditional so all paths are covered.
---
Outside diff comments:
In `@src/module/identifier/armor_detection.cpp`:
- Around line 117-127: The function explain is declared noexcept but can throw
when pushing into final_result (final_result.push_back(armor)) if allocation
fails; remove the noexcept specifier from the explain function
declaration/definition so the function can propagate std::bad_alloc, or
alternatively ensure final_result cannot allocate (e.g. pre-reserve sufficient
capacity before the loop where kept_points is used), but the preferred fix is to
drop noexcept on explain; locate explain and its signature and update it
accordingly (also adjust any callers or exception-safety contracts if
necessary).
---
Nitpick comments:
In `@src/runtime.cpp`:
- Around line 196-203: The code redundantly checks armors_3d twice; simplify by
merging the logic so the earlier null/empty check (the one used for
visualization) also performs the odom-to-camera transform step: when armors_3d
is valid, grab control_state.odom_to_camera_transform, call
pose_estimator.set_odom_to_camera_transform(transform) and then replace
armors_3d with pose_estimator.odom_to_camera(*armors_3d); otherwise skip both
visualization and transform steps. Update the block around the existing
armors_3d usage to remove the second if/else and ensure only one presence check
controls both visualization and the calls to set_odom_to_camera_transform and
odom_to_camera.
- Around line 150-155: Remove the redundant "std::ignore = stream_guard"
statement and instead mark the scope_exit variable to avoid unused-variable
warnings; replace "auto stream_guard = std::experimental::scope_exit { [&] { ...
} };" with "[[maybe_unused]] auto stream_guard = std::experimental::scope_exit {
[&] { if (visualization.initialized()) visualization.send_image(*image); } };"
so the scope guard (stream_guard) still runs at scope exit but does not require
the unnecessary std::ignore assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8963ebf2-db07-4a37-ae40-2e9e36148a62
📒 Files selected for processing (7)
config/config.yamlsrc/kernel/visualization.cppsrc/module/identifier/armor_detection.cppsrc/runtime.cppsrc/utility/rclcpp/visual/armor.cpptest/model_infer.cpptest/solve_pnp.cpp
💤 Files with no reviewable changes (1)
- test/solve_pnp.cpp
creeper5820
approved these changes
Mar 11, 2026
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.
ROI偏移校正修复
概述
本PR修复并统一了装甲板检测中的ROI偏移校正机制，用roi_offset替代原先的按角落校正方式；同时对可视化组件与运行时流程进行了重构，并调整了若干配置与测试以支持ROI分段处理。
关键变更
装甲检测逻辑（src/module/identifier/armor_detection.cpp）
use_corner_correction（不再在Config/YAML中暴露）。可视化重构（src/kernel/visualization.cpp）
运行时重建（src/runtime.cpp）
配置调整（config/config.yaml）
测试更新（test/model_infer.cpp & test/solve_pnp.cpp）
use_corner_correction: false。其他小改动
影响范围