refactor: robot model and tracker - #64
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough本次变更将图像输入统一改为 Changes自瞄追踪与火控 V2 重构
Sequence Diagram(s)sequenceDiagram
participant AutoAim
participant TrackerV2
participant RobotModel
participant FireControllerV2
participant ShootEvaluator
AutoAim->>TrackerV2: store(armors/lightbars)
AutoAim->>TrackerV2: execute(timestamp)
TrackerV2->>RobotModel: predict(dt) / correct(armors, lightbars)
RobotModel-->>TrackerV2: state()/converge()
TrackerV2-->>AutoAim: Trackable
AutoAim->>FireControllerV2: aim(trackable)
FireControllerV2->>FireControllerV2: 迭代求解飞行时间/yaw/pitch
FireControllerV2->>ShootEvaluator: evaluate(yaw,pitch,center,attack)
ShootEvaluator-->>FireControllerV2: shoot决策
FireControllerV2-->>AutoAim: Aimed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 9
🧹 Nitpick comments (5)
src/kernel/identifier.cpp (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value统一枚举命名 同一字段前后混用
DeviceId::OUTPOST/BASE和ArmorGenre::OUTPOST/BASE，虽然ArmorGenre只是DeviceId的别名，但建议统一一种写法，避免阅读时来回切换。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/identifier.cpp` around lines 48 - 52, The enum usage in the identifier filtering logic is inconsistent because the same field is referenced with both DeviceId::OUTPOST/BASE and ArmorGenre::OUTPOST/BASE; update the checks in the identifier-related code to use one naming style consistently, preferably the symbol already used in this function’s detected armor loop, so readers do not have to switch between aliases while following the classification logic.src/module/capturer/local_video.cpp (1)
184-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value可选：提取重复的“克隆 mat + 复制 timestamp”模式。
该模式在 158-162 行（
optional<Image>→unique_ptr<Image>）与本处（unique_ptr<Image>→optional<Image>）中各出现一次，可考虑提取一个小工具函数以减少重复，但收益有限，可视情况推迟。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/module/capturer/local_video.cpp` around lines 184 - 189, The repeated “clone mat plus copy timestamp” logic appears in both the optional-to-unique_ptr and unique_ptr-to-optional conversions in local_video.cpp, so consider extracting that behavior into a small helper to reduce duplication. Update the code around the image assignment in the relevant conversion paths to use a shared utility for copying an Image while preserving the timestamp, and keep the existing behavior unchanged if you choose not to refactor now.src/kernel/visualization.hpp (1)
7-11: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value移除重复的
cv::Mat头文件 include：src/utility/image/drawable.hpp已经引入了<opencv2/core/mat.hpp>，这里在src/kernel/visualization.hpp里再直接包含一次是冗余的；如果没有独立依赖，建议删掉这行。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/visualization.hpp` around lines 7 - 11, Remove the redundant OpenCV mat include from visualization.hpp since drawable.hpp already brings in cv::Mat; update the includes in src/kernel/visualization.hpp so it relies on the existing transitive dependency unless a direct dependency is actually required. Verify the remaining includes still satisfy all symbols used by visualization.hpp, especially those coming from drawable.hpp and pimpl.hpp.Source: Coding guidelines
src/kernel/tracker.cpp (1)
222-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议对
compute_distance2cam_x的调用使用具名初始化构造Transform。
{ camera.translation, camera.orientation }依赖Transform成员的声明顺序，而附近代码（如 189-192、205-208 行的update_transform）均采用{.translation = ..., .orientation = ...}具名初始化方式。为保持一致性并避免未来Transform成员重排导致隐蔽错误，建议这里也改为具名初始化。♻️ 建议修改
- const auto distance_score = - compute_distance2cam_x({ camera.translation, camera.orientation }, p); + const auto distance_score = compute_distance2cam_x( + { .translation = camera.translation, .orientation = camera.orientation }, p);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/tracker.cpp` around lines 222 - 232, The call to compute_distance2cam_x in the calculate lambda is using positional aggregate initialization for Transform, which is inconsistent with the nearby update_transform usage and can break if Transform member order changes. Update that call to construct the Transform with named fields for translation and orientation, keeping the initialization explicit and aligned with the rest of tracker.cpp.src/utility/math/robot.cpp (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win补齐
<numbers>和<ranges>的显式包含。std::numbers::pi和std::views::iota直接依赖这两个头文件，不要靠间接包含。建议修改
`#include` <eigen3/Eigen/Geometry> +#include <numbers> +#include <ranges>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utility/math/robot.cpp` at line 4, The include list in robot.cpp is missing the explicit standard headers required by the symbols used elsewhere: add direct includes for <numbers> and <ranges> alongside the existing Eigen geometry include. Update the file’s top-level include section so std::numbers::pi and std::views::iota are satisfied by their own headers rather than relying on indirect transitive includes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kernel/auto_aim.cpp`:
- Around line 91-95: The early continue in auto_aim.cpp after populating
armor2ds/lightbar2ds skips the later current_command = cmd write-back, which can
leave stale should_shoot/should_track state and also prevents tracker_v2 from
seeing non-empty lightbar2ds. Update the auto-aim loop around
armor2ds/lightbar2ds handling so empty armor frames still flow through the state
update path, or explicitly assign AutoAimState::kInvalid() before any continue;
keep the current_command update and the tracker_v2/AutoAimState logic aligned
with the auto aim state machine.
In `@src/kernel/fire_control.cpp`:
- Around line 45-53: In Impl’s YAML constructor in fire_control.cpp, add
validation after config.serialize succeeds to reject invalid fire-control
settings before they reach runtime. Specifically, ensure config.bullet_speed is
greater than 0 and that both values in config.attack_window are non-negative and
ordered sensibly before converting with util::deg2rad. If any check fails, throw
std::runtime_error from Impl with a clear FireControllerV2 message so
FireControllerV2::Impl never allows zero/negative bullet_speed or invalid
attack_window values into the rest of the logic.
- Around line 135-156: The convergence check in fire_control.cpp is using the
updated fly_time value, so the comparison in the iteration loop always becomes
zero and exits immediately. Update the loop in the fire_control logic to compare
the newly solved fly_time against the previous iteration’s fly_time before
assigning it, using the existing TrajectorySolution::solve result and the
fly_time variable so the kEpsilon break condition reflects real convergence.
- Around line 161-182: The Aimed result in fire_control.cpp is assigning both
aim_yaw and raw_yaw from the same post-offset yaw, so raw_yaw no longer
preserves the original solved angle. Update the yaw handling in the logic around
util::normalize_angle and shoot_evaluator.evaluate so you keep a pre-offset raw
yaw value before applying config.offset_yaw, then return that preserved value
through Aimed::raw_yaw while keeping the corrected value in Aimed::aim_yaw.
In `@src/kernel/identifier.cpp`:
- Around line 139-146: The length filter in identifier.cpp needs a zero-length
guard because armor_length can be 0 for a degenerate armor box, causing the
ratio check to produce inf/NaN and let invalid cases pass. Update the logic
around the armor_length/detected_length comparison to explicitly skip or reject
when armor_length is zero before performing the division, and keep the existing
threshold check in the same filter block.
In `@src/kernel/tracker.cpp`:
- Around line 60-68: The invalid fallback_color path in TrackerV2 currently only
logs an error and lets the constructor continue with the default
ArmorColor::DARK, which causes silent misconfiguration. Update the TrackerV2
construction logic in tracker.cpp so that the invalid config.fallback_color case
is treated as a hard failure, similar to serialize-related failures: after
logging, immediately stop construction by throwing an exception or otherwise
propagating an error to the caller. Use the existing symbols
config.fallback_color, track_color, logging.error, and the TrackerV2 constructor
block to locate the fix.
- Around line 184-219: The `robot_stamps` update at the end of the `tracker.cpp`
loop is re-creating entries that were just erased in the init-failure and
`model.diverged()` paths. Adjust the control flow around `robot_models`,
`robot_stamps`, and `id` so the timestamp is only refreshed when the
corresponding model still exists, and skip the final `robot_stamps[id] =
timestamp` after any branch that erases state.
In `@src/module/tracker/model/robot.hpp`:
- Around line 1-4: The robot.hpp header is missing direct standard library
includes for the types it uses, so it currently relies on transitive includes
from utility/robot/armor.hpp and utility/pimpl.hpp. Add the needed standard
headers directly in robot.hpp for std::vector, std::span, and std::numbers,
keeping the existing includes intact so the header is self-sufficient regardless
of include order or upstream changes.
In `@src/module/tracker/trackable.hpp`:
- Around line 3-10: The Trackable header uses std::unique_ptr and std::vector
without including their standard headers, so add explicit includes for those
types in trackable.hpp rather than relying on utility/clock.hpp or
utility/math/linear.hpp. Update the Trackable struct’s includes so the Unique
and Points aliases are self-contained and no longer depend on include order.
---
Nitpick comments:
In `@src/kernel/identifier.cpp`:
- Around line 48-52: The enum usage in the identifier filtering logic is
inconsistent because the same field is referenced with both
DeviceId::OUTPOST/BASE and ArmorGenre::OUTPOST/BASE; update the checks in the
identifier-related code to use one naming style consistently, preferably the
symbol already used in this function’s detected armor loop, so readers do not
have to switch between aliases while following the classification logic.
In `@src/kernel/tracker.cpp`:
- Around line 222-232: The call to compute_distance2cam_x in the calculate
lambda is using positional aggregate initialization for Transform, which is
inconsistent with the nearby update_transform usage and can break if Transform
member order changes. Update that call to construct the Transform with named
fields for translation and orientation, keeping the initialization explicit and
aligned with the rest of tracker.cpp.
In `@src/kernel/visualization.hpp`:
- Around line 7-11: Remove the redundant OpenCV mat include from
visualization.hpp since drawable.hpp already brings in cv::Mat; update the
includes in src/kernel/visualization.hpp so it relies on the existing transitive
dependency unless a direct dependency is actually required. Verify the remaining
includes still satisfy all symbols used by visualization.hpp, especially those
coming from drawable.hpp and pimpl.hpp.
In `@src/module/capturer/local_video.cpp`:
- Around line 184-189: The repeated “clone mat plus copy timestamp” logic
appears in both the optional-to-unique_ptr and unique_ptr-to-optional
conversions in local_video.cpp, so consider extracting that behavior into a
small helper to reduce duplication. Update the code around the image assignment
in the relevant conversion paths to use a shared utility for copying an Image
while preserving the timestamp, and keep the existing behavior unchanged if you
choose not to refactor now.
In `@src/utility/math/robot.cpp`:
- Line 4: The include list in robot.cpp is missing the explicit standard headers
required by the symbols used elsewhere: add direct includes for <numbers> and
<ranges> alongside the existing Eigen geometry include. Update the file’s
top-level include section so std::numbers::pi and std::views::iota are satisfied
by their own headers rather than relying on indirect transitive includes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a8d673e-d299-4baa-b725-51f0efe6eb4e
📒 Files selected for processing (118)
CMakeLists.txtconfig/config.yamlsrc/component.cppsrc/kernel/auto_aim.cppsrc/kernel/capturer.cppsrc/kernel/capturer.hppsrc/kernel/fire_control.cppsrc/kernel/fire_control.hppsrc/kernel/identifier.cppsrc/kernel/identifier.hppsrc/kernel/pose_estimator.cppsrc/kernel/pose_estimator.hppsrc/kernel/tracker.cppsrc/kernel/tracker.hppsrc/kernel/visualization.cppsrc/kernel/visualization.hppsrc/module/capturer/common.hppsrc/module/capturer/hikcamera.cppsrc/module/capturer/hikcamera.hppsrc/module/capturer/local_video.cppsrc/module/capturer/local_video.hppsrc/module/capturer/video.cppsrc/module/debug/action_throttler.hppsrc/module/fire_control/armor_selector.cppsrc/module/fire_control/armor_selector.hppsrc/module/fire_control/planner/mpc_solver.cppsrc/module/fire_control/planner/mpc_solver.hppsrc/module/fire_control/planner/mpc_types.hppsrc/module/fire_control/planner/reference_builder.cppsrc/module/fire_control/planner/reference_builder.hppsrc/module/fire_control/planner/trajectory_planner.cppsrc/module/fire_control/planner/trajectory_planner.hppsrc/module/fire_control/shoot_evaluator.cppsrc/module/fire_control/shoot_evaluator.hppsrc/module/fire_control/target_solver.cppsrc/module/fire_control/target_solver.hppsrc/module/fire_control/trajectory_solution.cppsrc/module/fire_control/trajectory_solution.hppsrc/module/fire_control/types.hppsrc/module/identifier/adjacency_lightbar.cppsrc/module/identifier/adjacency_lightbar.hppsrc/module/identifier/armor_detection.cppsrc/module/identifier/armor_detection.hppsrc/module/identifier/green_light.cppsrc/module/identifier/green_light.hppsrc/module/identifier/lightbar.cppsrc/module/identifier/lightbar.hppsrc/module/identifier/preprocess.cppsrc/module/identifier/preprocess.hppsrc/module/perception/rotation_estimator.cppsrc/module/pipeline/common.hppsrc/module/predictor/model/robot.hppsrc/module/predictor/outpost/robot_state.cppsrc/module/predictor/outpost/robot_state.hppsrc/module/predictor/outpost/snapshot.cppsrc/module/predictor/outpost/snapshot.hppsrc/module/predictor/regular/ekf_parameter.hppsrc/module/predictor/regular/robot_state.cppsrc/module/predictor/regular/robot_state.hppsrc/module/predictor/regular/snapshot.cppsrc/module/predictor/regular/snapshot.hppsrc/module/predictor/robot_state.cppsrc/module/predictor/robot_state.hppsrc/module/predictor/snapshot.cppsrc/module/predictor/snapshot.hppsrc/module/tracker/armor_filter.cppsrc/module/tracker/armor_filter.hppsrc/module/tracker/decider.cppsrc/module/tracker/decider.hppsrc/module/tracker/model/outpost.cppsrc/module/tracker/model/outpost.hppsrc/module/tracker/model/robot.cppsrc/module/tracker/model/robot.hppsrc/module/tracker/trackable.hppsrc/utility/coroutine/common.hppsrc/utility/duck_type.hppsrc/utility/image/drawable.cppsrc/utility/image/drawable.hppsrc/utility/image/image.cppsrc/utility/image/image.details.hppsrc/utility/image/image.hppsrc/utility/image/text.cppsrc/utility/image/text.hppsrc/utility/math/camera.cppsrc/utility/math/camera.hppsrc/utility/math/corners_optimizor.cppsrc/utility/math/corners_optimizor.hppsrc/utility/math/linear.hppsrc/utility/math/robot.cppsrc/utility/math/robot.hppsrc/utility/math/solve_pnp/pnp_solution.cppsrc/utility/math/solve_pnp/pnp_solution.hppsrc/utility/math/solve_pnp/solve_pnp.hppsrc/utility/math/tiny_mpc_solver.hppsrc/utility/rclcpp/node.cppsrc/utility/rclcpp/node.details.hppsrc/utility/rclcpp/visual/lightbar.cppsrc/utility/rclcpp/visual/movable.hppsrc/utility/rclcpp/visual/scalar.cppsrc/utility/robot/armor.hppsrc/utility/serializable.hppsrc/utility/shared/context.hppsrc/utility/shared/interprocess.hppsrc/utility/yaml/tf.hpptest/CMakeLists.txttest/duck_type.cpptest/model_infer.cpptest/pipeline.cpptest/serializable.cpptest/solve_pnp.cpptest/tiny_mpc_solver.cpptool/cxx/CMakeLists.txttool/cxx/hikcamera_test.cpptool/cxx/ov_model_status.cpptool/cxx/rotation_estimator.cpptool/cxx/test_outpost_ekf.cpptool/cxx/test_robot_ekf.cpptool/cxx/visualization.cpp
💤 Files with no reviewable changes (38)
- src/module/fire_control/target_solver.hpp
- src/module/predictor/outpost/robot_state.hpp
- src/module/fire_control/planner/trajectory_planner.hpp
- src/utility/image/image.details.hpp
- src/module/predictor/model/robot.hpp
- test/tiny_mpc_solver.cpp
- src/module/fire_control/planner/mpc_types.hpp
- src/module/predictor/snapshot.hpp
- src/utility/math/tiny_mpc_solver.hpp
- src/module/predictor/outpost/snapshot.cpp
- src/module/pipeline/common.hpp
- src/module/tracker/armor_filter.hpp
- src/module/fire_control/planner/trajectory_planner.cpp
- src/module/predictor/robot_state.hpp
- src/module/predictor/regular/robot_state.cpp
- src/module/predictor/regular/robot_state.hpp
- src/utility/image/image.cpp
- src/module/predictor/outpost/snapshot.hpp
- src/module/tracker/decider.hpp
- src/module/predictor/regular/snapshot.hpp
- test/pipeline.cpp
- src/module/fire_control/armor_selector.hpp
- src/module/fire_control/types.hpp
- src/module/fire_control/target_solver.cpp
- src/module/predictor/regular/ekf_parameter.hpp
- src/module/tracker/armor_filter.cpp
- src/module/fire_control/planner/reference_builder.hpp
- src/module/predictor/outpost/robot_state.cpp
- src/module/fire_control/planner/reference_builder.cpp
- src/module/predictor/robot_state.cpp
- CMakeLists.txt
- src/module/fire_control/armor_selector.cpp
- src/module/fire_control/planner/mpc_solver.cpp
- src/module/fire_control/planner/mpc_solver.hpp
- src/module/predictor/regular/snapshot.cpp
- test/CMakeLists.txt
- src/module/predictor/snapshot.cpp
- src/module/tracker/decider.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/kernel/tracker.cpp (2)
224-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win不要把记忆目标的评分直接清零。
Line 231 让当前
track_genre的score恒为 0；只要旧模型仍converge()，距离更优的新目标也无法被选中。建议使用非零权重/偏置，而不是消除距离项。🐛 建议修复
- const auto memory_score = (id == track_genre) ? 0 : 1; + constexpr auto kTrackedTargetBias = 0.8; + const auto memory_score = (id == track_genre) ? kTrackedTargetBias : 1.0; return distance_score * memory_score;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/tracker.cpp` around lines 224 - 234, The scoring logic in calculate currently zeroes out the score for the remembered target via memory_score, which makes track_genre always win regardless of distance. Update calculate in tracker.cpp so the remembered target keeps a non-zero weight or bias instead of multiplying the distance score by 0; keep the distance term active and adjust the memory preference only as a soft preference tied to track_genre.
76-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win不要用主点反推图像宽高
camera.camera_matrix[0][2] * 2/[1][2] * 2只在主点正好居中时才成立；一旦有裁剪、缩放或非居中标定，边界过滤就会误删/误放目标。改为直接使用实际帧的cols/rows。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/tracker.cpp` around lines 76 - 88, The boundary filtering in store() is deriving image size from camera.camera_matrix principal points, which is only valid when the principal point is centered. Update the logic to use the actual frame dimensions (cols/rows from the image source or frame object used by tracker.cpp) instead of kWidth/kHeight computed from camera.camera_matrix, and keep the min_x/max_x/min_y/max_y margin checks unchanged.
🧹 Nitpick comments (2)
src/kernel/tracker.cpp (2)
239-241: 🎯 Functional Correctness | 🔵 Trivial请把这个
@FIXME作为显式跟踪项。这里描述的一帧误识别即可
converge()会直接影响出靶结果；如果本 PR 暂不修，建议至少补 issue 或回归用例。需要的话我可以帮忙拆测试用例。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/tracker.cpp` around lines 239 - 241, The current `@FIXME` in Tracker::converge handling is a tracked defect and should be made explicit rather than left as an inline note. Update the logic around outpost->converge() in src/kernel/tracker.cpp to either create a formal issue/reference for this one-frame false-positive convergence case or add a regression test that captures the misrecognized-robot scenario; if the fix is deferred, leave a clear tracked link or test name tied to Tracker and converge so it is visible and actionable.
184-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win复用
try_emplace返回的迭代器。 这里已经拿到元素引用，继续写robot_models[id]会重复查找；直接用it->second更简洁。♻️ 建议调整
- robot_models.try_emplace(id, robot_config); - robot_models[id].update_camera( + auto [it, inserted] = robot_models.try_emplace(id, robot_config); + auto& model = it->second; + model.update_camera( std::bit_cast<std::array<double, 9>>(camera.camera_matrix), camera.distort_coeff); - robot_models[id].update_transform({ + model.update_transform({ .translation = camera.translation, .orientation = camera.orientation, }); - if (!robot_models[id].init(target.armor2ds)) { + if (!model.init(target.armor2ds)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/tracker.cpp` around lines 184 - 193, The tracker logic in the robot_models insertion path is doing redundant map lookups after try_emplace. Reuse the iterator returned by robot_models.try_emplace in the containing block of the update_camera, update_transform, and init calls, and switch all robot_models[id] accesses to the inserted element via it->second to avoid повторное поиск and keep the code concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/kernel/tracker.cpp`:
- Around line 224-234: The scoring logic in calculate currently zeroes out the
score for the remembered target via memory_score, which makes track_genre always
win regardless of distance. Update calculate in tracker.cpp so the remembered
target keeps a non-zero weight or bias instead of multiplying the distance score
by 0; keep the distance term active and adjust the memory preference only as a
soft preference tied to track_genre.
- Around line 76-88: The boundary filtering in store() is deriving image size
from camera.camera_matrix principal points, which is only valid when the
principal point is centered. Update the logic to use the actual frame dimensions
(cols/rows from the image source or frame object used by tracker.cpp) instead of
kWidth/kHeight computed from camera.camera_matrix, and keep the
min_x/max_x/min_y/max_y margin checks unchanged.
---
Nitpick comments:
In `@src/kernel/tracker.cpp`:
- Around line 239-241: The current `@FIXME` in Tracker::converge handling is a
tracked defect and should be made explicit rather than left as an inline note.
Update the logic around outpost->converge() in src/kernel/tracker.cpp to either
create a formal issue/reference for this one-frame false-positive convergence
case or add a regression test that captures the misrecognized-robot scenario; if
the fix is deferred, leave a clear tracked link or test name tied to Tracker and
converge so it is visible and actionable.
- Around line 184-193: The tracker logic in the robot_models insertion path is
doing redundant map lookups after try_emplace. Reuse the iterator returned by
robot_models.try_emplace in the containing block of the update_camera,
update_transform, and init calls, and switch all robot_models[id] accesses to
the inserted element via it->second to avoid повторное поиск and keep the code
concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 387ef271-9a2f-4c70-9c64-9c115210e476
📒 Files selected for processing (4)
src/kernel/auto_aim.cppsrc/kernel/fire_control.cppsrc/kernel/tracker.cppsrc/module/tracker/model/robot.hpp
💤 Files with no reviewable changes (1)
- src/kernel/auto_aim.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/module/tracker/model/robot.hpp
- src/kernel/fire_control.cpp
This was referenced Jul 3, 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.
概括
引入了基于 2D 观测的全新机器人 EKF 模型，效果拔群，性能优越，顺手重构了
snapshot体系，trackable将取而代之重构止步于火控模块，
planner或等效优化器将于后续的 PR 引入本次变更围绕“机器人模型与跟踪器重构”展开，核心是将旧的
Image/Snapshot/Tracker/FireControl链路替换为基于cv::Mat/Trackable/TrackerV2/FireControllerV2的新体系，并在火控模块上止步于“瞄准/开火决策”（规划器/优化器后续另提）。机器人 EKF 模型与 trackable 抽象
RobotModel（EKF）用于机器人状态估计，提供相机/坐标更新、init/predict/correct、收敛/发散判断与装甲/灯条几何输出。Trackable及Ins<State>封装层：统一aimpoints()、direction()、timestamp()、jump_into()、clone()等接口，支撑 2D→3D→预测后的通用跟踪对象。跟踪系统重构：TrackerV2
TrackerV2替换旧的Tracker/Decider/ArmorFilter/Snapshot等体系，改为update_* / clean / store(...) / execute(Timestamp)的缓存-执行管线。update_track_color / update_track_genre设置跟踪条件，并在execute阶段更新前哨站与机器人模型、做目标选择，输出Trackable::Unique与addition()（tracked 2D/3D 与可选 lightbar 列表）。converge()、State增加index等）。火控系统重构：FireControllerV2（去除旧 MPC/规划链路）
FireControllerV2替换旧FireControl，核心接口从solve(snapshot, gimbal_state)迁移为aim(trackable)。trackable.direction()估计飞行时间、jump_into(shoot_delay + fly_time)后对aimpoints()进行装甲候选筛选与选择（带攻击窗口/角度容差），迭代TrajectorySolution求解得到 yaw/pitch，并应用偏置。ShootEvaluator的命令字段改为center/attack命名体系；同时移除了旧版 MPC/轨迹规划/目标求解器等模块（如TinyMPC、MpcAxisSolver、ReferenceBuilder、TrajectoryPlanner、TargetSolver相关接口与实现）。AutoAim主流程对应调整为仅输出新的cmd.should_track/should_shoot与 yaw/pitch、中心/攻击点等信息。图像数据流与接口大规模迁移（Image → cv::Mat）
utility::Image { cv::Mat mat; Timestamp timestamp; }，并移除Image::Details/details().mat等依赖；多个模块（捕获、预处理、识别、位姿估计、可视化、绘图/文字）改为直接以cv::Mat作为入参。identifier/pose_estimator/adjacency_lightbar/armor_detection/green_light/...等接口参数类型切换为cv::Mat。capturer.tick(newest->mat)、Visualization::update_image(cv::Mat&)等）。与配置/构建/测试的同步调整
config/config.yaml更新了tracker、fire_control等字段命名与结构（移除旧敌方颜色配置、弹道/偏置命名调整、新增/替换攻击窗口与相关参数块等）。CMakeLists.txt移除FetchContent拉取/启用TinyMPC，并调整依赖链接（tinympcstatic→openvino::runtime）。tool/cxx/test_robot_ekf.cpp），同时修正 outpost ekf 测试使用的新头文件来源。