feat: JudgeDisplayPro - #140
Conversation
- 通过 SlideRoot.SetJudgeObject 登记 monitor,覆盖普通 Slide 与 SlideFan 的对象池复用 - 修正 Break/非 Break 的 CRITICAL 矩阵及 Slide PERFECT 的 timing 素材映射 - 每次判定复位 FAST/LATE 和附加图层,并保持判定位置 OFF 时隐藏 - 按 PERFECT/GREAT/GOOD 各自显示模式统计 FAST/LATE,防止重复判定和 TrackSkip 误计 - 保留 GameScoreList 单曲快照的大 P 分桶,并让主结算读取同一份快照 - 限制设置项增减边界,并按枚举名称反序列化玩家设置
This comment was marked as low quality.
This comment was marked as low quality.
Reviewer's Guide介绍 JudgeDisplayPro 用户体验模组,将按玩家的判定显示配置接入现有设置系统、游戏中判定渲染(tap/touch/slide)、FAST/LATE 分数统计、结算流程以及选项预览贴图,同时收紧补丁应用与 AssetBundle 加载逻辑。 通过 GameScoreList.SetResult 更新后的 FAST/LATE 统计时序图sequenceDiagram
participant GameLogic
participant GameScoreList
participant JudgeDisplayPro
participant Logic
participant NotesManager
GameLogic->>GameScoreList: SetResult(index, timing)
activate GameScoreList
GameScoreList->>JudgeDisplayPro: PreGameScoreListSetResult(__instance, index, timing, monitorIndex, out FastLateState)
activate JudgeDisplayPro
JudgeDisplayPro->>NotesManager: Instance(monitorIndex)
NotesManager-->>JudgeDisplayPro: getReader().GetNoteList()[index]
JudgeDisplayPro->>Logic: ShouldCountFastLate(settings, timing, isBreak)
Logic-->>JudgeDisplayPro: bool
JudgeDisplayPro-->>GameScoreList: FastLateState (saved as __state)
deactivate JudgeDisplayPro
GameScoreList-->>GameScoreList: original SetResult logic
GameScoreList->>JudgeDisplayPro: PostGameScoreListSetResult(__instance, __state)
activate JudgeDisplayPro
JudgeDisplayPro->>GameScoreList: setFast(__instance, newFast)
JudgeDisplayPro->>GameScoreList: setLate(__instance, newLate)
deactivate JudgeDisplayPro
deactivate GameScoreList
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience打开你的 dashboard 以:
Getting HelpOriginal review guide in EnglishReviewer's GuideIntroduces the JudgeDisplayPro UX mod, wiring per-player judge display configuration into the existing settings system, gameplay judge rendering (tap/touch/slide), FAST/LATE score counting, result flow, and option preview sprites, while tightening patch application and asset bundle loading. Sequence diagram for updated FAST/LATE counting via GameScoreList.SetResultsequenceDiagram
participant GameLogic
participant GameScoreList
participant JudgeDisplayPro
participant Logic
participant NotesManager
GameLogic->>GameScoreList: SetResult(index, timing)
activate GameScoreList
GameScoreList->>JudgeDisplayPro: PreGameScoreListSetResult(__instance, index, timing, monitorIndex, out FastLateState)
activate JudgeDisplayPro
JudgeDisplayPro->>NotesManager: Instance(monitorIndex)
NotesManager-->>JudgeDisplayPro: getReader().GetNoteList()[index]
JudgeDisplayPro->>Logic: ShouldCountFastLate(settings, timing, isBreak)
Logic-->>JudgeDisplayPro: bool
JudgeDisplayPro-->>GameScoreList: FastLateState (saved as __state)
deactivate JudgeDisplayPro
GameScoreList-->>GameScoreList: original SetResult logic
GameScoreList->>JudgeDisplayPro: PostGameScoreListSetResult(__instance, __state)
activate JudgeDisplayPro
JudgeDisplayPro->>GameScoreList: setFast(__instance, newFast)
JudgeDisplayPro->>GameScoreList: setLate(__instance, newLate)
deactivate JudgeDisplayPro
deactivate GameScoreList
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as low quality.
This comment was marked as low quality.
There was a problem hiding this comment.
Hey - 我发现了两个问题,并留下了一些高层次的反馈:
UserSettings.Deserialize方法假定输入格式是固定且有效的,并且使用bool.Parse/Enum.Parse时没有做边界或错误检查。建议你先验证values.Length,并使用TryParse/ 默认回退值,这样可以避免在已损坏或旧版本存储数据的情况下发生硬崩溃。JudgeDisplayPro.JudgeGrade中的大型嵌套switch(以及类似的 slide 处理逻辑)在ETiming、显示模式和精灵之间重复了大量映射逻辑;可以考虑抽取共享的辅助方法或查找表,使这部分逻辑更易维护,并在添加新模式或时序时减少出错风险。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `UserSettings.Deserialize` method assumes a fixed, valid format and uses `bool.Parse`/`Enum.Parse` without bounds or error checking, so consider validating `values.Length` and using `TryParse`/default fallbacks to avoid hard crashes on corrupted or older stored data.
- The large nested `switch` blocks in `JudgeDisplayPro.JudgeGrade` (and similarly in the slide handling) duplicate a lot of mapping logic between `ETiming`, display modes and sprites; extracting shared helpers or lookup tables would make this logic easier to maintain and less error-prone when adding new modes or timings.
## Individual Comments
### Comment 1
<location path="AquaMai.Mods/UX/JudgeDisplayPro/Models.cs" line_range="54-61" />
<code_context>
+ return $"{IsEnable},{CriticalDisplayMode},{PerfectDisplayMode},{GreatDisplayMode},{GoodDisplayMode}";
+ }
+
+ public void Deserialize(string data)
+ {
+ var values = data.Split(',');
+ IsEnable = bool.Parse(values[0]);
+ CriticalDisplayMode = (CriticalDisplayMode)Enum.Parse(typeof(CriticalDisplayMode), values[1]);
+ PerfectDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[2]);
+ GreatDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[3]);
+ GoodDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[4]);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Make `Deserialize` more robust against malformed or older serialized data.
This relies on a perfectly formed string (5 comma‑separated values, valid enum names). Any deviation (empty or shorter string, renamed enum values, corrupted settings) will throw and can break startup/settings loading.
Please add basic guards: verify `values.Length >= 5`, use `bool.TryParse` / `Enum.TryParse(..., ignoreCase: true, out ...)`, and on failure either keep existing values or use safe defaults. That will improve compatibility with older formats and make the app more resilient to bad data.
</issue_to_address>
### Comment 2
<location path="AquaMai.Mods/UX/JudgeDisplayPro/JudgeDisplayPro.Score.cs" line_range="77-86" />
<code_context>
+ [HarmonyTranspiler]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Failing hard in the transpiler on pattern mismatch can make the whole mod brittle.
Since IL layouts can change between game versions, throwing `InvalidOperationException` on a pattern mismatch may stop the process from starting for a non-critical UX feature. Consider a softer failure: log a warning (including `matches.Length`) and return the original `instructions` when the pattern isn’t exactly 1, so behavior degrades gracefully instead of crashing.
Suggested implementation:
```csharp
if (matches.Length != 1)
{
UnityEngine.Debug.LogWarning($"[JudgeDisplayPro] ResultProcess.OnStart transpiler expected exactly one match for pattern but found {matches.Length}; leaving method unpatched.");
return instructions;
}
```
Because I only see part of the file, you may need to:
1. Adjust the `SEARCH` block to match the exact text of your existing `InvalidOperationException` line(s) if the message differs.
2. Ensure the `matches` symbol is indeed the array/collection whose count you’re checking; if it has a different name or type, update the condition and the interpolated string accordingly.
3. If you prefer a different logging mechanism (e.g., your mod’s own logger instead of `UnityEngine.Debug.LogWarning`), replace the logging call with your project’s standard logging API while keeping the early `return instructions;`.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The
UserSettings.Deserializemethod assumes a fixed, valid format and usesbool.Parse/Enum.Parsewithout bounds or error checking, so consider validatingvalues.Lengthand usingTryParse/default fallbacks to avoid hard crashes on corrupted or older stored data. - The large nested
switchblocks inJudgeDisplayPro.JudgeGrade(and similarly in the slide handling) duplicate a lot of mapping logic betweenETiming, display modes and sprites; extracting shared helpers or lookup tables would make this logic easier to maintain and less error-prone when adding new modes or timings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `UserSettings.Deserialize` method assumes a fixed, valid format and uses `bool.Parse`/`Enum.Parse` without bounds or error checking, so consider validating `values.Length` and using `TryParse`/default fallbacks to avoid hard crashes on corrupted or older stored data.
- The large nested `switch` blocks in `JudgeDisplayPro.JudgeGrade` (and similarly in the slide handling) duplicate a lot of mapping logic between `ETiming`, display modes and sprites; extracting shared helpers or lookup tables would make this logic easier to maintain and less error-prone when adding new modes or timings.
## Individual Comments
### Comment 1
<location path="AquaMai.Mods/UX/JudgeDisplayPro/Models.cs" line_range="54-61" />
<code_context>
+ return $"{IsEnable},{CriticalDisplayMode},{PerfectDisplayMode},{GreatDisplayMode},{GoodDisplayMode}";
+ }
+
+ public void Deserialize(string data)
+ {
+ var values = data.Split(',');
+ IsEnable = bool.Parse(values[0]);
+ CriticalDisplayMode = (CriticalDisplayMode)Enum.Parse(typeof(CriticalDisplayMode), values[1]);
+ PerfectDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[2]);
+ GreatDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[3]);
+ GoodDisplayMode = (NormalDisplayMode)Enum.Parse(typeof(NormalDisplayMode), values[4]);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Make `Deserialize` more robust against malformed or older serialized data.
This relies on a perfectly formed string (5 comma‑separated values, valid enum names). Any deviation (empty or shorter string, renamed enum values, corrupted settings) will throw and can break startup/settings loading.
Please add basic guards: verify `values.Length >= 5`, use `bool.TryParse` / `Enum.TryParse(..., ignoreCase: true, out ...)`, and on failure either keep existing values or use safe defaults. That will improve compatibility with older formats and make the app more resilient to bad data.
</issue_to_address>
### Comment 2
<location path="AquaMai.Mods/UX/JudgeDisplayPro/JudgeDisplayPro.Score.cs" line_range="77-86" />
<code_context>
+ [HarmonyTranspiler]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Failing hard in the transpiler on pattern mismatch can make the whole mod brittle.
Since IL layouts can change between game versions, throwing `InvalidOperationException` on a pattern mismatch may stop the process from starting for a non-critical UX feature. Consider a softer failure: log a warning (including `matches.Length`) and return the original `instructions` when the pattern isn’t exactly 1, so behavior degrades gracefully instead of crashing.
Suggested implementation:
```csharp
if (matches.Length != 1)
{
UnityEngine.Debug.LogWarning($"[JudgeDisplayPro] ResultProcess.OnStart transpiler expected exactly one match for pattern but found {matches.Length}; leaving method unpatched.");
return instructions;
}
```
Because I only see part of the file, you may need to:
1. Adjust the `SEARCH` block to match the exact text of your existing `InvalidOperationException` line(s) if the message differs.
2. Ensure the `matches` symbol is indeed the array/collection whose count you’re checking; if it has a different name or type, update the condition and the interpolated string accordingly.
3. If you prefer a different logging mechanism (e.g., your mod’s own logger instead of `UnityEngine.Debug.LogWarning`), replace the logging call with your project’s standard logging API while keeping the early `return instructions;`.
</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.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
There are 9 total unresolved issues (including 5 from previous reviews).
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53d4124. Configure here.
There was a problem hiding this comment.
Hey - 我发现了 3 个问题,并给出了一些整体性的反馈:
GameSettingsManagerSprites.bundleMap在RegisterBundle中会在锁内被修改,但在GetOptionValueSprite中却在没有任何同步措施的情况下(并且通过 LINQ)被读取;建议使用并发/不可变字典或共享锁来保护读写,并在这一性能敏感路径中避免使用 LINQ,以防止竞态条件并减少每帧的分配。UserSettings.Serialize/Deserialize假定固定的逗号分隔布局,并在没有校验的情况下使用bool.Parse/Enum.Parse;将解析逻辑改得更“防御性”一些(例如长度检查、带默认值的TryParse、版本控制)会让已保存的设置在数据损坏或未来格式变更的情况下更加健壮。CreateScoreCounterSetter假定GameScoreList的Fast/Late属性 setter 永远存在;在游戏更新重命名或移除这些属性时,这有可能导致空引用异常。更安全的做法是对解析得到的MethodInfo做空检查,并在失败时优雅地处理(或记录日志)。
给 AI Agent 的提示
请根据本次代码评审中的评论进行修改:
## 总体评论
- `GameSettingsManagerSprites.bundleMap` 在 `RegisterBundle` 中会在锁内被修改,但在 `GetOptionValueSprite` 中却在没有任何同步措施的情况下(并且通过 LINQ)被读取;建议使用并发/不可变字典或共享锁来保护读写,并在这一性能敏感路径中避免使用 LINQ,以防止竞态条件并减少每帧的分配。
- `UserSettings.Serialize/Deserialize` 假定固定的逗号分隔布局,并在没有校验的情况下使用 `bool.Parse/Enum.Parse`;将解析逻辑改得更“防御性”一些(例如长度检查、带默认值的 `TryParse`、版本控制)会让已保存的设置在数据损坏或未来格式变更的情况下更加健壮。
- `CreateScoreCounterSetter` 假定 `GameScoreList` 的 `Fast/Late` 属性 setter 永远存在;在游戏更新重命名或移除这些属性时,这有可能导致空引用异常。更安全的做法是对解析得到的 `MethodInfo` 做空检查,并在失败时优雅地处理(或记录日志)。
## 具体评论
### 评论 1
<location path="AquaMai.Mods/UX/JudgeDisplayPro/Models.cs" line_range="60-69" />
<code_context>
+ public void Deserialize(string data)
</code_context>
<issue_to_address>
**suggestion:** `Deserialize` 假定数据格式始终正确,这会在遇到格式错误或旧版本数据时抛出异常;建议使用更防御性的解析方式。
`Deserialize` 假定 `data` 总是合法:
```csharp
var values = data.Split(',');
IsEnable = bool.Parse(values[0]);
CriticalDisplayMode = (CriticalDisplayMode)Enum.Parse(..., values[1]);
PerfectDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[2]);
if (values.Length >= 6)
{
BreakPerfectDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[3]);
GreatDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[4]);
GoodDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[5]);
}
else
{
BreakPerfectDisplayMode = PerfectDisplayMode;
GreatDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[3]);
GoodDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[4]);
}
```
损坏、截断或版本不匹配的输入可能导致 `IndexOutOfRangeException`,或者在 `Enum.Parse` 时抛出 `ArgumentException`。建议在访问每个索引前检查 `values.Length`,并使用 `TryParse` 配合安全默认值,这样即便设置字符串有问题,也不会让整个反序列化过程失败。
</issue_to_address>
### 评论 2
<location path="AquaMai.Core/Helpers/GameSettingsManagerSprites.cs" line_range="11-18" />
<code_context>
+
+public class GameSettingsManagerSprites
+{
+ private static Dictionary<string, AssetBundle> bundleMap = new();
+ private static bool isPatched = false;
+ private static readonly object patchLock = new();
+
+ public static void RegisterBundle(string prefix, AssetBundle bundle)
+ {
+ if(bundleMap.ContainsKey(prefix)) return;
+ bundleMap.Add(prefix, bundle);
+ lock(patchLock)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 在存在并发读取的情况下,Bundle 注册过程不是线程安全的;建议对写操作加锁。
`RegisterBundle` 只在处理 `isPatched`/`ApplyPatch` 时使用了 `patchLock`,但对 `bundleMap` 的修改没有同步:
```csharp
if (bundleMap.ContainsKey(prefix)) return;
bundleMap.Add(prefix, bundle);
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManagerSprites));
}
}
```
`GetOptionValueSprite` 通过 `FirstOrDefault` 枚举 `bundleMap`,因此并发注册可能导致竞态条件,或者在枚举期间修改字典从而触发 `InvalidOperationException`。建议将 `ContainsKey`/`Add` 包裹在同一个锁中,或改用 `ConcurrentDictionary` 来保证线程安全访问。
建议实现如下:
```csharp
public static void RegisterBundle(string prefix, AssetBundle bundle)
{
lock (patchLock)
{
if (bundleMap.ContainsKey(prefix)) return;
bundleMap.Add(prefix, bundle);
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManagerSprites));
}
}
}
```
在 `GetOptionValueSprite`(以及其他读者)中,`bundleMap` 仍然是在没有同步的情况下被读取和枚举。为了完全避免竞态条件和在枚举期间的 `InvalidOperationException`,可以:
1. 将所有对 `bundleMap` 的读取/枚举都放到 `lock (patchLock) { ... }` 中,或者
2. 将 `Dictionary<string, AssetBundle>` 替换为 `ConcurrentDictionary<string, AssetBundle>`,并相应调整用法(例如使用 `TryAdd`、`TryGetValue`,并直接枚举 `ConcurrentDictionary` 实例)。
</issue_to_address>
### 评论 3
<location path="AquaMai.Mods/UX/JudgeDisplayPro/SettingsEntryBase.cs" line_range="9-11" />
<code_context>
+{
+ public string GetSpriteFile(int player)
+ {
+ var suffix = GetSpriteSuffix(player);
+ if(suffix == null) return "UI_OPT_00_00";
+ return "AQM_JudgeDisplayPro_" + GetSpriteSuffix(player);
+ }
+
</code_context>
<issue_to_address>
**nitpick:** 避免重复调用 `GetSpriteSuffix`,并确保复用已计算的值。
在 `GetSpriteFile` 中虽然计算了 `suffix`,却没有复用它:
```csharp
var suffix = GetSpriteSuffix(player);
if (suffix == null) return "UI_OPT_00_00";
return "AQM_JudgeDisplayPro_" + GetSpriteSuffix(player);
```
建议复用缓存值,既避免多余调用,又能保证空检查与返回值的一致性:
```csharp
var suffix = GetSpriteSuffix(player);
return suffix == null
? "UI_OPT_00_00"
: "AQM_JudgeDisplayPro_" + suffix;
```
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会基于你的反馈改进后续的评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- GameSettingsManagerSprites.bundleMap is mutated under a lock in RegisterBundle but read without any synchronization (and via LINQ) in GetOptionValueSprite; consider using a concurrent/immutable dictionary or shared lock and avoiding LINQ in this hot path to prevent race conditions and reduce per-frame allocations.
- UserSettings.Serialize/Deserialize assumes a fixed comma-separated layout and uses bool.Parse/Enum.Parse without validation; making this parsing more defensive (e.g., length checks, TryParse with defaults, versioning) would make saved settings more robust to corruption or future format changes.
- CreateScoreCounterSetter assumes the GameScoreList Fast/Late property setters always exist; it may be safer to null-check the resolved MethodInfo and fail gracefully (or log) instead of risking a null reference if game updates rename or remove these properties.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GameSettingsManagerSprites.bundleMap is mutated under a lock in RegisterBundle but read without any synchronization (and via LINQ) in GetOptionValueSprite; consider using a concurrent/immutable dictionary or shared lock and avoiding LINQ in this hot path to prevent race conditions and reduce per-frame allocations.
- UserSettings.Serialize/Deserialize assumes a fixed comma-separated layout and uses bool.Parse/Enum.Parse without validation; making this parsing more defensive (e.g., length checks, TryParse with defaults, versioning) would make saved settings more robust to corruption or future format changes.
- CreateScoreCounterSetter assumes the GameScoreList Fast/Late property setters always exist; it may be safer to null-check the resolved MethodInfo and fail gracefully (or log) instead of risking a null reference if game updates rename or remove these properties.
## Individual Comments
### Comment 1
<location path="AquaMai.Mods/UX/JudgeDisplayPro/Models.cs" line_range="60-69" />
<code_context>
+ public void Deserialize(string data)
</code_context>
<issue_to_address>
**suggestion:** `Deserialize` assumes well-formed data and can throw on malformed or older values; consider defensive parsing.
`Deserialize` assumes `data` is always valid:
```csharp
var values = data.Split(',');
IsEnable = bool.Parse(values[0]);
CriticalDisplayMode = (CriticalDisplayMode)Enum.Parse(..., values[1]);
PerfectDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[2]);
if (values.Length >= 6)
{
BreakPerfectDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[3]);
GreatDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[4]);
GoodDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[5]);
}
else
{
BreakPerfectDisplayMode = PerfectDisplayMode;
GreatDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[3]);
GoodDisplayMode = (NormalDisplayMode)Enum.Parse(..., values[4]);
}
```
Corrupted, truncated, or version-mismatched input can cause `IndexOutOfRangeException` or `ArgumentException` from `Enum.Parse`. Consider checking `values.Length` before each index and using `TryParse` with safe defaults so a bad settings string doesn’t break deserialization entirely.
</issue_to_address>
### Comment 2
<location path="AquaMai.Core/Helpers/GameSettingsManagerSprites.cs" line_range="11-18" />
<code_context>
+
+public class GameSettingsManagerSprites
+{
+ private static Dictionary<string, AssetBundle> bundleMap = new();
+ private static bool isPatched = false;
+ private static readonly object patchLock = new();
+
+ public static void RegisterBundle(string prefix, AssetBundle bundle)
+ {
+ if(bundleMap.ContainsKey(prefix)) return;
+ bundleMap.Add(prefix, bundle);
+ lock(patchLock)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Bundle registration is not thread-safe while the map is read concurrently; consider locking around writes.
`RegisterBundle` uses `patchLock` only for `isPatched`/`ApplyPatch`, but `bundleMap` is modified without synchronization:
```csharp
if (bundleMap.ContainsKey(prefix)) return;
bundleMap.Add(prefix, bundle);
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManagerSprites));
}
}
```
`GetOptionValueSprite` enumerates `bundleMap` via `FirstOrDefault`, so concurrent registration could cause race conditions or `InvalidOperationException` from modifying the dictionary during enumeration. Consider guarding the `ContainsKey`/`Add` with the same lock or switching to a `ConcurrentDictionary` to ensure thread-safe access.
Suggested implementation:
```csharp
public static void RegisterBundle(string prefix, AssetBundle bundle)
{
lock (patchLock)
{
if (bundleMap.ContainsKey(prefix)) return;
bundleMap.Add(prefix, bundle);
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManagerSprites));
}
}
}
```
`bundleMap` is still read without synchronization in `GetOptionValueSprite` (and any other readers). To fully avoid race conditions and `InvalidOperationException` during enumeration, either:
1. Wrap all reads/enumerations of `bundleMap` in `lock (patchLock) { ... }`, or
2. Replace `Dictionary<string, AssetBundle>` with `ConcurrentDictionary<string, AssetBundle>` and update uses accordingly (e.g., use `TryAdd`, `TryGetValue`, and enumerate the `ConcurrentDictionary` instance directly).
</issue_to_address>
### Comment 3
<location path="AquaMai.Mods/UX/JudgeDisplayPro/SettingsEntryBase.cs" line_range="9-11" />
<code_context>
+{
+ public string GetSpriteFile(int player)
+ {
+ var suffix = GetSpriteSuffix(player);
+ if(suffix == null) return "UI_OPT_00_00";
+ return "AQM_JudgeDisplayPro_" + GetSpriteSuffix(player);
+ }
+
</code_context>
<issue_to_address>
**nitpick:** Avoid calling `GetSpriteSuffix` twice and ensure consistent use of the computed value.
`suffix` is computed but not reused in `GetSpriteFile`:
```csharp
var suffix = GetSpriteSuffix(player);
if (suffix == null) return "UI_OPT_00_00";
return "AQM_JudgeDisplayPro_" + GetSpriteSuffix(player);
```
Reuse the cached value to avoid the redundant call and keep the null check and returned value consistent:
```csharp
var suffix = GetSpriteSuffix(player);
return suffix == null
? "UI_OPT_00_00"
: "AQM_JudgeDisplayPro_" + suffix;
```
</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.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Starrah <starrah@foxmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…nto feat/JudgeDisplayPro
There was a problem hiding this comment.
我本来想进一步重构一下这个文件,在PostJudgeGradeInitialize里判断是不是绝赞、直接用正确的isBreak参数调用ApplyNormalJudgeGradeDisplay和ApplyCriticalJudgeGradeDisplay,从而避免PostJudgeGradeInitializeBreak里的相似代码的。
然后看了半天发现,JudgeGrade里面的实现混乱无比,没有一个稳定的变量用于判定一个音符是不是绝赞。(NoteBase.JudgeType对保护套绝赞会直接输出EJudgeType.ExTap)。很明显就是fes当年新增保护套绝赞的时候乱搞导致的。SBGA程序员这一块。
综合想了一下,现在这种写法已经比较好了,很难找到一个完全比现在好的写法
…音符时才打开spriteRenderAdd。 然后给一些比较复杂和边界的逻辑增加了注释(和小重构),也许稍微增强了代码的可读性
|
但是CI怎么一直在炸,这个可能得您看看 @clansty |
|
CI 是上次加固导致的,因为 PR 相关的没怎么测试 |
反正是build CI又不是release,直接推到一个新分支上测,测完了squash回主分支就好吧() |
|
我也 LGTM |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体层面的反馈:
- 在
GameSettingsManagerSprites中,RegisterBundle会在加锁的情况下修改bundleMap,但GetOptionValueSprite在读取时没有做任何同步;如果在游戏开始后仍然可能注册新的 bundle,建议把bundleMap改成ConcurrentDictionary,或者至少在读取时也加锁,以避免潜在的字典损坏。 - 当 IL 模式数量不等于 1 时,
ResultProcessOnStartTranspiler会抛出InvalidOperationException,这会在游戏更新时导致硬崩溃;你可能希望在这种情况下只记录一个警告并跳过打补丁,这样游戏仍然可以运行(只是没有增强的判定显示集成)。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- 在 `GameSettingsManagerSprites` 中,`RegisterBundle` 会在加锁的情况下修改 `bundleMap`,但 `GetOptionValueSprite` 在读取时没有做任何同步;如果在游戏开始后仍然可能注册新的 bundle,建议把 `bundleMap` 改成 `ConcurrentDictionary`,或者至少在读取时也加锁,以避免潜在的字典损坏。
- 当 IL 模式数量不等于 1 时,`ResultProcessOnStartTranspiler` 会抛出 `InvalidOperationException`,这会在游戏更新时导致硬崩溃;你可能希望在这种情况下只记录一个警告并跳过打补丁,这样游戏仍然可以运行(只是没有增强的判定显示集成)。
## Individual Comments
### Comment 1
<location path="AquaMai.Core/Helpers/GameSettingsManager.cs" line_range="37-46" />
<code_context>
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
- if (!isPatched)
+ lock (patchLock)
{
- isPatched = true;
- Startup.ApplyPatch(typeof(GameSettingsManager));
- Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
+ if (!isPatched)
+ {
+ isPatched = true;
+ Startup.ApplyPatch(typeof(GameSettingsManager));
+ Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
+ }
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 考虑使用同一个锁来保护 settings 列表,以避免在多线程注册时发生竞争。
`settings.Add` 和 `settings.Sort` 仍然在没有同步的情况下执行,因此来自多个线程的并发注册可能会产生竞争或抛出异常(例如在排序时抛出 `InvalidOperationException`)。请将 `lock (patchLock)` 扩展到这些列表操作,或者为 `settings` 添加一个专门的锁,并在注册和枚举时都一致地使用它来保证线程安全。
建议实现如下:
```csharp
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
```
` 因为它们没有在这里展示)。
以下是具体的编辑:
<file_operations>
<file_operation operation="edit" file_path="AquaMai.Core/Helpers/GameSettingsManager.cs">
<<<<<<< SEARCH
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
=======
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
为了实现完全线程安全,任何读取、枚举或修改 `settings` 的其他方法也应在 `patchLock` 上进行同步。例如,如果有类似 `GetSettings()`、`foreach (var s in settings)` 的方法,或者其他修改操作(`Clear`、`Remove` 等),都应该在访问时使用同一个锁对象 `lock (patchLock)` 包裹,以避免数据竞争以及在枚举期间发生 `InvalidOperationException`。
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
GameSettingsManagerSprites,bundleMapis mutated under a lock inRegisterBundlebut read without synchronization inGetOptionValueSprite; if bundles can be registered after gameplay has started, consider makingbundleMapaConcurrentDictionaryor at least locking on reads to avoid potential dictionary corruption. ResultProcessOnStartTranspilerthrows anInvalidOperationExceptionwhen the IL pattern count is not exactly 1, which will hard-crash on game updates; you might want to log a warning and skip patching in that case so the game still runs (just without the enhanced judge display integration).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GameSettingsManagerSprites`, `bundleMap` is mutated under a lock in `RegisterBundle` but read without synchronization in `GetOptionValueSprite`; if bundles can be registered after gameplay has started, consider making `bundleMap` a `ConcurrentDictionary` or at least locking on reads to avoid potential dictionary corruption.
- `ResultProcessOnStartTranspiler` throws an `InvalidOperationException` when the IL pattern count is not exactly 1, which will hard-crash on game updates; you might want to log a warning and skip patching in that case so the game still runs (just without the enhanced judge display integration).
## Individual Comments
### Comment 1
<location path="AquaMai.Core/Helpers/GameSettingsManager.cs" line_range="37-46" />
<code_context>
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
- if (!isPatched)
+ lock (patchLock)
{
- isPatched = true;
- Startup.ApplyPatch(typeof(GameSettingsManager));
- Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
+ if (!isPatched)
+ {
+ isPatched = true;
+ Startup.ApplyPatch(typeof(GameSettingsManager));
+ Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
+ }
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider protecting the settings list with the same lock to avoid races when registering from multiple threads.
`settings.Add` and `settings.Sort` still run without synchronization, so concurrent registration from multiple threads can race or throw (e.g., `InvalidOperationException` during sort). Please either extend `lock (patchLock)` to cover these list operations, or add a dedicated lock for `settings` and use it consistently for registration and enumeration to ensure thread safety.
Suggested implementation:
```csharp
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
```
` since they’re not shown here).
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="AquaMai.Core/Helpers/GameSettingsManager.cs">
<<<<<<< SEARCH
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
=======
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To be fully thread-safe, any other methods that read, enumerate, or modify `settings` should also synchronize on `patchLock`. For example, if there are methods like `GetSettings()`, `foreach (var s in settings)`, or other mutations (`Clear`, `Remove`, etc.), those should wrap access in `lock (patchLock)` using the same lock object to avoid data races and `InvalidOperationException` during enumeration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| settings.Add(setting); | ||
| settings.Sort((a, b) => a.Sort.CompareTo(b.Sort)); | ||
| if (!isPatched) | ||
| lock (patchLock) | ||
| { | ||
| isPatched = true; | ||
| Startup.ApplyPatch(typeof(GameSettingsManager)); | ||
| Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension)); | ||
| if (!isPatched) | ||
| { | ||
| isPatched = true; | ||
| Startup.ApplyPatch(typeof(GameSettingsManager)); | ||
| Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension)); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): 考虑使用同一个锁来保护 settings 列表,以避免在多线程注册时发生竞争。
settings.Add 和 settings.Sort 仍然在没有同步的情况下执行,因此来自多个线程的并发注册可能会产生竞争或抛出异常(例如在排序时抛出 InvalidOperationException)。请将 lock (patchLock) 扩展到这些列表操作,或者为 settings 添加一个专门的锁,并在注册和枚举时都一致地使用它来保证线程安全。
建议实现如下:
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}` 因为它们没有在这里展示)。
以下是具体的编辑:
<file_operations>
<file_operation operation="edit" file_path="AquaMai.Core/Helpers/GameSettingsManager.cs">
<<<<<<< SEARCH
private static readonly List settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
REPLACE
</file_operation>
</file_operations>
<additional_changes>
为了实现完全线程安全,任何读取、枚举或修改 settings 的其他方法也应在 patchLock 上进行同步。例如,如果有类似 GetSettings()、foreach (var s in settings) 的方法,或者其他修改操作(Clear、Remove 等),都应该在访问时使用同一个锁对象 lock (patchLock) 包裹,以避免数据竞争以及在枚举期间发生 InvalidOperationException。
</additional_changes>
Original comment in English
suggestion (bug_risk): Consider protecting the settings list with the same lock to avoid races when registering from multiple threads.
settings.Add and settings.Sort still run without synchronization, so concurrent registration from multiple threads can race or throw (e.g., InvalidOperationException during sort). Please either extend lock (patchLock) to cover these list operations, or add a dedicated lock for settings and use it consistently for registration and enumeration to ensure thread safety.
Suggested implementation:
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}` since they’re not shown here).
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="AquaMai.Core/Helpers/GameSettingsManager.cs">
<<<<<<< SEARCH
private static readonly List settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
lock (patchLock)
{
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
private static readonly List<IPlayerSettingsItem> settings = [];
public static void RegisterSetting(IPlayerSettingsItem setting)
{
lock (patchLock)
{
settings.Add(setting);
settings.Sort((a, b) => a.Sort.CompareTo(b.Sort));
if (!isPatched)
{
isPatched = true;
Startup.ApplyPatch(typeof(GameSettingsManager));
Startup.ApplyPatch(typeof(PatchOptionCategoryIDExtension));
}
}
}
REPLACE
</file_operation>
</file_operations>
<additional_changes>
To be fully thread-safe, any other methods that read, enumerate, or modify settings should also synchronize on patchLock. For example, if there are methods like GetSettings(), foreach (var s in settings), or other mutations (Clear, Remove, etc.), those should wrap access in lock (patchLock) using the same lock object to avoid data races and InvalidOperationException during enumeration.


Note
Medium Risk
Wide Harmony surface on judge rendering, scoring, and result IL; incorrect patches could skew FAST/LATE stats or break on game updates, though behavior is gated by per-player enable flags.
Overview
Adds JudgeDisplayPro, a per-player mod that overrides tap, touch, slide, and break judge sprites and visibility from new in-game option rows, with settings persisted across music select.
Gameplay integration: Harmony hooks on
JudgeGrade,SlideJudge, and related paths apply display modes (judge-only, FAST/LATE, colored, hidden) and critical-perfect rules; touch judges get monitor index via thread-local state from parent notes, slides viaSlideRootbindings. Score handling setsDispJudgefrom critical mode, adjusts FAST/LATE counts when timing is shown, and transpilesResultProcess.OnStartto read judge display fromGameScoreListinstead of raw user data.Infrastructure:
GameSettingsManagerpatches under a lock; newGameSettingsManagerSpritespatchesMusicSelectProcess.GetOptionValueSpritefor custom option preview sprites from a registered asset bundle (judgedisplayproembedded resource).SongConstantSortdrops unused compressedlevel.abfallback;level.abpath moves underResources/.Reviewed by Cursor Bugbot for commit 9d0a300. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by Sourcery
引入可配置的 JudgeDisplayPro 用户体验(UX)Mod,用于自定义游戏内判定视觉效果,并让分数/统计信息显示与画面中显示的内容保持一致。
新功能:
GameSettingsManagerSprites从内嵌的 asset bundle 加载自定义选项贴图(sprites)。UserSettings模型,在不同游戏会话之间为每位玩家持久化 JudgeDisplayPro 设置。改进:
GameSettingsManager的补丁注册线程安全。GameScoreList打补丁以及对ResultProcess.OnStart进行 transpile,使 FAST/LATE 计数器和结算画面的判定显示与 JudgeDisplayPro 行为保持一致。构建:
judgedisplayproasset bundle 注册为内嵌资源,并通过移除未使用的compressed level.ab回退选项,简化SongConstantSortasset bundle 的加载逻辑。Original summary in English
Summary by Sourcery
Introduce a configurable JudgeDisplayPro UX mod that customizes in-game judge visuals and keeps score/stat displays consistent with what is shown.
New Features:
Enhancements:
Build:
Original summary in English
Summary by Sourcery
引入可配置的 JudgeDisplayPro 用户体验(UX)Mod,用于自定义游戏内判定视觉效果,并让分数/统计信息显示与画面中显示的内容保持一致。
新功能:
GameSettingsManagerSprites从内嵌的 asset bundle 加载自定义选项贴图(sprites)。UserSettings模型,在不同游戏会话之间为每位玩家持久化 JudgeDisplayPro 设置。改进:
GameSettingsManager的补丁注册线程安全。GameScoreList打补丁以及对ResultProcess.OnStart进行 transpile,使 FAST/LATE 计数器和结算画面的判定显示与 JudgeDisplayPro 行为保持一致。构建:
judgedisplayproasset bundle 注册为内嵌资源,并通过移除未使用的compressed level.ab回退选项,简化SongConstantSortasset bundle 的加载逻辑。Original summary in English
Summary by Sourcery
Introduce a configurable JudgeDisplayPro UX mod that customizes in-game judge visuals and keeps score/stat displays consistent with what is shown.
New Features:
Enhancements:
Build: