Conversation
Summary of ChangesHello @WUGqnwvMQPzl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求引入了一个新模组，允许用户在游戏标题画面播放自定义的视频和音频文件。通过替换默认的标题动画，它为玩家提供了更丰富的个性化选项，并旨在重现旧版本游戏中曾有的标题视频功能。该模组通过HarmonyLib对游戏广告流程进行修改，以实现视频和音频的无缝集成和管理。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
这个 PR 实现了一个很棒的功能，允许在标题画面播放自定义视频。代码结构清晰，并利用 Harmony 来修改游戏行为，实现了预期的效果。
在代码审查中，我发现了一些可以改进的地方，主要集中在并发处理、资源管理和代码健壮性方面。具体来说，存在一个关于视频准备状态的竞态条件，可能会导致不稳定的行为；还有一个小的内存泄漏问题，与动态创建的材质有关。此外，一些硬编码的值和口语化的注释也可以优化，以提高代码的可读性和可维护性。
请查看我提出的具体建议，这些修改将有助于提高这个功能的稳定性和代码质量。
|
|
||
| private static List<string>[] _disabledCompoments = [[], []]; | ||
|
|
||
| private static bool _isVideoPrepared = false; |
There was a problem hiding this comment.
_isVideoPrepared 是一个静态布尔值，但它被用于处理两个显示器上两个独立的异步视频准备操作。这会引发竞态条件。例如，如果一个视频准备成功（将 _isVideoPrepared 设为 true），而另一个失败（将其设为 false），最终状态将取决于哪个事件最后触发。这可能导致行为不一致，比如一个准备好的视频没有播放。
为了解决这个问题，建议不要使用单一的布尔值。可以考虑使用一个计数器来跟踪已准备好的视频数量，并用另一个布尔标志来表示是否发生了任何错误。
例如:
private static int _preparedVideoCount = 0;
private static volatile bool _videoHasError = false;然后在 OnStart 中重置它们，在 prepareCompleted 回调中使用 Interlocked.Increment 来增加计数器，在 errorReceived 回调中设置错误标志。最后，在 OnUpdate 中根据这两个变量的组合来决定逻辑。
|
|
||
| _videoPlayers[i].Prepare(); | ||
|
|
||
| var movieMaterial = new Material(Shader.Find("Sprites/Default")); |
There was a problem hiding this comment.
这里使用 new Material(...) 创建了一个新的材质实例，但它从未被销毁，这会导致内存泄漏。每次进入标题画面都会泄漏一个新的材质。
你应该将创建的材质存储在一个类级别的字段中（例如 private static Material[] _movieMaterials），并在 OnRelease_Prefix 方法中销毁它们，以避免内存泄漏。
| private static GameObject[] _movieObjects = new GameObject[2]; | ||
| private static VideoPlayer[] _videoPlayers = new VideoPlayer[2]; | ||
|
|
||
| private static List<string>[] _disabledCompoments = [[], []]; |
There was a problem hiding this comment.
这些数组的大小被硬编码为2。虽然游戏通常在双显示器上运行，但更好的做法是让代码更具弹性，以防将来监视器数量发生变化。
建议将这些字段声明为未初始化的数组，然后在 OnStart_Postfix 方法开始时，根据 ____monitors.Length 动态地初始化它们。例如：_movieObjects = new GameObject[____monitors.Length];。
| { | ||
| case AdvertiseProcess.AdvertiseSequence.Logo: | ||
| // Re-enable original title screen elements if the video is unavailable | ||
| // yeah calling it early so the switch is unnoticeable |
There was a problem hiding this comment.
注释 “yeah calling it early so the switch is unnoticeable” 过于口语化。建议修改为更清晰地解释为什么在这里执行此操作，以提高代码的可读性。
// This is called early in the Logo sequence to make the fallback to the original title screen seamless.| { | ||
| for (int i = 0; i < ____monitors.Length; ++i) | ||
| { | ||
| // Stop yelling "maimai deluxe" I'm tired to hearing it |
There was a problem hiding this comment.
这条注释 “Stop yelling 'maimai deluxe' I'm tired to hearing it” 过于口语化。建议修改为更专业的描述，解释停止语音的技术原因，例如，为了防止与自定义音频重叠。
// Stop the "maimai deluxe" voiceover to prevent it from overlapping with the custom audio.| { | ||
| // Stop and unloads title music | ||
| SoundManager.StopMusic(); | ||
| Singleton<SoundCtrl>.Instance.UnloadCueSheet(1); |
There was a problem hiding this comment.
UnloadCueSheet(1) 中的数字 1 是一个“魔术数字”。它的含义不明确，这会使代码难以理解和维护。建议将其定义为一个有描述性名称的常量（例如 private const int CustomMusicCueSheetId = 1;），并在调用时使用该常量，以提高代码的可读性。
| _videoPlayers[i].prepareCompleted -= null; | ||
| _videoPlayers[i].errorReceived -= null; |
There was a problem hiding this comment.
使用 -= null 来取消订阅事件是无效的，这段代码实际上什么也没做。虽然在这种情况下，销毁 VideoPlayer 组件可能足以防止内存泄漏，但这行代码是错误的，并且会产生误导。建议直接删除这两行代码。
| // So uhh... When I was testing the feature, this method makes title screen suddently go black before transition | ||
| // I don't like the sudden cutout so I disabled it, not sure about the side effect or compatibility though |
There was a problem hiding this comment.
这里的注释 “So uhh...” 以及后面的解释过于口语化。建议使用更专业、更具技术性的语言来解释为什么需要禁用此方法。一个好的注释应该清楚地说明问题和解决方案，以便其他开发者理解。
// The original AllStop method causes the screen to turn black abruptly before the transition.
// Returning false here skips the original method to provide a smoother visual transition.1 participant
No description provided.