fix: register And/Or sub-recognition results in runtime cache#1281
Merged
fix: register And/Or sub-recognition results in runtime cache#1281
Conversation
…tom access And/Or sub-recognition results were only stored in the recognizer's shared sub_best_box_ maps, invisible to Custom recognizers that call context.run_reco() (which creates a new RecognitionTask/Recognizer). Now each successful sub-recognition registers a node detail and latest-node entry in the runtime cache, enabling subsequent subs (e.g. Custom -> run_reco -> roi referencing a sibling) to resolve PreTask ROI references. Closes #1267 Made-with: Cursor
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我在这里给出了一些高层次的反馈：
and_()和or_()中的节点注册逻辑是重复的；可以考虑抽取一个小的辅助函数（例如register_sub_result_in_cache(const RecoResult&)），把NodeDetail的创建和latest_node的更新集中到一起。- 将
TaskBase::generate_node_id()设为静态函数会改变其生命周期与所有权语义；请仔细检查它依赖的任何内部状态（例如计数器、线程安全机制等），在从多个TaskBase实例以及从Recognizer调用时，是否依然保持正确。 - 在
and_()分支中，你现在会将每个成功的子识别结果注册到运行时缓存，即使整体的And之后失败；请确认这是不是期望的行为。如果不是，考虑将缓存注册延后到确认整个组合识别都成功之后再进行。
供 AI Agents 使用的提示词
Please address the comments from this code review:
## Overall Comments
- The node registration logic in `and_()` and `or_()` is duplicated; consider extracting a small helper (e.g., `register_sub_result_in_cache(const RecoResult&)`) to centralize the `NodeDetail` creation and `latest_node` update.
- Making `TaskBase::generate_node_id()` static changes its lifetime/ownership semantics; double-check that any internal state it relies on (e.g., counters, thread-safety mechanisms) is still correct when called from multiple `TaskBase` instances and from `Recognizer`.
- In the `and_()` path you now register each successful sub-recognition to the runtime cache even if the overall `And` later fails; confirm this is desirable behavior and, if not, consider deferring cache registration until you know the whole composite recognition has succeeded.帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've left some high level feedback:
- The node registration logic in
and_()andor_()is duplicated; consider extracting a small helper (e.g.,register_sub_result_in_cache(const RecoResult&)) to centralize theNodeDetailcreation andlatest_nodeupdate. - Making
TaskBase::generate_node_id()static changes its lifetime/ownership semantics; double-check that any internal state it relies on (e.g., counters, thread-safety mechanisms) is still correct when called from multipleTaskBaseinstances and fromRecognizer. - In the
and_()path you now register each successful sub-recognition to the runtime cache even if the overallAndlater fails; confirm this is desirable behavior and, if not, consider deferring cache registration until you know the whole composite recognition has succeeded.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The node registration logic in `and_()` and `or_()` is duplicated; consider extracting a small helper (e.g., `register_sub_result_in_cache(const RecoResult&)`) to centralize the `NodeDetail` creation and `latest_node` update.
- Making `TaskBase::generate_node_id()` static changes its lifetime/ownership semantics; double-check that any internal state it relies on (e.g., counters, thread-safety mechanisms) is still correct when called from multiple `TaskBase` instances and from `Recognizer`.
- In the `and_()` path you now register each successful sub-recognition to the runtime cache even if the overall `And` later fails; confirm this is desirable behavior and, if not, consider deferring cache registration until you know the whole composite recognition has succeeded.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes runtime-cache visibility of And/Or sub-recognition results so Custom recognizers can reference sibling node results via roi string names (PreTask/Anchor), addressing #1267.
Changes:
- Register
NodeDetail+latest_nodeentries for successfulAnd/Orsub-recognitions in the runtime cache. - Expose node-id generation as a
staticfunction onTaskBaseto support node-id allocation outside an instance context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| source/MaaFramework/Task/TaskBase.h | Makes generate_node_id() static to enable shared node-id generation. |
| source/MaaFramework/Task/Component/Recognizer.cpp | After sub-recognition hits in and_() / or_(), writes node metadata into the runtime cache for name-based lookup. |
Comment on lines
+398
to
+401
| auto& cache = tasker_->runtime_cache(); | ||
| auto sub_node_id = TaskBase::generate_node_id(); | ||
| cache.set_node_detail(sub_node_id, NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id }); | ||
| cache.set_latest_node(res.name, sub_node_id); |
There was a problem hiding this comment.
TaskBase::generate_node_id() is declared under protected: in TaskBase, so calling it here from Recognizer (a non-derived class) will fail to compile due to access control. Consider moving node-id generation to a publicly accessible API (e.g., Tasker/RuntimeCache) or make TaskBase::generate_node_id() public if that’s acceptable.
| if (res.box.has_value() && !res.name.empty()) { | ||
| auto& cache = tasker_->runtime_cache(); | ||
| auto sub_node_id = TaskBase::generate_node_id(); | ||
| cache.set_node_detail(sub_node_id, NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id }); |
There was a problem hiding this comment.
The NodeDetail registered for this successful sub-recognition leaves completed at its default false. Since latest_node(name) will now point at this NodeDetail, APIs like MaaTaskerGetNodeDetail will report it as not completed even though recognition succeeded. Set completed = true (and keep action_id invalid) for these sub-recognition entries.
Suggested change
| cache.set_node_detail(sub_node_id, NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id }); | |
| cache.set_node_detail( | |
| sub_node_id, | |
| NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id, .completed = true }); |
|
|
||
| if (has_hit && !res.name.empty()) { | ||
| auto& cache = tasker_->runtime_cache(); | ||
| auto sub_node_id = TaskBase::generate_node_id(); |
There was a problem hiding this comment.
TaskBase::generate_node_id() is declared under protected: in TaskBase, so calling it here from Recognizer (a non-derived class) will fail to compile due to access control. Consider moving node-id generation to a publicly accessible API (e.g., Tasker/RuntimeCache) or make TaskBase::generate_node_id() public if that’s acceptable.
Suggested change
| auto sub_node_id = TaskBase::generate_node_id(); | |
| auto sub_node_id = res.reco_id; |
| if (has_hit && !res.name.empty()) { | ||
| auto& cache = tasker_->runtime_cache(); | ||
| auto sub_node_id = TaskBase::generate_node_id(); | ||
| cache.set_node_detail(sub_node_id, NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id }); |
There was a problem hiding this comment.
The NodeDetail registered for this successful sub-recognition leaves completed at its default false. Since latest_node(name) will now point at this NodeDetail, APIs like MaaTaskerGetNodeDetail will report it as not completed even though recognition succeeded. Set completed = true (and keep action_id invalid) for these sub-recognition entries.
Suggested change
| cache.set_node_detail(sub_node_id, NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id }); | |
| cache.set_node_detail( | |
| sub_node_id, | |
| NodeDetail { .node_id = sub_node_id, .name = res.name, .reco_id = res.reco_id, .completed = true }); |
| ActionResult run_action(const RecoResult& reco, const PipelineData& data); | ||
| cv::Mat screencap(); | ||
| MaaNodeId generate_node_id(); | ||
| static MaaNodeId generate_node_id(); |
There was a problem hiding this comment.
generate_node_id() is still protected on TaskBase. If the goal is to allow Recognizer (and other non-TaskBase components) to allocate node IDs, this will need to be moved to a public API or exposed via a different helper (otherwise call sites like Recognizer.cpp won’t compile).
Made-with: Cursor
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层次的反馈：
- 将
TaskBase::generate_node_id()设为静态函数会改变它的生命周期和共享语义；请再次确认其实现依然是线程安全的，并且在现在被全局共享的情况下，仍然能保证在所有任务/识别器之间的节点 ID 唯一性。 register_sub_result_in_cache会在每次子识别成功时无条件调用set_latest_node(res.name, ...)，这可能会覆盖同名节点已有的 latest-node 条目；请考虑是否需要对子节点名称进行区分或增加保护，以避免在缓存中意外覆盖无关节点。
给 AI 代理的提示
请处理本次代码审查中的评论：
## 总体评论
- 将 `TaskBase::generate_node_id()` 设为静态函数会改变它的生命周期和共享语义；请再次确认其实现依然是线程安全的，并且在现在被全局共享的情况下，仍然能保证在所有任务/识别器之间的节点 ID 唯一性。
- `register_sub_result_in_cache` 会在每次子识别成功时无条件调用 `set_latest_node(res.name, ...)`，这可能会覆盖同名节点已有的 latest-node 条目；请考虑是否需要对子节点名称进行区分或增加保护，以避免在缓存中意外覆盖无关节点。
## 逐条评论
### 评论 1
<location path="source/MaaFramework/Task/Component/Recognizer.h" line_range="50" />
<code_context>
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
+ void register_sub_result_in_cache(const RecoResult& res);
private:
</code_context>
<issue_to_address>
**建议：** 鉴于 `register_sub_result_in_cache` 看起来是一个内部辅助函数，建议将其设为 private。
将其保持为私有辅助函数可以减少 `Recognizer` 的公共 API 表面，并避免鼓励外部调用方依赖内部缓存细节，从而让未来的重构更加容易。
建议的实现方式：
```c
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
private:
void register_sub_result_in_cache(const RecoResult& res);
bool debug_mode() const;
```
如果当前在 `Recognizer` 外部有对 `register_sub_result_in_cache` 的调用，这些调用点需要被重构为使用更高层的公共 API，因为该方法现在是私有的。
</issue_to_address>请帮我变得更有用！欢迎在每条评论上点 👍 或 👎，我会根据反馈改进后续的审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Making
TaskBase::generate_node_id()static changes its lifetime and sharing semantics; please double-check that its implementation remains thread-safe and still guarantees uniqueness across all tasks/recognizers now that it’s globally shared. register_sub_result_in_cacheunconditionally callsset_latest_node(res.name, ...)for every successful sub-recognition, which may overwrite existing latest-node entries for the same name; consider whether sub-node names should be disambiguated or guarded to avoid clobbering unrelated nodes in the cache.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Making `TaskBase::generate_node_id()` static changes its lifetime and sharing semantics; please double-check that its implementation remains thread-safe and still guarantees uniqueness across all tasks/recognizers now that it’s globally shared.
- `register_sub_result_in_cache` unconditionally calls `set_latest_node(res.name, ...)` for every successful sub-recognition, which may overwrite existing latest-node entries for the same name; consider whether sub-node names should be disambiguated or guarded to avoid clobbering unrelated nodes in the cache.
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Task/Component/Recognizer.h" line_range="50" />
<code_context>
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
+ void register_sub_result_in_cache(const RecoResult& res);
private:
</code_context>
<issue_to_address>
**suggestion:** Consider making `register_sub_result_in_cache` private since it appears to be an internal helper.
Keeping this as a private helper reduces the public API surface of `Recognizer` and avoids encouraging external callers to depend on an internal caching detail, which makes future refactors easier.
Suggested implementation:
```c
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
private:
void register_sub_result_in_cache(const RecoResult& res);
bool debug_mode() const;
```
If `register_sub_result_in_cache` is currently called from outside `Recognizer`, those call sites will need to be refactored to use a higher-level public API instead, since the method is now private.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false); | ||
| std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best); | ||
| void save_draws(const std::string& node_name, const RecoResult& result) const; | ||
| void register_sub_result_in_cache(const RecoResult& res); |
Contributor
There was a problem hiding this comment.
建议： 鉴于 register_sub_result_in_cache 看起来是一个内部辅助函数，建议将其设为 private。
将其保持为私有辅助函数可以减少 Recognizer 的公共 API 表面，并避免鼓励外部调用方依赖内部缓存细节，从而让未来的重构更加容易。
建议的实现方式：
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
private:
void register_sub_result_in_cache(const RecoResult& res);
bool debug_mode() const;如果当前在 Recognizer 外部有对 register_sub_result_in_cache 的调用，这些调用点需要被重构为使用更高层的公共 API，因为该方法现在是私有的。
Original comment in English
suggestion: Consider making register_sub_result_in_cache private since it appears to be an internal helper.
Keeping this as a private helper reduces the public API surface of Recognizer and avoids encouraging external callers to depend on an internal caching detail, which makes future refactors easier.
Suggested implementation:
std::vector<cv::Rect> get_rois(const MAA_VISION_NS::Target& roi, bool use_best = false);
std::vector<cv::Rect> get_rois_from_pretask(const std::string& name, bool use_best);
void save_draws(const std::string& node_name, const RecoResult& result) const;
private:
void register_sub_result_in_cache(const RecoResult& res);
bool debug_mode() const;If register_sub_result_in_cache is currently called from outside Recognizer, those call sites will need to be refactored to use a higher-level public API instead, since the method is now private.
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
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
And/Orbeing unable to access sibling node recognition results viaroistring references (PreTask/Anchor)Root Cause
And/Orsub-recognition results were only stored in two places:sub_best_box_/sub_filtered_boxes_— shared amongRecognizercopies within the sameAnd/Or, but NOT accessible from a newRecognitionTaskreco_details_(by reco_id) — stored viaset_reco_detailinRecognizer::recognize(), but not retrievable by node nameWhen a Custom recognizer called
context.run_reco(), it created a newRecognitionTask→ newRecognizerwith emptysub_best_box_. The new recognizer'sget_rois_from_pretask("sibling_node")then triedruntime_cache().get_latest_node("sibling_node"), which also failed because no node detail or latest-node entry was ever registered forAnd/Orsub-recognitions.Fix
After each successful sub-recognition in
and_()/or_(), register aNodeDetailandlatest_nodeentry in the runtime cache. This allows any subsequentrun_recocall to find sibling results through the standardget_latest_node(name)→get_node_detail(node_id)→get_reco_result(reco_id)lookup chain.Also made
TaskBase::generate_node_id()staticsoRecognizercan use the shared node ID counter.Closes #1267
Summary by Sourcery
在运行时缓存中注册 And/Or 子节点的识别结果，以便同级识别器可以通过标准的节点查找方式访问这些结果。
Bug Fixes:
Enhancements:
TaskBase::generate_node_id()暴露为静态方法，以便识别器可以为子识别结果创建节点 ID。Original summary in English
Summary by Sourcery
Register recognition results of And/Or sub-nodes in the runtime cache so sibling recognizers can access them via standard node lookups.
Bug Fixes:
Enhancements:
Original summary in English
Summary by Sourcery
在运行时缓存中注册 And/Or 子节点的识别结果，以便同级识别器可以通过标准的节点查找方式访问这些结果。
Bug Fixes:
Enhancements:
TaskBase::generate_node_id()暴露为静态方法，以便识别器可以为子识别结果创建节点 ID。Original summary in English
Summary by Sourcery
Register recognition results of And/Or sub-nodes in the runtime cache so sibling recognizers can access them via standard node lookups.
Bug Fixes:
Enhancements: