PCSX2 injection support for vanilla JP KH1, vanilla USA KH1 and JP ReCoM#1255
Open
kikeprime wants to merge 6 commits intoOpenKH:masterfrom
Open
PCSX2 injection support for vanilla JP KH1, vanilla USA KH1 and JP ReCoM#1255kikeprime wants to merge 6 commits intoOpenKH:masterfrom
kikeprime wants to merge 6 commits intoOpenKH:masterfrom
Conversation
Mentioning the new JP ReCoM support.
Accept JP ReCoM ISO.
Added JP ReCoM support. Some instruction parameters had to be changed in the hook functions so I duplicated them. A more elegant solution is welcome.
📝 WalkthroughWalkthroughAdds JP-region detection and PCSX2 injection offsets/hooks for Kingdom Hearts Re:Chain of Memories (Recom), plus a UI label update to indicate JP and US ISO availability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs (1)
487-546: Consider parameterizing JP/US hook generation to avoid drift.These two JP arrays are near-duplicates of existing hooks with shifted immediates. A small builder method would keep behavior identical and reduce maintenance risk when hooks evolve.
♻️ Refactor sketch
- private static readonly uint[] LoadFileAsyncHook = new uint[] { ... }; - private static readonly uint[] LoadFileAsyncHookJp = new uint[] { ... }; + private static uint[] BuildLoadFileAsyncHook(short oA, short oB, short oC, short oD) => new uint[] + { + LUI(T6, HookStack), + LW(T2, V0, oA), + LW(T3, V0, oB), + ADDI(T4, V0, oC), + ... + LW(A2, V0, oD), + ... + }; + private static readonly uint[] LoadFileAsyncHook = BuildLoadFileAsyncHook(-0x2A10, -0x2A24, -0x29F8, -0x29F4); + private static readonly uint[] LoadFileAsyncHookJp = BuildLoadFileAsyncHook(0x18B0, 0x189C, 0x18C8, 0x18CC); - private static readonly uint[] GetFileSizeRecomHook = new uint[] { ... }; - private static readonly uint[] GetFileSizeRecomHookJp = new uint[] { ... }; + private static uint[] BuildGetFileSizeRecomHook(short fileDirOffset) => new uint[] + { + LUI(T6, HookStack), + LUI(V1, 0x5C), + ADDI(T4, V1, fileDirOffset), + ... + }; + private static readonly uint[] GetFileSizeRecomHook = BuildGetFileSizeRecomHook(-0x29F8); + private static readonly uint[] GetFileSizeRecomHookJp = BuildGetFileSizeRecomHook(0x18C8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 487 - 546, The two JP hook arrays (LoadFileAsyncHookJp and GetFileSizeRecomHookJp) are near-duplicates of US hooks with only immediate/address shifts; create a small builder method (e.g., BuildHookSequence or BuildJpHook) that accepts the varying immediates/offsets (V1 high words like 0x5B/0x5C, the AddI/AddIU immediates such as 0x18C8/0x18CC, and any differing constant like Operation.GetFileSizeRecom) and returns the uint[] of instructions using the same instruction helpers (LUI, LW, SW, BEQ, BNE, ADDI, ADDIU, JR, NOP, etc.); replace the hardcoded arrays LoadFileAsyncHookJp and GetFileSizeRecomHookJp with calls to that builder passing the specific constants (maintain use of HookStack, Param1/Param2/ParamOperator/ParamReturn, and registers T0..T6, V0/V1, S1/S2, RA) so behavior stays identical while preventing drift when the common hook template changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs`:
- Around line 487-546: The two JP hook arrays (LoadFileAsyncHookJp and
GetFileSizeRecomHookJp) are near-duplicates of US hooks with only
immediate/address shifts; create a small builder method (e.g., BuildHookSequence
or BuildJpHook) that accepts the varying immediates/offsets (V1 high words like
0x5B/0x5C, the AddI/AddIU immediates such as 0x18C8/0x18CC, and any differing
constant like Operation.GetFileSizeRecom) and returns the uint[] of instructions
using the same instruction helpers (LUI, LW, SW, BEQ, BNE, ADDI, ADDIU, JR, NOP,
etc.); replace the hardcoded arrays LoadFileAsyncHookJp and
GetFileSizeRecomHookJp with calls to that builder passing the specific constants
(maintain use of HookStack, Param1/Param2/ParamOperator/ParamReturn, and
registers T0..T6, V0/V1, S1/S2, RA) so behavior stays identical while preventing
drift when the common hook template changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f06c1840-0840-438b-96d1-5ac5e33bbebf
📒 Files selected for processing (3)
OpenKh.Tools.ModsManager/Services/GameService.csOpenKh.Tools.ModsManager/Services/Pcsx2Injector.csOpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
Both vanilla JP and USA KH1 needs a smaller skip jump than KH1FM. I did the same duplicate method as for JP ReCoM.
Vanilla JP and USA versions of KH1 are now valid KH1 ISOs for the ISO selector.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs (2)
938-945: Consider extracting repeated hook-injection boilerplate.The three new conditional blocks repeat the same
Log.Info+WritePatch(... JAL(WriteHook(...)), ADDIU(...))pattern. A small helper would reduce copy/paste risk as more variants are added.Also applies to: 955-961, 998-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 938 - 945, Extract the repeated injection boilerplate into a small helper method (e.g., InjectHook) and replace the three repeated blocks with calls to it; the helper should take the stream, the target offset (like offsets.LoadFileTaskVanilla), a hook writer/delegate (like LoadFileTaskHookVanilla passed to WriteHook), the operation enum value (Operation.LoadFileTask) and a name for logging, then perform the Log.Info and the WritePatch(stream, offset, ADDIU(...), JAL(WriteHook(...)), ADDIU(...)) sequence using WriteHook and WritePatch so callers simply call InjectHook(stream, offsets.LoadFileTaskVanilla, LoadFileTaskHookVanilla, Operation.LoadFileTask, nameof(offsets.LoadFileTaskVanilla)); update the other occurrences (lines referencing offsets.* and their respective Hook and Operation) to use this helper.
427-455: ParameterizeLoadFileTaskhook construction to avoid code drift.
LoadFileTaskHookandLoadFileTaskHookVanillaare identical except the skip immediate at Line 451 (0x64vs0x98in the other hook). This is a good candidate for a small hook-builder helper.♻️ Suggested refactor
- private static readonly uint[] LoadFileTaskHook = new uint[] - { - ... - ADDIU(RA, RA, 0x98), - ... - }; - - private static readonly uint[] LoadFileTaskHookVanilla = new uint[] - { - ... - ADDIU(RA, RA, 0x64), - ... - }; + private static uint[] BuildLoadFileTaskHook(short skipImmediate) => new uint[] + { + LUI(T6, HookStack), + SW(S1, T6, Param1), + SW(S0, T6, Param2), + SW(V0, T6, Param3), + SW(T5, T6, ParamOperator), + LW(T5, T6, ParamOperator), + BNE(T5, (byte)Operation.HookExit, -2), + LW(V1, T6, ParamReturn), + BEQ(V1, Zero, 3), + MOVE(S2, V0), + BEQ(Zero, Zero, 2), + ADDIU(RA, RA, skipImmediate), + LI(V0, -1), + JR(RA), + NOP(), + }; + + private static readonly uint[] LoadFileTaskHook = BuildLoadFileTaskHook(0x98); + private static readonly uint[] LoadFileTaskHookVanilla = BuildLoadFileTaskHook(0x64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 427 - 455, The two hook arrays LoadFileTaskHook and LoadFileTaskHookVanilla differ only by the skip immediate (ADDIU(RA, RA, 0x64) vs 0x98); refactor by extracting a hook-builder (e.g., BuildLoadFileTaskHook or CreateLoadFileTaskHook) that returns the uint[] and takes the skipImmediate as a parameter, then replace both LoadFileTaskHook and LoadFileTaskHookVanilla with calls to that builder (pass 0x64 and 0x98 respectively); ensure the builder reproduces the exact sequence of instructions (LUI(T6, HookStack), SW(...), LW(...), BNE(...), LW(V1,...), BEQ(...), MOVE(...), BEQ(...), ADDIU(RA, RA, skipImmediate), LI(V0, -1), JR(RA), NOP()) and retains the same identifiers (T6, S1, S0, V0, T5, V1, S2, RA) so all references remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs`:
- Around line 938-945: Extract the repeated injection boilerplate into a small
helper method (e.g., InjectHook) and replace the three repeated blocks with
calls to it; the helper should take the stream, the target offset (like
offsets.LoadFileTaskVanilla), a hook writer/delegate (like
LoadFileTaskHookVanilla passed to WriteHook), the operation enum value
(Operation.LoadFileTask) and a name for logging, then perform the Log.Info and
the WritePatch(stream, offset, ADDIU(...), JAL(WriteHook(...)), ADDIU(...))
sequence using WriteHook and WritePatch so callers simply call
InjectHook(stream, offsets.LoadFileTaskVanilla, LoadFileTaskHookVanilla,
Operation.LoadFileTask, nameof(offsets.LoadFileTaskVanilla)); update the other
occurrences (lines referencing offsets.* and their respective Hook and
Operation) to use this helper.
- Around line 427-455: The two hook arrays LoadFileTaskHook and
LoadFileTaskHookVanilla differ only by the skip immediate (ADDIU(RA, RA, 0x64)
vs 0x98); refactor by extracting a hook-builder (e.g., BuildLoadFileTaskHook or
CreateLoadFileTaskHook) that returns the uint[] and takes the skipImmediate as a
parameter, then replace both LoadFileTaskHook and LoadFileTaskHookVanilla with
calls to that builder (pass 0x64 and 0x98 respectively); ensure the builder
reproduces the exact sequence of instructions (LUI(T6, HookStack), SW(...),
LW(...), BNE(...), LW(V1,...), BEQ(...), MOVE(...), BEQ(...), ADDIU(RA, RA,
skipImmediate), LI(V0, -1), JR(RA), NOP()) and retains the same identifiers (T6,
S1, S0, V0, T5, V1, S2, RA) so all references remain valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5745a92-2026-4e9c-8a8d-77b79dc33084
📒 Files selected for processing (1)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs
Mention vanilla USA and JP KH1 support.
Author
|
In the 3 commits I made today I added support for the vanilla USA and JP versions of KH1. |
kikeprime
changed the title
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.
I added the necessary changes to the Mod Managers code so the PCSX2 injection feature is compatible with the original Japanese PS2 release of ReCoM.
The 2 offsets had to be shifted with +0x90 while the negative instruction parameters in the 2 hook functions with +0x42C0.
I didn't want to touch the structure of the file so for now I duplicated the 2 hook functions but I propose an offset parameter based on the USA release since that was added first.
Summary by CodeRabbit
New Features
UI Updates