Dev c#1619
Open
LGG686 wants to merge 2 commits into
Open
Conversation
- 将 battle_theme_model 改为字典推导式风格，与其他配置保持一致 - 在 check_costume_battle 方法中添加未采集资源跳过逻辑 - 将 I_WIN/I_DE_WIN/I_FALSE 改为动态命名，支持所有主题自定义资源
- 新增登云问翠和茸茨跃动战斗皮肤配置 - 更新翻译
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些高层次的反馈：
- 新的
battle_theme_model字典推导目前与数值范围range(1, 15)和COSTUME_BATTLE_{i}命名强耦合；建议考虑直接遍历BattleType成员（或一个专门的列表），而不是依赖硬编码下标，这样可以避免未来枚举或名称发生偏移时导致getattr失败。 - 在
battle_theme_model中，将属性名作为字符串存储，并在check_costume_battle中通过getattr解析，会增加一层间接性；如果可能，考虑直接存储实际的RuleImage属性，或一个直接描述 ROI 的小数据结构，以减少拼写错误和运行时错误的风险。 check_costume_battle中的hasattr(costume_battle_assets, value)判断会静默地跳过缺失的资源；如果某些战斗预期必须存在特定资源，你可能希望区分可选和必需键（例如通过拆分模型或增加显式标记），以免必需资源缺失时悄无声息地失败。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `battle_theme_model` dict comprehension is tightly coupled to the numeric range `range(1, 15)` and `COSTUME_BATTLE_{i}` naming; consider iterating over `BattleType` members themselves (or a dedicated list) instead of hardcoded indices to avoid future enum/name drift causing `getattr` failures.
- In `battle_theme_model`, mapping to attribute names as strings and resolving them with `getattr` in `check_costume_battle` adds a layer of indirection; if possible, consider storing the actual `RuleImage` attributes or a small data structure describing ROIs directly to reduce the risk of typos and runtime errors.
- The `hasattr(costume_battle_assets, value)` guard in `check_costume_battle` silently skips missing assets; if certain assets are expected to be present for some battles, you might want to distinguish between optional and required keys (e.g., by splitting the model or adding an explicit flag) so that missing required assets don’t fail quietly.
## Individual Comments
### Comment 1
<location path="tasks/Component/Costume/costume_base.py" line_range="127-128" />
<code_context>
logger.info(f'Switch battle theme {battle_type}')
costume_battle_assets = CostumeBattleAssets()
for key, value in battle_theme_model[battle_type].items():
+ if not hasattr(costume_battle_assets, value):
+ # 尚未采集完成的资产，跳过
+ continue
assert_value: RuleImage = getattr(costume_battle_assets, value)
</code_context>
<issue_to_address>
**issue (bug_risk):** Silently skipping missing assets via hasattr can hide configuration or naming errors.
This guard makes incomplete asset collections safer, but it also hides genuine mistakes in `CostumeBattleAssets` (e.g., typos like a misnamed `I_WIN_13`), causing unexpected behavior with no logs or exceptions.
Consider differentiating optional vs required assets: keep `hasattr` + `continue` for optional ones, but for required keys either assert their presence or at least log a warning when `hasattr` is False. This preserves flexibility while surfacing real configuration errors.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进代码评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
battle_theme_modeldict comprehension is tightly coupled to the numeric rangerange(1, 15)andCOSTUME_BATTLE_{i}naming; consider iterating overBattleTypemembers themselves (or a dedicated list) instead of hardcoded indices to avoid future enum/name drift causinggetattrfailures. - In
battle_theme_model, mapping to attribute names as strings and resolving them withgetattrincheck_costume_battleadds a layer of indirection; if possible, consider storing the actualRuleImageattributes or a small data structure describing ROIs directly to reduce the risk of typos and runtime errors. - The
hasattr(costume_battle_assets, value)guard incheck_costume_battlesilently skips missing assets; if certain assets are expected to be present for some battles, you might want to distinguish between optional and required keys (e.g., by splitting the model or adding an explicit flag) so that missing required assets don’t fail quietly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `battle_theme_model` dict comprehension is tightly coupled to the numeric range `range(1, 15)` and `COSTUME_BATTLE_{i}` naming; consider iterating over `BattleType` members themselves (or a dedicated list) instead of hardcoded indices to avoid future enum/name drift causing `getattr` failures.
- In `battle_theme_model`, mapping to attribute names as strings and resolving them with `getattr` in `check_costume_battle` adds a layer of indirection; if possible, consider storing the actual `RuleImage` attributes or a small data structure describing ROIs directly to reduce the risk of typos and runtime errors.
- The `hasattr(costume_battle_assets, value)` guard in `check_costume_battle` silently skips missing assets; if certain assets are expected to be present for some battles, you might want to distinguish between optional and required keys (e.g., by splitting the model or adding an explicit flag) so that missing required assets don’t fail quietly.
## Individual Comments
### Comment 1
<location path="tasks/Component/Costume/costume_base.py" line_range="127-128" />
<code_context>
logger.info(f'Switch battle theme {battle_type}')
costume_battle_assets = CostumeBattleAssets()
for key, value in battle_theme_model[battle_type].items():
+ if not hasattr(costume_battle_assets, value):
+ # 尚未采集完成的资产，跳过
+ continue
assert_value: RuleImage = getattr(costume_battle_assets, value)
</code_context>
<issue_to_address>
**issue (bug_risk):** Silently skipping missing assets via hasattr can hide configuration or naming errors.
This guard makes incomplete asset collections safer, but it also hides genuine mistakes in `CostumeBattleAssets` (e.g., typos like a misnamed `I_WIN_13`), causing unexpected behavior with no logs or exceptions.
Consider differentiating optional vs required assets: keep `hasattr` + `continue` for optional ones, but for required keys either assert their presence or at least log a warning when `hasattr` is False. This preserves flexibility while surfacing real configuration errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+127
to
+128
| if not hasattr(costume_battle_assets, value): | ||
| # 尚未采集完成的资产，跳过 |
There was a problem hiding this comment.
issue (bug_risk): 通过 hasattr 静默跳过缺失资源，可能会隐藏配置或命名错误。
这个保护逻辑可以让不完整的资源集合更安全，但同时也会掩盖 CostumeBattleAssets 中的真实错误（例如像误写为 I_WIN_13 这样的拼写问题），从而在没有日志或异常的情况下导致意外行为。
建议区分可选资源和必需资源：对可选项保持 hasattr + continue 的逻辑，而对于必需键，要么显式断言其存在，要么在 hasattr 为 False 时至少记录一个警告日志。这样既保留了灵活性，又能暴露真实的配置错误。
Original comment in English
issue (bug_risk): Silently skipping missing assets via hasattr can hide configuration or naming errors.
This guard makes incomplete asset collections safer, but it also hides genuine mistakes in CostumeBattleAssets (e.g., typos like a misnamed I_WIN_13), causing unexpected behavior with no logs or exceptions.
Consider differentiating optional vs required assets: keep hasattr + continue for optional ones, but for required keys either assert their presence or at least log a warning when hasattr is False. This preserves flexibility while surfacing real configuration errors.
Change Summary
Reconstructed Intent点击此处展开- 这次改动最可能是为了把“战斗主题适配”从零散特例改成统一模型,降低后续继续加主题时的改动面。 - 更像是在尝试解决:不同战斗皮肤下,通用 `I_BATTLE_INFO` 识别不稳定,因此为每套战斗主题补专属战斗信息图,连同胜利/失败图一起按主题覆盖。 - 新增 13、14 主题本身是显式目标,但从代码行为看,作者同时想把现有 1-12 的战斗页识别链路也纳入同一套替换机制。Observed Constraints点击此处展开- 这套实现依赖 `BattleType` 枚举名与 `COSTUME_BATTLE_{i}`、资源属性名 `I_*_{i}`、目录 `battle{i}` 三者严格同构;任何一处命名漂移都会在运行时失配。 - `check_costume_battle` 只会替换当前对象上已经存在的同名资产,因此它默认依赖消费方已经暴露 `I_BATTLE_INFO`、`I_WIN`、`I_FALSE` 等通用键。 - `I_WIN` / `I_DE_WIN` / `I_FALSE` 现在被视为“可缺省资源”,实现默认只有 8、12、13、14 真正采集完整,其余主题缺失时允许静默回退到原始通用资产。 - `I_LOCAL` 仍保留 `roi_back` 不替换的旧约束,说明作者认为绿标定位的后景范围必须继续沿用原链路。Intent Alignment
Release Risk
Validation Gaps点击此处展开- 未看到针对 1-14 各主题的战斗页识别回归验证,尤其是 `I_BATTLE_INFO` 替换后是否仍能稳定驱动 `GeneralBattle` 的进出战判断。 - 未看到 13、14 主题下胜利、失败、封魔特殊结算三条分支的链路验证。 - 未看到“资源缺失时应回退哪些键、哪些键必须命中”的显式验证,因此当前行为更像容错策略,而不是被验证过的约束。Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
💡 Tip: tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"See Network Configuration for more information.
|
Findings
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
💡 Tip: tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"See Network Configuration for more information.
|
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
1 participant
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 by Sourcery
为额外的服装战主题添加支持，并通过可选资源统一战斗素材的处理方式。
新特性：
改进：
Original summary in English
Summary by Sourcery
Add support for additional costume battle themes and unify battle asset handling with optional resources.
New Features:
Enhancements: