fix:OneKeyEntryEnd的两个bug#127
Conversation
审阅者指南(在小型 PR 上默认折叠)审阅者指南此 PR 修复了 OneKeyEntryEnd UX 模组中的两个 Bug: 游客模式下修补后的 NetDataManager.GetGuestLogId 序列图sequenceDiagram
actor Player
participant GameClient
participant NetDataManager
participant GamePlayManager
participant GameScoreList
participant HarmonyFinalizer_GetGuestLogIdFix
Player ->> GameClient: EndPlayCardInGuestMode
GameClient ->> NetDataManager: GetGuestLogId()
NetDataManager ->> GamePlayManager: GetGameScore(playId)
GamePlayManager -->> NetDataManager: null
NetDataManager ->> GameScoreList: AccessGameScoreList
GameScoreList -->> NetDataManager: NullReferenceException
NetDataManager -->> HarmonyFinalizer_GetGuestLogIdFix: __exception, __result
HarmonyFinalizer_GetGuestLogIdFix ->> HarmonyFinalizer_GetGuestLogIdFix: Check __exception
HarmonyFinalizer_GetGuestLogIdFix ->> HarmonyFinalizer_GetGuestLogIdFix: Set __result = 0
HarmonyFinalizer_GetGuestLogIdFix -->> NetDataManager: return null (swallow exception)
NetDataManager -->> GameClient: guestLogId = 0
GameClient -->> Player: Guest session ends without crash
OneKeyEntryEnd 修复及相关游戏类的类图classDiagram
class OneKeyEntryEnd {
+static void DoQuickSkip()
+static Exception GetGuestLogIdFix(Exception __exception, ulong __result)
}
class NetDataManager {
+ulong GetGuestLogId()
}
class GamePlayManager {
+static GamePlayManager Instance
+object GetGameScore(int playId)
}
class ProcessManager {
+void AddProcess(Process process)
}
class Process {
+void Execute()
}
class FadeProcess {
+FadeProcess(ProcessDataContainer container, Process nextProcess, Process afterFadeProcess)
}
class UnlockMusicProcess {
+UnlockMusicProcess(ProcessDataContainer container)
}
class MusicSelectProcess {
+MusicSelectProcess(ProcessDataContainer container)
}
class ProcessDataContainer {
}
class SharedInstances {
+static ProcessDataContainer ProcessDataContainer
}
%% Relationships for DoQuickSkip behavior
OneKeyEntryEnd --> ProcessManager : uses
ProcessManager --> Process : manages
FadeProcess ..|> Process
UnlockMusicProcess ..|> Process
MusicSelectProcess ..|> Process
OneKeyEntryEnd --> FadeProcess : creates
OneKeyEntryEnd --> UnlockMusicProcess : creates
OneKeyEntryEnd --> MusicSelectProcess : creates
OneKeyEntryEnd --> SharedInstances : uses
SharedInstances --> ProcessDataContainer
%% Relationships for GetGuestLogId fix
OneKeyEntryEnd ..> NetDataManager : HarmonyPatch GetGuestLogId
NetDataManager --> GamePlayManager : calls
GamePlayManager --> Process : returns score-like object
更新后的 DoQuickSkip 循环与集合修改修复的流程图flowchart TD
A_Start[Start DoQuickSkip] --> B_Iterate[Foreach process in processList]
B_Iterate --> C_CheckType{Is target process to end PC?}
C_CheckType -->|No| D_Next[Move to next process]
D_Next --> B_Iterate
C_CheckType -->|Yes| E_AddFadeProcess[Add FadeProcess to processManager]
E_AddFadeProcess --> F_AddUnlockMusicProcess[Add UnlockMusicProcess to processManager]
F_AddUnlockMusicProcess --> G_GotoOutLoop[Goto outLoop to exit foreach]
G_GotoOutLoop --> H_outLoop[outLoop label]
H_outLoop --> I_HasProcessToRelease{processToRelease != null?}
I_HasProcessToRelease -->|Yes| J_AddMusicSelectProcess[Add MusicSelectProcess to processManager]
I_HasProcessToRelease -->|No| K_End[End DoQuickSkip]
J_AddMusicSelectProcess --> K_End
文件级变更
技巧与命令与 Sourcery 交互
自定义你的体验前往你的 控制面板 可以:
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes two bugs in the OneKeyEntryEnd UX mod: a collection-modification exception when quickly skipping, and a null-reference crash in guest mode by adding a loop-escape and a Harmony finalizer around NetDataManager.GetGuestLogId to restore pre‑1.65 behavior. Sequence diagram for patched NetDataManager.GetGuestLogId in guest modesequenceDiagram
actor Player
participant GameClient
participant NetDataManager
participant GamePlayManager
participant GameScoreList
participant HarmonyFinalizer_GetGuestLogIdFix
Player ->> GameClient: EndPlayCardInGuestMode
GameClient ->> NetDataManager: GetGuestLogId()
NetDataManager ->> GamePlayManager: GetGameScore(playId)
GamePlayManager -->> NetDataManager: null
NetDataManager ->> GameScoreList: AccessGameScoreList
GameScoreList -->> NetDataManager: NullReferenceException
NetDataManager -->> HarmonyFinalizer_GetGuestLogIdFix: __exception, __result
HarmonyFinalizer_GetGuestLogIdFix ->> HarmonyFinalizer_GetGuestLogIdFix: Check __exception
HarmonyFinalizer_GetGuestLogIdFix ->> HarmonyFinalizer_GetGuestLogIdFix: Set __result = 0
HarmonyFinalizer_GetGuestLogIdFix -->> NetDataManager: return null (swallow exception)
NetDataManager -->> GameClient: guestLogId = 0
GameClient -->> Player: Guest session ends without crash
Class diagram for OneKeyEntryEnd fixes and related game classesclassDiagram
class OneKeyEntryEnd {
+static void DoQuickSkip()
+static Exception GetGuestLogIdFix(Exception __exception, ulong __result)
}
class NetDataManager {
+ulong GetGuestLogId()
}
class GamePlayManager {
+static GamePlayManager Instance
+object GetGameScore(int playId)
}
class ProcessManager {
+void AddProcess(Process process)
}
class Process {
+void Execute()
}
class FadeProcess {
+FadeProcess(ProcessDataContainer container, Process nextProcess, Process afterFadeProcess)
}
class UnlockMusicProcess {
+UnlockMusicProcess(ProcessDataContainer container)
}
class MusicSelectProcess {
+MusicSelectProcess(ProcessDataContainer container)
}
class ProcessDataContainer {
}
class SharedInstances {
+static ProcessDataContainer ProcessDataContainer
}
%% Relationships for DoQuickSkip behavior
OneKeyEntryEnd --> ProcessManager : uses
ProcessManager --> Process : manages
FadeProcess ..|> Process
UnlockMusicProcess ..|> Process
MusicSelectProcess ..|> Process
OneKeyEntryEnd --> FadeProcess : creates
OneKeyEntryEnd --> UnlockMusicProcess : creates
OneKeyEntryEnd --> MusicSelectProcess : creates
OneKeyEntryEnd --> SharedInstances : uses
SharedInstances --> ProcessDataContainer
%% Relationships for GetGuestLogId fix
OneKeyEntryEnd ..> NetDataManager : HarmonyPatch GetGuestLogId
NetDataManager --> GamePlayManager : calls
GamePlayManager --> Process : returns score-like object
Flow diagram for updated DoQuickSkip loop and collection modification fixflowchart TD
A_Start[Start DoQuickSkip] --> B_Iterate[Foreach process in processList]
B_Iterate --> C_CheckType{Is target process to end PC?}
C_CheckType -->|No| D_Next[Move to next process]
D_Next --> B_Iterate
C_CheckType -->|Yes| E_AddFadeProcess[Add FadeProcess to processManager]
E_AddFadeProcess --> F_AddUnlockMusicProcess[Add UnlockMusicProcess to processManager]
F_AddUnlockMusicProcess --> G_GotoOutLoop[Goto outLoop to exit foreach]
G_GotoOutLoop --> H_outLoop[outLoop label]
H_outLoop --> I_HasProcessToRelease{processToRelease != null?}
I_HasProcessToRelease -->|Yes| J_AddMusicSelectProcess[Add MusicSelectProcess to processManager]
I_HasProcessToRelease -->|No| K_End[End DoQuickSkip]
J_AddMusicSelectProcess --> K_End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体反馈:
- 建议避免使用
goto outLoop。可以考虑把这段逻辑提取到一个辅助方法中,或者使用标志位/break之类的结构,使控制流更容易理解和维护。 - 目前 Harmony 的 finalizer 会吞掉
GetGuestLogId的所有异常;如果可行的话,建议将其收窄到你预期的特定异常类型(例如 NRE),以避免掩盖无关的问题。 - 在
GetGuestLogIdFix中,你可能只想在__exception是预期类型时才显式设置__result,在其他情况下保持不变,以便在意料之外的错误场景下更好地保留原有行为。
给 AI 代理的提示
请根据这次代码评审中的评论进行修改:
## 总体评论
- 建议避免使用 `goto outLoop`。可以考虑把这段逻辑提取到一个辅助方法中,或者使用标志位/`break` 之类的结构,使控制流更容易理解和维护。
- 目前 Harmony 的 finalizer 会吞掉 `GetGuestLogId` 的所有异常;如果可行的话,建议将其收窄到你预期的特定异常类型(例如 NRE),以避免掩盖无关的问题。
- 在 `GetGuestLogIdFix` 中,你可能只想在 `__exception` 是预期类型时才显式设置 `__result`,在其他情况下保持不变,以便在意料之外的错误场景下更好地保留原有行为。
## 单独评论
### 评论 1
<location path="AquaMai.Mods/UX/OneKeyEntryEnd.cs" line_range="105-110" />
<code_context>
+ [EnableGameVersion(26500, noWarn: true)]
+ [HarmonyPatch(typeof(NetDataManager), "GetGuestLogId")]
+ [HarmonyFinalizer]
+ public static Exception GetGuestLogIdFix(Exception __exception, ref ulong __result)
+ {
+ // 如果在游客模式下,一首歌都没打就跳关了:
+ // NetDataManager.GetGuestLogId中会调用Singleton<GamePlayManager>.Instance.GetGameScore,这个函数查不到对应的GameScoreList对象,就会返回null;于是就NPE了
+ // 我们则捕获这里的异常,返回数字0(这正是1.60之前的行为)
+ if (__exception != null) __result = 0L;
+ return null;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** 请收窄异常处理范围,以免无关的失败被静默吞掉。
注释表明,这个 finalizer 主要是为了处理 `GetGameScore` 返回 null 时导致的 `NullReferenceException`。但按当前实现,它会把任何异常(包括无关的网络或逻辑错误)都转换为结果 `0`,这样会掩盖真实问题。
请将特殊处理限制在预期场景,例如:
```csharp
if (__exception is NullReferenceException)
{
__result = 0UL; // matches pre‑1.60 behavior
return null; // swallow only this known issue
}
return __exception; // let other exceptions propagate
```
这样既能保持兼容性行为,又能避免屏蔽无关的异常。
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding the
goto outLoopby extracting the logic into a helper method or using a flag/breakstructure so the control flow is easier to follow and maintain. - The Harmony finalizer currently swallows all exceptions for
GetGuestLogId; if feasible, narrow this to the specific exception type(s) you expect (e.g., NRE) to avoid hiding unrelated issues. - In
GetGuestLogIdFix, you may want to explicitly set__resultonly when__exceptionis of the expected type and leave it untouched otherwise, to better preserve the original behavior in unexpected error cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the `goto outLoop` by extracting the logic into a helper method or using a flag/`break` structure so the control flow is easier to follow and maintain.
- The Harmony finalizer currently swallows all exceptions for `GetGuestLogId`; if feasible, narrow this to the specific exception type(s) you expect (e.g., NRE) to avoid hiding unrelated issues.
- In `GetGuestLogIdFix`, you may want to explicitly set `__result` only when `__exception` is of the expected type and leave it untouched otherwise, to better preserve the original behavior in unexpected error cases.
## Individual Comments
### Comment 1
<location path="AquaMai.Mods/UX/OneKeyEntryEnd.cs" line_range="105-110" />
<code_context>
+ [EnableGameVersion(26500, noWarn: true)]
+ [HarmonyPatch(typeof(NetDataManager), "GetGuestLogId")]
+ [HarmonyFinalizer]
+ public static Exception GetGuestLogIdFix(Exception __exception, ref ulong __result)
+ {
+ // 如果在游客模式下,一首歌都没打就跳关了:
+ // NetDataManager.GetGuestLogId中会调用Singleton<GamePlayManager>.Instance.GetGameScore,这个函数查不到对应的GameScoreList对象,就会返回null;于是就NPE了
+ // 我们则捕获这里的异常,返回数字0(这正是1.60之前的行为)
+ if (__exception != null) __result = 0L;
+ return null;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Narrow the exception handling so unrelated failures are not silently swallowed.
The comment suggests this finalizer is meant to handle a `NullReferenceException` from `GetGameScore` returning null. As implemented, it converts any exception (including unrelated network or logic failures) into a result of `0`, which can hide real issues.
Limit the special handling to the expected case, for example:
```csharp
if (__exception is NullReferenceException)
{
__result = 0UL; // matches pre‑1.60 behavior
return null; // swallow only this known issue
}
return __exception; // let other exceptions propagate
```
This keeps the compatibility behavior while avoiding masking unrelated exceptions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request modifies OneKeyEntryEnd.cs to prevent a System.InvalidOperationException by exiting a loop early after modifying the process list, and introduces a Harmony finalizer to handle potential null pointer exceptions in NetDataManager.GetGuestLogId during guest mode. The review feedback suggests replacing the goto statement with a return to simplify the control flow and remove the unnecessary label.
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="AquaMai.Mods/UX/OneKeyEntryEnd.cs">
<violation number="1" location="AquaMai.Mods/UX/OneKeyEntryEnd.cs:110">
P2: The finalizer unconditionally suppresses all exceptions from `GetGuestLogId`; only the expected null-reference case should be swallowed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
1. 在1.65以上,如果开游客模式、一首歌不打直接结束PC,游戏会直接崩溃。由1.65新增的NetDataManager.GetGuestLogId函数引起,具体原因详见代码中的注释。 2. (轻微bug,不影响任何功能,只是会在控制台打印出一行Error不好看) `DoQuickSkip`调用AddProcess、尝试结束PC后,没有立即跳出循环,造成迭代器继续遍历时抛异常。
7d406e2 to
214c13d
Compare
|
上面两个AI讲的问题都是一件事:觉得我Finalizer里屏蔽一切异常太过分,最好只抓NPE。但我是怎么想的呢:
综上,我认为就这样挺好的,无需改成只捕获NPE。 |
毕竟 AI 训练数据里面大多都是通用的应用程序,而对于像这样的 Mod 开发,本来就有很多操作是非寻常的,比如说需要使用歪门邪道的。这些 AI 对于这样的操作可能是很难很好的理解的 |
NetDataManager.GetGuestLogId(1.65新增的函数)中会调用Singleton<GamePlayManager>.Instance.GetGameScore,由于一首歌都没打,GetGameScore函数查不到对应的GameScoreList对象,就会返回nullGetGuestLogId会直接使用GetGameScore所返回的结果,于是就NPE了DoQuickSkip调用AddProcess、尝试结束PC后,没有立即跳出循环,造成迭代器继续遍历时抛异常。System.InvalidOperationException: Collection was modified; enumeration operation may not execute.由 Sourcery 提供的总结
修复在特定条件下跳过或结束播放时发生的 OneKeyEntryEnd 崩溃和错误。
错误修复:
NetDataManager.GetGuestLogId发生崩溃。DoQuickSkip抛出集合在枚举期间被修改的异常。Original summary in English
Summary by Sourcery
Fix OneKeyEntryEnd crashes and errors when skipping or ending play under specific conditions.
Bug Fixes: