Skip to content

fix(Task): get_target_rect 所有 target 类型均 normalize 负宽高#1372

Closed
ocsin1 wants to merge 1 commit into
MaaXYZ:mainfrom
ocsin1:fix/get_target_rect
Closed

fix(Task): get_target_rect 所有 target 类型均 normalize 负宽高#1372
ocsin1 wants to merge 1 commit into
MaaXYZ:mainfrom
ocsin1:fix/get_target_rect

Conversation

@ocsin1

@ocsin1 ocsin1 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

此前 normalize_rect 仅对 Region 类型生效。Self/PreTask/Anchor 类型 的 target_offset 若包含负宽高,会直接送入 std::clamp 变为为 0,导致
cv::Rect::empty() 命中和 "failed to get target rect" 错误。
示例:

{
    "1": {
        "roi": [
            790,
            308,
            154,
            18
        ],
        "action": "Click", // 这样会报错
        "target_offset": [
            0,
            -50,
            0,
            -30
        ]
    },
    "2": {
        "roi": [
            790,
            258,
            154,
            -12
        ],
        "action": "Click" // 直接点击偏移后位置不会
    }
}

将 normalize_rect 移至 offset 应用之后并对所有 target 类型生效,使
负宽高遵循 ROI 语义(取绝对值并反向调整 x/y),与 roi 字段行为一致。

Summary by Sourcery

Bug Fixes:

  • 防止在偏移量中具有负宽度/高度的目标矩形被折叠为空矩形,从而在所有目标类型中导致“failed to get target rect”错误。
Original summary in English

Summary by Sourcery

Bug Fixes:

  • Prevent target rectangles with negative width/height in offsets from collapsing to empty rectangles and causing "failed to get target rect" errors across all target types.

此前 normalize_rect 仅对 Region 类型生效。Self/PreTask/Anchor 类型
的 target_offset 若包含负宽高,会直接送入 std::clamp 变为为 0,导致
cv::Rect::empty() 命中和 "failed to get target rect" 错误。

将 normalize_rect 移至 offset 应用之后并对所有 target 类型生效,使
负宽高遵循 ROI 语义(取绝对值并反向调整 x/y),与 roi 字段行为一致。

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - 我发现了 1 个问题,并给出了一些整体反馈:

  • 现在 normalize_rect 已经在 offset 之后对所有 target 类型统一调用,那么在 Region 分支里的那个更早的 normalize_rect 调用就变得多余了,并且会导致该分支被归一化两次;建议移除第一次调用,只依赖 offset 之后的统一归一化,这样可以让行为更一致、逻辑更简单。
  • 如果 MAA_VISION_NS::normalize_rect 本身已经保证返回的矩形在图像范围内且非负,那么对 raw 额外调用的 std::clamp 可能是多余的;你可以考虑要么移除这些 clamp,要么写上注释说明为什么需要同时做归一化和 clamp,以避免后续产生困惑。
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Now that `normalize_rect` is applied after the offset to all target types, the earlier `normalize_rect` call inside the `Region` branch becomes redundant and causes a double-normalization for that case; consider removing the first call and relying on the unified post-offset normalization to keep behavior consistent and simpler.
- If `MAA_VISION_NS::normalize_rect` already guarantees in-bounds, non-negative rectangles, the extra `std::clamp` calls on `raw` may be unnecessary; you could consider either removing them or documenting why both normalization and clamping are needed to avoid future confusion.

## Individual Comments

### Comment 1
<location path="source/MaaFramework/Task/Component/ActionHelper.cpp" line_range="227-236" />
<code_context>
         raw = MAA_VISION_NS::normalize_rect(raw, image.cols, image.rows);
     }

-    int x = std::clamp(raw.x + target.offset.x, 0, image.cols);
-    int y = std::clamp(raw.y + target.offset.y, 0, image.rows);
-    int width = std::clamp(raw.width + target.offset.width, 0, image.cols - x);
-    int height = std::clamp(raw.height + target.offset.height, 0, image.rows - y);
+    raw.x += target.offset.x;
+    raw.y += target.offset.y;
+    raw.width += target.offset.width;
+    raw.height += target.offset.height;

-    return cv::Rect(x, y, width, height);
+    // 对所有 target 类型,offset 后 normalize 以支持负宽高(ROI 语义)
+    raw = MAA_VISION_NS::normalize_rect(raw, image.cols, image.rows);
+
+    raw.x = std::clamp(raw.x, 0, image.cols);
+    raw.y = std::clamp(raw.y, 0, image.rows);
+    raw.width = std::clamp(raw.width, 0, image.cols - raw.x);
+    raw.height = std::clamp(raw.height, 0, image.rows - raw.y);
+
+    return raw;
</code_context>
<issue_to_address>
**question (bug_risk):** Consider whether the second normalize call changes semantics for targets already normalized earlier.

Some target types already pass `raw` through `normalize_rect` earlier in this function (pre-offset). With this change they’ll now be normalized twice: once before and once after applying the offset, which is a behavioral change and may affect edge-adjacent rectangles.

Please check whether you can rely on a single normalization after applying offsets (and then clamping), and remove the earlier call if it’s redundant. That would simplify the logic and avoid subtle differences from double-normalization.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Original comment in English

Hey - I've found 1 issue, and left some high level feedback:

  • Now that normalize_rect is applied after the offset to all target types, the earlier normalize_rect call inside the Region branch becomes redundant and causes a double-normalization for that case; consider removing the first call and relying on the unified post-offset normalization to keep behavior consistent and simpler.
  • If MAA_VISION_NS::normalize_rect already guarantees in-bounds, non-negative rectangles, the extra std::clamp calls on raw may be unnecessary; you could consider either removing them or documenting why both normalization and clamping are needed to avoid future confusion.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Now that `normalize_rect` is applied after the offset to all target types, the earlier `normalize_rect` call inside the `Region` branch becomes redundant and causes a double-normalization for that case; consider removing the first call and relying on the unified post-offset normalization to keep behavior consistent and simpler.
- If `MAA_VISION_NS::normalize_rect` already guarantees in-bounds, non-negative rectangles, the extra `std::clamp` calls on `raw` may be unnecessary; you could consider either removing them or documenting why both normalization and clamping are needed to avoid future confusion.

## Individual Comments

### Comment 1
<location path="source/MaaFramework/Task/Component/ActionHelper.cpp" line_range="227-236" />
<code_context>
         raw = MAA_VISION_NS::normalize_rect(raw, image.cols, image.rows);
     }

-    int x = std::clamp(raw.x + target.offset.x, 0, image.cols);
-    int y = std::clamp(raw.y + target.offset.y, 0, image.rows);
-    int width = std::clamp(raw.width + target.offset.width, 0, image.cols - x);
-    int height = std::clamp(raw.height + target.offset.height, 0, image.rows - y);
+    raw.x += target.offset.x;
+    raw.y += target.offset.y;
+    raw.width += target.offset.width;
+    raw.height += target.offset.height;

-    return cv::Rect(x, y, width, height);
+    // 对所有 target 类型,offset 后 normalize 以支持负宽高(ROI 语义)
+    raw = MAA_VISION_NS::normalize_rect(raw, image.cols, image.rows);
+
+    raw.x = std::clamp(raw.x, 0, image.cols);
+    raw.y = std::clamp(raw.y, 0, image.rows);
+    raw.width = std::clamp(raw.width, 0, image.cols - raw.x);
+    raw.height = std::clamp(raw.height, 0, image.rows - raw.y);
+
+    return raw;
</code_context>
<issue_to_address>
**question (bug_risk):** Consider whether the second normalize call changes semantics for targets already normalized earlier.

Some target types already pass `raw` through `normalize_rect` earlier in this function (pre-offset). With this change they’ll now be normalized twice: once before and once after applying the offset, which is a behavioral change and may affect edge-adjacent rectangles.

Please check whether you can rely on a single normalization after applying offsets (and then clamping), and remove the earlier call if it’s redundant. That would simplify the logic and avoid subtle differences from double-normalization.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread source/MaaFramework/Task/Component/ActionHelper.cpp
@ocsin1

ocsin1 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

第一次 normalize 不多余吧,处理的是 Region raw 自带的负宽高

@ocsin1 ocsin1 marked this pull request as draft June 19, 2026 01:34
@ocsin1 ocsin1 closed this Jun 19, 2026
@ocsin1 ocsin1 deleted the fix/get_target_rect branch June 19, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant