Add reusable INI game fix for Imperivm#1009
Add reusable INI game fix for Imperivm#1009utkarshdalal merged 3 commits intoutkarshdalal:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds INI-file-based game-fix infrastructure and registers a new Steam game fix (gameId 752580) that updates Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Registry as GameFixesRegistry
participant Fix as IniFileFix / KeyedIniFileFix
participant FS as FileSystem
participant Container as Container
App->>Registry: applyFor(containerId, context, ...)
Registry->>Registry: resolve fix by (gameSource, gameId)
Registry->>Fix: apply(context, gameId, installPath, container)
Fix->>Container: read migrationKey extra?
alt migration flag == "1"
Fix-->>Registry: return false (skipped)
else
Fix->>FS: read File(installPath, "Settings.ini") (UTF-8)
FS-->>Fix: file content
Fix->>Fix: updateIniValue for each key (regex/append)
Fix->>FS: write updated file (UTF-8) if changed
Fix->>Container: set migrationKey = "1" and save
Fix-->>Registry: return true (applied)
end
Registry-->>App: result
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 |
Misazam
force-pushed
the
fix/imperivm-audio-language
branch
from
0537933 to
3cd90ce
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt">
<violation number="1" location="app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt:84">
P1: Settings migration is marked complete before `Settings.ini` is successfully written, so write failures can permanently skip future `Music=0` enforcement.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:2552">
P2: Game fixes are applied after earlier Wine/system/audio prep, so first launch can run those prep steps with stale pre-fix container settings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt`:
- Around line 82-107: The migration currently sets IMPERIVM_SETTINGS_MIGRATION
and marks changed/updatedMusic before the Settings.ini is actually written, so
on write failure the migration is incorrectly considered complete; fix by
deferring setting container.putExtra(IMPERIVM_SETTINGS_MIGRATION, "1") and
updating updatedMusic/changed until after settingsFile.writeText(...) succeeds
(or only set the migration flag when the file already contained Music=0), i.e.
call updateIniValue and compute the new content, attempt
settingsFile.writeText(updated,...), and only on successful write set
IMPERIVM_SETTINGS_MIGRATION, updatedMusic=true and changed=true (and keep the
existing onFailure block to log errors).
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 2551-2560: The code reparses the in-memory container JSON
(container.containerJson) which can reintroduce pre-fix values; instead reload
the container from disk and use that fresh instance. Replace the
container.loadData(JSONObject(container.containerJson)) and subsequent uses of
container.* for state with a freshContainer obtained by calling
ContainerUtils.getOrCreateContainer(context, appId) (or the appropriate
disk-load helper), call freshContainer.loadData(...) if needed, and use
freshContainer.graphicsDriver, freshContainer.graphicsDriverVersion,
freshContainer.audioDriver, freshContainer.dxWrapper,
freshContainer.dxWrapperConfig (pass through DXVKHelper.parseConfig), and
freshContainer.screenSize when updating xServerState and before launch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c71989b3-a83c-47bf-9432-ca515f521d1c
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.ktapp/src/main/java/app/gamenative/gamefixes/STEAM_752580.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/core/WineUtils.java
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt (2)
101-108: Clarify purpose of clearing extra data entries.Lines 103-104 set
extraDataentries (not the actualcontainer.audioDriver/container.winComponentsfields) to null. The naming overlap with the actual container fields is potentially confusing for future maintainers. Consider adding a brief comment explaining that theseputExtracalls target the extra data mechanism to signal launch-time refresh, not the actual audio/component configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt` around lines 101 - 108, Add a short clarifying comment above the container.putExtra(...) calls explaining these calls set entries in the extras map used to signal a launch-time refresh (not touching container.audioDriver or container.winComponents fields directly); reference the container.putExtra calls and the IMPERIVM_AUDIO_FIX_MIGRATION key so future maintainers understand this is an inter-process/launch flag mechanism, not the actual audio/component configuration mutation.
17-25: Minor: Consider preserving original line endings.
System.lineSeparator()returns\non Android, while Windows INI files typically use CRLF (\r\n). Most parsers tolerate mixed line endings, but for consistency you could detect and match the original file's line ending style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt` around lines 17 - 25, The updateIniValue function currently uses System.lineSeparator(), which can differ from the file's existing endings; detect the INI file's existing line-ending style (e.g. if content contains "\r\n" use "\r\n", else if it contains "\n" use "\n", otherwise fallback to System.lineSeparator()) and store it in a local lineEnding variable, then use that variable instead of System.lineSeparator() when computing suffix and when appending "$key=$value" so new lines match the original file; keep using the same Regex and updateIniValue as the insertion point.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt`:
- Around line 101-108: Add a short clarifying comment above the
container.putExtra(...) calls explaining these calls set entries in the extras
map used to signal a launch-time refresh (not touching container.audioDriver or
container.winComponents fields directly); reference the container.putExtra calls
and the IMPERIVM_AUDIO_FIX_MIGRATION key so future maintainers understand this
is an inter-process/launch flag mechanism, not the actual audio/component
configuration mutation.
- Around line 17-25: The updateIniValue function currently uses
System.lineSeparator(), which can differ from the file's existing endings;
detect the INI file's existing line-ending style (e.g. if content contains
"\r\n" use "\r\n", else if it contains "\n" use "\n", otherwise fallback to
System.lineSeparator()) and store it in a local lineEnding variable, then use
that variable instead of System.lineSeparator() when computing suffix and when
appending "$key=$value" so new lines match the original file; keep using the
same Regex and updateIniValue as the insertion point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1d98e1f-1051-48fc-80e8-28b93d1fe056
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.ktapp/src/main/java/app/gamenative/gamefixes/STEAM_752580.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/core/WineUtils.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.kt
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt (1)
76-99: Skip unnecessarySettings.inireads after migration is complete.Line 78 reads the file on every launch even when
imperivm_music_off_v2=1. Guarding before I/O avoids repeated disk work on hot launch paths.♻️ Proposed refactor
- if (settingsFile.isFile) { + if (settingsFile.isFile && container.getExtra(IMPERIVM_SETTINGS_MIGRATION) != "1") { runCatching { val original = settingsFile.readText(StandardCharsets.UTF_8) var updated = original var updatedMusic = false - if (container.getExtra(IMPERIVM_SETTINGS_MIGRATION) != "1") { - updated = updateIniValue(updated, "Music", "0") + updated = updateIniValue(updated, "Music", "0") - if (updated != original) { - settingsFile.writeText(updated, StandardCharsets.UTF_8) - updatedMusic = true - } + if (updated != original) { + settingsFile.writeText(updated, StandardCharsets.UTF_8) + updatedMusic = true + } - container.putExtra(IMPERIVM_SETTINGS_MIGRATION, "1") - changed = true - } + container.putExtra(IMPERIVM_SETTINGS_MIGRATION, "1") + changed = true if (updatedMusic) { Timber.tag("GameFixes").i("Disabled Imperivm music to reduce DirectMusic CPU cost") } }.onFailure { error -> Timber.tag("GameFixes").w(error, "Failed to update Imperivm Settings.ini") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt` around lines 76 - 99, Move the migration guard so we check container.getExtra(IMPERIVM_SETTINGS_MIGRATION) != "1" before doing any file I/O: only call settingsFile.readText(StandardCharsets.UTF_8) and run updateIniValue when the migration flag is not set; if the flag is set, skip the read/write entirely. Ensure you still set container.putExtra(IMPERIVM_SETTINGS_MIGRATION, "1") and changed = true only when you actually perform the update, and keep the existing logging path (updatedMusic -> Timber.tag("GameFixes").i(...)) and the onFailure Timber warning intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt`:
- Around line 76-99: Move the migration guard so we check
container.getExtra(IMPERIVM_SETTINGS_MIGRATION) != "1" before doing any file
I/O: only call settingsFile.readText(StandardCharsets.UTF_8) and run
updateIniValue when the migration flag is not set; if the flag is set, skip the
read/write entirely. Ensure you still set
container.putExtra(IMPERIVM_SETTINGS_MIGRATION, "1") and changed = true only
when you actually perform the update, and keep the existing logging path
(updatedMusic -> Timber.tag("GameFixes").i(...)) and the onFailure Timber
warning intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09d4fb63-566a-45a7-8ab5-8aee5f44d56b
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt
There was a problem hiding this comment.
Hi,
While we appreciate the effort you put into this, we have already existing system in place which changes configs to work for a game. (known config system). Also it kinda feels like you are doing fixes audio fixes here which are not related to Imperivm III. Maybe they could be a separate PR?
|
@Misazam - awaiting your input here. |
|
Hi, I re-checked this from the known-config side as well. At the time of testing, I did try the known config path in the app, but no known config was offered for this title. I also checked the current best-config path again and it returns From the codebase, the known config system can apply container-level fields such as That distinction mattered here. I tested the substance of what a known config could express first:
Those changes improved correctness in some areas, but they did not remove the heavy slowdown on their own. The bottleneck I validated on-device was specifically the music / MIDI / DirectMusic path, not audio output in general. The consistently working state in live testing was:
So I do not think an existing known config entry was skipped here. The practical fix that restored playability was game-specific: keep sound enabled, but disable only the in-game music path for Imperivm. That said, I agree with the architectural concern about scope. The |
Ok, so then I think it would be best to scope this PR to create a (reusable) gameFixRegistry class that can edit ini files and do only the ```
That way you can keep it scoped and we use the already in place components as much as possible. |
Misazam
force-pushed
the
fix/imperivm-audio-language
branch
from
f537a69 to
b0f27e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/gamefixes/IniFileFix.kt (1)
15-16: Consider using explicit"\n"for appended entries.
System.lineSeparator()returns"\n"on Android, while Windows INI files typically use"\r\n". This creates mixed line endings when appending new keys. While Wine and most Windows programs tolerate this, using a consistent"\n"(or detecting the file's existing line ending style) would be cleaner.♻️ Optional: use explicit newline
- val suffix = if (content.endsWith("\n") || content.isEmpty()) "" else System.lineSeparator() - content + suffix + "$key=$value" + System.lineSeparator() + val suffix = if (content.endsWith("\n") || content.isEmpty()) "" else "\n" + content + suffix + "$key=$value\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/gamefixes/IniFileFix.kt` around lines 15 - 16, The current append logic in IniFileFix.kt uses System.lineSeparator() when building the suffix and when terminating the new entry which can introduce mixed CRLF/LF endings; update the logic in the function that computes suffix (uses variables content and suffix) and the final concatenation (content + suffix + "$key=$value" + System.lineSeparator()) to use a consistent newline (e.g. "\n") or detect the existing file's line-ending style from content (check for "\r\n" presence) and reuse that style for both the suffix and the trailing newline so appended entries match the file's existing line endings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/gamefixes/IniFileFix.kt`:
- Around line 15-16: The current append logic in IniFileFix.kt uses
System.lineSeparator() when building the suffix and when terminating the new
entry which can introduce mixed CRLF/LF endings; update the logic in the
function that computes suffix (uses variables content and suffix) and the final
concatenation (content + suffix + "$key=$value" + System.lineSeparator()) to use
a consistent newline (e.g. "\n") or detect the existing file's line-ending style
from content (check for "\r\n" presence) and reuse that style for both the
suffix and the trailing newline so appended entries match the file's existing
line endings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 049064ce-9d20-4942-94c4-64157d1ce565
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.ktapp/src/main/java/app/gamenative/gamefixes/IniFileFix.ktapp/src/main/java/app/gamenative/gamefixes/STEAM_752580.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.kt
Misazam
changed the title
|
I’ve updated the PR to match the requested scope. It now only includes:
I’ve also updated the migration flow so the fix is marked complete when the file is already in the desired state, which avoids rereading Settings.ini on every launch. The PR description has been updated to reflect the current diff, and :app:compileDebugKotlin passes. |
|
Nice! I approve of the PR. |
|
Also just a quick note. Here we are adding tests for all previous game fix types. #1143 |
|
Done, I removed the migration marker logic so the IniFileFix always reapplies directly to Settings.ini, and I added a regression test for the legacy marker/import-export case. |
3 participants
Summary
This PR is now intentionally scoped to the smallest change requested in review for
Imperivm RTC - HD Edition / Great Battles of Rome HD(Steam App ID 752580).It adds a reusable INI game-fix helper and applies only the game-specific
Settings.inivalues needed for Imperivm.What this changes
IniFileFix/KeyedIniFileFixfor game fixes that need to edit.inifilesSTEAM_752580game fix inGameFixesRegistrySettings.inivalues for Imperivm:Music=0SoundFX=1Speech=1Settings.iniis already in the desired state, so it is not reread unnecessarily on every launchWhy
Testing on device showed the main performance problem came from the in-game music path rather than audio output in general.
Disabling only music while keeping effects and speech enabled was the practical fix that restored playability.
Scope
This PR no longer includes audio-driver, wincomponent,
XServerScreen, orWineUtilschanges.Those were removed to keep the PR limited to the reusable INI helper plus the Imperivm-specific
Settings.inifix requested in review.Tested on
Imperivm RTC - HD Edition / Great Battles of Rome HDSteam752580gbr.exeOnePlus OPD241516Validation
:app:compileDebugKotlinMusic=0SoundFX=1Speech=1Summary by CodeRabbit