fix: add default value for missing skill in CopilotOperator#496
Merged
wangl-cc merged 1 commit intoMaaAssistantArknights:mainfrom Mar 21, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #496 +/- ##
=======================================
Coverage 70.52% 70.52%
=======================================
Files 61 61
Lines 5842 5842
Branches 5842 5842
=======================================
Hits 4120 4120
Misses 1406 1406
Partials 316 316 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
13d6dfb to
04edeeb
Compare
There was a problem hiding this comment.
Hey - 我在这里提供了一些高层次的反馈:
- 在 copilot 文件解析循环中,你现在忽略了
resolve_copilot_uris返回的内存中的 JSON 值,而是总是从file重新读取,这可能会破坏基于 URI 或内联的 copilot 定义;建议保留并规范化已经解析好的值,而不是直接丢弃它。 load_skill_support_map目前会把所有在battle_data.json中缺失的干员视为稀有度 -1,并因此强制将技能 1–3 设为 0;如果这并不是严格想要的行为,你可能需要区分“未知干员”和“已知低稀有度干员”,这样未知名字就可以保留其已有的技能配置。is_skill_supported对于任何超出 1–3 的技能编号都会返回true,这会在无提示的情况下把意外的技能下标当作受支持的;如果你期望技能编号严格为 0–3,那么可能应该显式拒绝或规范化这些超出范围的值。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In the copilot file resolution loop you now ignore the in‑memory JSON value from `resolve_copilot_uris` and always re‑read from `file`, which may break URI/inlined copilot definitions; consider preserving and normalizing the already-parsed value instead of discarding it.
- `load_skill_support_map` currently treats any operator missing from `battle_data.json` as having rarity -1 and thus forces skills 1–3 to 0; if that’s not strictly desired, you may want to distinguish between unknown and known-low-rarity operators so that unknown names keep their configured skills.
- `is_skill_supported` returns `true` for any skill number outside 1–3, which silently treats unexpected skill indices as supported; if you expect skills to be strictly 0–3 you might instead reject or normalize out-of-range values explicitly.帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've left some high level feedback:
- In the copilot file resolution loop you now ignore the in‑memory JSON value from
resolve_copilot_urisand always re‑read fromfile, which may break URI/inlined copilot definitions; consider preserving and normalizing the already-parsed value instead of discarding it. load_skill_support_mapcurrently treats any operator missing frombattle_data.jsonas having rarity -1 and thus forces skills 1–3 to 0; if that’s not strictly desired, you may want to distinguish between unknown and known-low-rarity operators so that unknown names keep their configured skills.is_skill_supportedreturnstruefor any skill number outside 1–3, which silently treats unexpected skill indices as supported; if you expect skills to be strictly 0–3 you might instead reject or normalize out-of-range values explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the copilot file resolution loop you now ignore the in‑memory JSON value from `resolve_copilot_uris` and always re‑read from `file`, which may break URI/inlined copilot definitions; consider preserving and normalizing the already-parsed value instead of discarding it.
- `load_skill_support_map` currently treats any operator missing from `battle_data.json` as having rarity -1 and thus forces skills 1–3 to 0; if that’s not strictly desired, you may want to distinguish between unknown and known-low-rarity operators so that unknown names keep their configured skills.
- `is_skill_supported` returns `true` for any skill number outside 1–3, which silently treats unexpected skill indices as supported; if you expect skills to be strictly 0–3 you might instead reject or normalize out-of-range values explicitly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
36a0912 to
1902409
Compare
Member
|
我最近比较忙,一直没顾上看这边。感谢你的贡献和补充 WPF 的思路。如果只是修复 #495 的问题,其实目前不需要这么复杂的改动,直接给 CopilotOperator 的 skill 设置默认值即可。 当前这个 PR 还引入了基于 battle_data.json 的 skill 标准化处理。我不太清楚 WPF 的具体实现方式,让 Codex 帮忙看了一下,它提到 WPF 会处理多语言的干员名称,在当前 PR 中这可能导致回归,非中文干员名可能会被强行将 skill 设为 0。虽然不确定作业站是否真有非中文作业,但这在理论上确实存在回归风险。 如果你愿意继续推进当前这个方向,我建议至少先把 #495 的最小修复独立出来,这样后续标准化部分可以单独讨论。 |
1902409 to
68ee02b
Compare
Member
|
LGTM!感谢贡献。 |
This was referenced Mar 22, 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.
前几天就遇到这个问题了,既然有人发issue了,就先开个pr
参照wpf那边的逻辑,从resource/battle_data.json中读取干员的rarity,并根据此对作业里的skill进行解析,暂时不考虑术兔这种5星却拥有3技能的情况skill标准化处理之后单开个issue
fix #495
适配上游最近更改的MaaAssistantArknights/MaaAssistantArknights#15898,将skill默认值改为0