Repository navigation
[4/8] Correct USA brightness help with scoped disc patches - #116
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9948069e4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // read itself follows this hook on the same thread, so the two agree. | ||
| if (M2Config::bPatchesPreserveConfiguration && MGS1_ConfigPTR != 0 | ||
| && width == 16 && offset == MGS1_ConfigPTR) { | ||
| MGS1_ConfigSeen = SQEmuTask<Squirk::Standard>::GetRamValue(16, MGS1_ConfigPTR) & 0xFFFF; |
There was a problem hiding this comment.
When PreserveConfiguration is enabled (the new default) and the collection reads GM_Configuration, this callback invokes getRamValue(16, MGS1_ConfigPTR) again before the outer native call has completed. SQEmuTask::GetRamValue goes through the same CallNative/SQNative_getRamValue hook, which re-enters this branch indefinitely and can stack-overflow as soon as the per-frame configuration read occurs. Read through an unhooked path or add a reentrancy guard instead.
Useful? React with 👍 / 👎.
| ApplyBlock(v, title, version, disk, patch.offset, | ||
| patch.data.data(), patch.data.size()); |
There was a problem hiding this comment.
In the default BrightnessText=fixed mode, both built-in offsets are inside the ranges registered by SetPatchRangeBlacklist. Calling ApplyBlock here routes the built-in data through the hooked entryCdRomPatch, where SQNative_entryCdRomPatch matches that same range and returns 1, suppressing the native write. Consequently the collection replacement is removed but the blank-row patch is also discarded, so fixed behaves like original instead of producing the advertised four-line texture.
Useful? React with 👍 / 👎.
|
|
||
| // What to do about the collection's replacement for MGS1's Option -> SCREEN | ||
| // help texture. See [Patches] BrightnessText in the ini. | ||
| enum class M2BrightnessText |
There was a problem hiding this comment.
LGTM, but I question the location of this
9948069 to
12b8499
Compare
Correct the USA Option/SCREEN brightness instructions using built-in patches for the two supported disc layouts. Scope collection-patch filtering to the correct title, disc and byte range, with explicit BrightnessText configuration. Include standalone range tests. Runtime appearance still needs gameplay verification; source/payload checks do not prove runtime routing.
Review order
PR 4 of 8 in the complete TideGear contribution. These are an ordered stack against upstream
master; GitHub shows earlier unmerged changes cumulatively. Merge in order 1 through 8 and update the remaining branches after each merge.Previous PR: #115
Review only this step
Validation
Ready for maintainer review. The documented gameplay testing remains outstanding; no gameplay pass is claimed.
Complete series