Skip to content

[3/8] Preserve MGS1 configuration and Integral language choices - #115

Merged
nuggslet merged 3 commits into
nuggslet:masterfrom
TideGear:pr/03-configuration-language
Sep 14, 2026
Merged

nuggslet merged 3 commits into
nuggslet:masterfrom
TideGear:pr/03-configuration-language

Conversation

@TideGear

@TideGear TideGear commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Collection initialization could reset MGS1 configuration or Integral language choices. Preserve requested configuration and apply the English-text language choice at the relevant game hooks. Add the required RAM hooks and configuration documentation.

Review order

PR 3 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: #114

Review only this step

Validation

  • This branch passed Release x64 and x86 builds with the development installer disabled.
  • Standalone native RAM repair tests passed for both architectures.

Ready for maintainer review. The documented gameplay testing remains outstanding; no gameplay pass is claimed.

Complete series

  1. [1/8] Build Zydis with the selected toolset and invalidate stale caches #113
  2. [2/8] Validate PPF3 files and preserve executable RAM patches #114
  3. [3/8] Preserve MGS1 configuration and Integral language choices #115
  4. [4/8] Correct USA brightness help with scoped disc patches #116
  5. [5/8] Correct thin textured polygon sampling in MGS1 #117
  6. [6/8] Add Integral English and grenade patches with the complete toolchain #118
  7. [7/8] Add optional MGS1 and VR testing aids #119
  8. [8/8] Add patch watches, RAM auditing and debugger tools #120

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38bc200492

ℹ️ 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".

Comment thread src/games/mgs1.cpp
// 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid recursively hooking the configuration snapshot read

With the default PreserveConfiguration enabled, a collection call to getRamValue(16, MGS1_ConfigPTR) enters SQNative_getRamValue, which calls this callback; this line then invokes the same hooked getRamValue again before the callback can return. Every nested invocation satisfies the same condition, causing unbounded recursion and a stack overflow instead of capturing the configuration. Read through an unhooked path or add a reentrancy guard around the internal read.

Useful? React with 👍 / 👎.

@nuggslet nuggslet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - weird .ini newline padding but I'll adjust it if I'm not keen :P

@nuggslet
nuggslet changed the base branch from master to pr/02-ketchup-reliability September 14, 2026 03:45
@nuggslet
nuggslet changed the base branch from pr/02-ketchup-reliability to master September 14, 2026 07:18
@nuggslet
nuggslet merged commit d01b24d into nuggslet:master Sep 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants