Conversation
Contributor
Author
|
@sourcery-ai summary |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体反馈：
- CursorMoveSink 被注册为一个共享的单例 sink，但内部包含可变字段（dirty、imgW、imgH），却没有任何同步机制。如果有多个 tasker/上下文并发运行，就可能导致数据竞争；建议为每个 tasker 创建独立实例，或者使用互斥锁来保护这些状态。
- moveCursor 直接使用缓存的图像宽度/高度作为坐标，但大多数坐标系是从 0 开始的，所以你可能需要使用 (imgW-1, imgH-1)，或者确认这里不会尝试把光标移动到有效边界之外。
- OnTaskerTask 中输出捕获图像大小的 info 级别日志会在每次 tasker 启动时触发；如果这只是在调试阶段需要，建议降级到 debug 级别，或者加上开关，以避免在正常运行时产生过多日志。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CursorMoveSink is registered as a single shared sink but holds mutable fields (dirty, imgW, imgH) without any synchronization, which can lead to data races if multiple taskers/contexts run concurrently; consider per-tasker instances or protecting state with a mutex.
- moveCursor uses the cached image width/height directly as coordinates, but most coordinate systems are 0-based, so you may want to use (imgW-1, imgH-1) or otherwise confirm this cannot attempt to move the cursor outside the valid bounds.
- The info-level log that prints the captured image size in OnTaskerTask will fire on every tasker start; if this is only needed for debugging, consider lowering it to debug or gating it to avoid noisy logs in normal runs.
## Individual Comments
### Comment 1
<location path="agent/go-service/taskersink/cursormove/sink.go" line_range="17-18" />
<code_context>
+ imgH int32
+}
+
+func (s *CursorMoveSink) moveCursor(ctrl *maa.Controller) {
+ ctrl.PostTouchMove(0, s.imgW, s.imgH, 0).Wait()
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** moveCursor relies on imgW/imgH being initialized, but they may still be zero when called.
`s.imgW`/`s.imgH` are only set in `OnTaskerTask` after a successful `CacheImage()`. If `OnNodeNextList` runs first, or `CacheImage` fails/doesn’t run for some flows, these stay zero and the cursor is moved to `(0, 0)` instead of bottom-right. Please guard `moveCursor` with `imgW > 0 && imgH > 0` and/or lazily compute the bounds in `OnNodeNextList` when they’re not initialized.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈来改进评论质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- CursorMoveSink is registered as a single shared sink but holds mutable fields (dirty, imgW, imgH) without any synchronization, which can lead to data races if multiple taskers/contexts run concurrently; consider per-tasker instances or protecting state with a mutex.
- moveCursor uses the cached image width/height directly as coordinates, but most coordinate systems are 0-based, so you may want to use (imgW-1, imgH-1) or otherwise confirm this cannot attempt to move the cursor outside the valid bounds.
- The info-level log that prints the captured image size in OnTaskerTask will fire on every tasker start; if this is only needed for debugging, consider lowering it to debug or gating it to avoid noisy logs in normal runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CursorMoveSink is registered as a single shared sink but holds mutable fields (dirty, imgW, imgH) without any synchronization, which can lead to data races if multiple taskers/contexts run concurrently; consider per-tasker instances or protecting state with a mutex.
- moveCursor uses the cached image width/height directly as coordinates, but most coordinate systems are 0-based, so you may want to use (imgW-1, imgH-1) or otherwise confirm this cannot attempt to move the cursor outside the valid bounds.
- The info-level log that prints the captured image size in OnTaskerTask will fire on every tasker start; if this is only needed for debugging, consider lowering it to debug or gating it to avoid noisy logs in normal runs.
## Individual Comments
### Comment 1
<location path="agent/go-service/taskersink/cursormove/sink.go" line_range="17-18" />
<code_context>
+ imgH int32
+}
+
+func (s *CursorMoveSink) moveCursor(ctrl *maa.Controller) {
+ ctrl.PostTouchMove(0, s.imgW, s.imgH, 0).Wait()
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** moveCursor relies on imgW/imgH being initialized, but they may still be zero when called.
`s.imgW`/`s.imgH` are only set in `OnTaskerTask` after a successful `CacheImage()`. If `OnNodeNextList` runs first, or `CacheImage` fails/doesn’t run for some flows, these stay zero and the cursor is moved to `(0, 0)` instead of bottom-right. Please guard `moveCursor` with `imgW > 0 && imgH > 0` and/or lazily compute the bounds in `OnNodeNextList` when they’re not initialized.
</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
该 PR 为 Win32 控制器引入光标移动事件接收器（CursorMoveSink），用于在动作执行后将鼠标光标移出画面，减少光标遮挡导致的识别干扰，并在 go-service 启动注册阶段自动启用该 sink。
Changes:
- 新增
taskersink/cursormove：实现CursorMoveSink（Context/Tasker sinks）并在特定控制器下注册。 - 在服务启动的
registerAll()中接入cursormove.Register()以完成自动注册。
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| agent/go-service/taskersink/cursormove/sink.go | 新增 CursorMoveSink：跟踪动作并在节点列表开始时移动光标 |
| agent/go-service/taskersink/cursormove/register.go | 根据控制器条件注册 Context/Tasker sinks，并输出注册日志 |
| agent/go-service/register.go | 将 cursormove 注册纳入启动聚合流程 |
| } | ||
|
|
||
| func (s *CursorMoveSink) moveCursor(ctrl *maa.Controller) { | ||
| ctrl.PostTouchMove(0, s.imgW, s.imgH, 0).Wait() |
There was a problem hiding this comment.
moveCursor() 直接使用 imgW/imgH 作为坐标很可能越界（bounds.Dx()/Dy() 是宽高而非最大索引），且当 imgW/imgH 仍为 0 时会把光标移动到 (0,0) 反而更容易遮挡 UI。建议在移动前对坐标做 clamp（例如使用 width-1/height-1 或留出边距），并在尺寸未初始化时跳过或先获取分辨率/截图后再移动。
Suggested change
| ctrl.PostTouchMove(0, s.imgW, s.imgH, 0).Wait() | |
| // 使用已缓存的图像宽高将光标移到右下角，避免遮挡 UI。 | |
| // 这里对坐标做 clamp：bounds.Dx()/Dy() 是宽高，最大合法索引应为 width-1/height-1。 | |
| if s.imgW <= 1 || s.imgH <= 1 { | |
| log.Warn(). | |
| Str("component", "cursormove"). | |
| Int32("width", s.imgW). | |
| Int32("height", s.imgH). | |
| Msg("skip cursor move due to invalid image size") | |
| return | |
| } | |
| x := s.imgW - 1 | |
| y := s.imgH - 1 | |
| ctrl.PostTouchMove(0, x, y, 0).Wait() |
Comment on lines
+26
to
+35
| ctrl := tasker.GetController() | ||
| if ctrl == nil { | ||
| log.Warn().Msg("[cursormove] failed to get controller from tasker") | ||
| return | ||
| } | ||
|
|
||
| img, err := ctrl.CacheImage() | ||
| if err != nil || img == nil { | ||
| log.Warn().Err(err).Msg("[cursormove] failed to get cached image for size") | ||
| return |
There was a problem hiding this comment.
这里的日志 Msg 带了 "[cursormove]" 前缀，不符合 go-service 的 zerolog 约定（上下文应放到字段中，Msg 保持简短便于检索）。建议添加 Str("component", "cursormove")（必要时再加 task_id/entry 等字段），并把 Msg 改为不带前缀的短语。
Comment on lines
+41
to
+45
| log.Info(). | ||
| Int32("width", s.imgW). | ||
| Int32("height", s.imgH). | ||
| Msg("[cursormove] captured image size") | ||
|
|
There was a problem hiding this comment.
Info 日志同样在 Msg 中使用了 "[cursormove]" 前缀；建议用字段承载 component，并将 Msg 简化为不带前缀的描述（例如 "captured image size"）。
Comment on lines
+54
to
+55
| ad, err := ctx.GetTasker().GetActionDetail(int64(detail.ActionID)) | ||
| if err != nil || ad == nil { |
There was a problem hiding this comment.
OnNodeAction 里直接调用 ctx.GetTasker() 再链式取 ActionDetail，若 ctx 或 tasker 在某些事件阶段不可用会导致 panic。仓库里已有对 ctx.GetTasker()==nil 的防御性写法（如 autostockpile/selector.go:362）。建议这里先判空 ctx 和 ctx.GetTasker()，必要时记录 debug/warn 后直接返回。
| maa.AgentServerAddTaskerSink(sink) | ||
| log.Info(). | ||
| Str("controller", pienv.ControllerName()). | ||
| Msg("[cursormove] sinks registered for Win32 controller") |
There was a problem hiding this comment.
这里日志 Msg 带 "[cursormove]" 前缀，建议按 go-service zerolog 规范把 component 放到字段（如 Str("component","cursormove")），Msg 改为简短描述。
Suggested change
| Msg("[cursormove] sinks registered for Win32 controller") | |
| Str("component", "cursormove"). | |
| Msg("sinks registered for Win32 controller") |
Comment on lines
+15
to
+17
| func Register() { | ||
| if pienv.ControllerName() != "Win32-Front" { | ||
| return |
There was a problem hiding this comment.
PR 描述里提到“当控制器类型为 Win32 时自动注册”，但这里实际是按 ControllerName()=="Win32-Front" 做条件判断。若确实只希望对 Win32-Front 生效，建议更新 PR 描述/注释以避免误解；若希望覆盖所有 Win32 controller，则应改为判断 ControllerType()=="Win32" 或检查 screencap/mouse 模式。
Comment on lines
+32
to
+36
| img, err := ctrl.CacheImage() | ||
| if err != nil || img == nil { | ||
| log.Warn().Err(err).Msg("[cursormove] failed to get cached image for size") | ||
| return | ||
| } |
There was a problem hiding this comment.
OnTaskerTask 里直接 CacheImage() 获取尺寸在任务刚启动时可能拿不到有效缓存（img==nil / size 为 0），随后 dirty 触发时会用到未初始化或上一次任务的尺寸。建议参考 aspectratio checker 的做法：必要时先 PostScreencap()/GetResolution 并带重试，成功后再设置 imgW/imgH，同时在任务启动时重置 dirty 以避免跨任务污染。
Comment on lines
+75
to
+77
| ctrl := ctx.GetTasker().GetController() | ||
| if ctrl == nil { | ||
| log.Warn().Msg("[cursormove] failed to get controller from context") |
There was a problem hiding this comment.
OnNodeNextList 同样假设 ctx.GetTasker() 一定非空；为避免极端情况下 panic，建议先判空 ctx / ctx.GetTasker() 后再取 controller。
Comment on lines
+9
to
+13
| var ( | ||
| _ maa.ContextEventSink = &CursorMoveSink{} | ||
| _ maa.TaskerEventSink = &CursorMoveSink{} | ||
| ) | ||
|
|
There was a problem hiding this comment.
编译期接口校验目前放在 register.go，而 go-service 约定要求将 var _ ... = &Type{} 放在类型定义附近，便于阅读实现时就能确认接口契约。建议把这两行移动到 sink.go 的 CursorMoveSink 定义旁。
Suggested change
| var ( | |
| _ maa.ContextEventSink = &CursorMoveSink{} | |
| _ maa.TaskerEventSink = &CursorMoveSink{} | |
| ) |
zmdyy0318
approved these changes
Mar 25, 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
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.
Sourcery 总结
为 Win32 控制器添加一个光标移动接收器（cursor movement sink），通过在识别周期之间重新定位光标，防止光标遮挡游戏界面。
新功能：
CursorMoveSink，用于跟踪影响光标的操作，并在使用 Win32 控制器时，于后续识别周期前重新定位光标。增强内容：
Original summary in English
Summary by Sourcery
Add a cursor movement sink for Win32 controllers to prevent the cursor from obstructing the game UI by repositioning it between recognition cycles.
New Features:
Enhancements:
新功能：
CursorMoveSink，用于跟踪影响光标位置的操作，并在使用 Win32 控制器时，在随后的识别周期之前重新定位光标。增强：
Original summary in English
Sourcery 总结
为 Win32 控制器添加一个光标移动接收器（cursor movement sink），通过在识别周期之间重新定位光标，防止光标遮挡游戏界面。
新功能：
CursorMoveSink，用于跟踪影响光标的操作，并在使用 Win32 控制器时，于后续识别周期前重新定位光标。增强内容：
Original summary in English
Summary by Sourcery
Add a cursor movement sink for Win32 controllers to prevent the cursor from obstructing the game UI by repositioning it between recognition cycles.
New Features:
Enhancements:
新功能：
CursorMoveSink，在使用 Win32 控制器时，在每个节点列表开始时重置光标位置。增强内容：
Original summary in English
Sourcery 总结
为 Win32 控制器添加一个光标移动接收器（cursor movement sink），通过在识别周期之间重新定位光标，防止光标遮挡游戏界面。
新功能：
CursorMoveSink，用于跟踪影响光标的操作，并在使用 Win32 控制器时，于后续识别周期前重新定位光标。增强内容：
Original summary in English
Summary by Sourcery
Add a cursor movement sink for Win32 controllers to prevent the cursor from obstructing the game UI by repositioning it between recognition cycles.
New Features:
Enhancements:
新功能：
CursorMoveSink，用于跟踪影响光标位置的操作，并在使用 Win32 控制器时，在随后的识别周期之前重新定位光标。增强：
Original summary in English
Sourcery 总结
为 Win32 控制器添加一个光标移动接收器（cursor movement sink），通过在识别周期之间重新定位光标，防止光标遮挡游戏界面。
新功能：
CursorMoveSink，用于跟踪影响光标的操作，并在使用 Win32 控制器时，于后续识别周期前重新定位光标。增强内容：
Original summary in English
Summary by Sourcery
Add a cursor movement sink for Win32 controllers to prevent the cursor from obstructing the game UI by repositioning it between recognition cycles.
New Features:
Enhancements: