Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体反馈:
NodeDetail上新增的jump_back字段没有包含在MEO_TOJSON宏调用中,因此它的值不会被序列化;建议将其添加到该宏(以及对应的反序列化逻辑中,如果适用),以保持结果 schema 的一致性。- 基于
next和get_pipeline_data来解析jump_back的逻辑现在位于run_next中,而run只消费传递过来的标志位;如果在其他地方也需要这段解析逻辑,建议将其提取到一个小的辅助函数中,以避免未来行为出现偏差。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `jump_back` field on `NodeDetail` is not included in the `MEO_TOJSON` macro call, so its value will not be serialized; consider adding it there (and to any corresponding deserialization, if applicable) to keep the result schema consistent.
- The logic to resolve `jump_back` based on `next` and `get_pipeline_data` now lives in `run_next` while `run` only consumes the propagated flag; if this resolution is needed elsewhere, consider extracting it into a small helper to avoid future drift in behavior.
## Individual Comments
### Comment 1
<location path="source/include/Common/TaskResultTypes.h" line_range="47-49" />
<code_context>
MaaRecoId reco_id = MaaInvalidId;
MaaActId action_id = MaaInvalidId;
bool completed = false;
+ bool jump_back = false;
MEO_TOJSON(node_id, name, reco_id, action_id, completed);
};
</code_context>
<issue_to_address>
**issue (bug_risk):** Include `jump_back` in the serialization macro to keep JSON in sync with the struct
`jump_back` was added to `NodeDetail` but not to `MEO_TOJSON(...)`, so it won’t be included in JSON and the wire format will diverge from the struct. Please add `jump_back` to the macro arguments so JSON-based consumers stay consistent with the in-memory representation.
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈持续改进代码评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
jump_backfield onNodeDetailis not included in theMEO_TOJSONmacro call, so its value will not be serialized; consider adding it there (and to any corresponding deserialization, if applicable) to keep the result schema consistent. - The logic to resolve
jump_backbased onnextandget_pipeline_datanow lives inrun_nextwhilerunonly consumes the propagated flag; if this resolution is needed elsewhere, consider extracting it into a small helper to avoid future drift in behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `jump_back` field on `NodeDetail` is not included in the `MEO_TOJSON` macro call, so its value will not be serialized; consider adding it there (and to any corresponding deserialization, if applicable) to keep the result schema consistent.
- The logic to resolve `jump_back` based on `next` and `get_pipeline_data` now lives in `run_next` while `run` only consumes the propagated flag; if this resolution is needed elsewhere, consider extracting it into a small helper to avoid future drift in behavior.
## Individual Comments
### Comment 1
<location path="source/include/Common/TaskResultTypes.h" line_range="47-49" />
<code_context>
MaaRecoId reco_id = MaaInvalidId;
MaaActId action_id = MaaInvalidId;
bool completed = false;
+ bool jump_back = false;
MEO_TOJSON(node_id, name, reco_id, action_id, completed);
};
</code_context>
<issue_to_address>
**issue (bug_risk):** Include `jump_back` in the serialization macro to keep JSON in sync with the struct
`jump_back` was added to `NodeDetail` but not to `MEO_TOJSON(...)`, so it won’t be included in JSON and the wire format will diverge from the struct. Please add `jump_back` to the macro arguments so JSON-based consumers stay consistent with the in-memory representation.
</issue_to_address>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
This PR fixes a bug (issue #1162) where using jump_back and anchor together in a pipeline node produced incorrect behavior. The root cause was that the old run() code re-resolved anchor references (to detect jump_back) after run_next() had already updated the anchor map — meaning the anchor lookup used stale/updated values, causing the wrong node to be matched as the jump-back source.
Changes:
jump_backdetection is moved intorun_next()and evaluated before the action (and before anchors are updated), ensuring the correct pre-update anchor state is used.- The
jump_backresult is propagated throughNodeDetailso the caller (run()) can use it without repeating anchor lookups.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
source/include/Common/TaskResultTypes.h |
Adds jump_back field to NodeDetail struct to carry the jump-back decision from run_next() to its caller. |
source/MaaFramework/Task/PipelineTask.cpp |
Moves jump_back resolution into run_next() before action execution/anchor update; simplifies run() to use node_detail.jump_back directly. |
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.
fix #1162
Summary by Sourcery
调整流水线任务节点的处理方式,使其能够在与锚点(anchor)更新相互独立的情况下确定并传递回跳(jump-back)行为,从而修复在同时使用
jump_back和anchor时出现的不正确行为。Bug Fixes:
jump_back和anchor配置时,对回跳行为处理不正确的问题。Enhancements:
NodeDetail结果传递jump_back状态,使下游处理能够感知并基于回跳决策进行相应处理。Original summary in English
Summary by Sourcery
Adjust pipeline task node processing to determine and propagate jump-back behavior independently of anchor updates, fixing incorrect behavior when jump_back and anchor are used together.
Bug Fixes:
Enhancements:
Summary by Sourcery
通过将回跳(jump-back)解析与锚点(anchor)更新解耦,并通过节点执行结果传播回跳状态,修复在流水线任务与锚点一起使用时的不正确回跳处理问题。
Bug 修复:
jump_back和锚点配置时,纠正流水线节点的回跳行为。增强功能:
NodeDetail传播jump_back状态,使下游的流水线处理能够对回跳决策作出响应。Original summary in English
Summary by Sourcery
Fix incorrect jump-back handling in pipeline tasks when used together with anchors by decoupling jump-back resolution from anchor updates and propagating jump-back state via node execution results.
Bug Fixes:
Enhancements: