Add turbo hotkey #1925
Add turbo hotkey #1925
Conversation
WalkthroughThe changes add a turbo mode toggle to the GUI. When the Tab key is pressed in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant GUI as GUI Module
participant EM as Emulator
U->>GUI: Press Tab key
GUI->>GUI: Toggle m_turboEnabled state
alt Turbo Enabled
GUI->>EM: Set scaler to 200
else Turbo Disabled
GUI->>EM: Set scaler to 100
end
GUI->>GUI: Update main menu ("Turbo" label if enabled)
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/gui/gui.cc (2)
1063-1066: Consider adding a safety mechanism for speed transitions.To address the hanging issue when returning to normal speed, it might be worth implementing a more gradual transition or adding safety checks during speed changes.
Consider implementing a smoother transition when disabling turbo mode:
if (ImGui::IsKeyPressed(ImGuiKey_Tab)) { m_turboEnabled = !m_turboEnabled; - g_emulator->settings.get<Emulator::SettingScaler>() = m_turboEnabled ? 200 : 100; + if (m_turboEnabled) { + // Immediately go to turbo speed + g_emulator->settings.get<Emulator::SettingScaler>() = 200; + } else { + // Schedule a safe return to normal speed + m_scheduleNormalSpeed = true; + m_normalSpeedTime = ImGui::GetTime() + 0.1f; // Small delay before transition + } } + + // Safe transition back to normal speed (add this after the Tab key check) + if (m_scheduleNormalSpeed && ImGui::GetTime() >= m_normalSpeedTime) { + g_emulator->settings.get<Emulator::SettingScaler>() = 100; + m_scheduleNormalSpeed = false; + }This would require adding two new member variables to track the scheduled speed change:
bool m_scheduleNormalSpeed = false;float m_normalSpeedTime = 0.0f;
1063-1066: Add documentation for the turbo mode feature.The code lacks any comments explaining the turbo functionality, its purpose, or the known limitations.
Add a comment explaining the turbo feature and its current limitations:
+ // Toggle turbo mode (2x speed) when Tab is pressed + // Note: There's a known issue where returning to normal speed can occasionally + // cause the emulator to hang. This appears to also occur when adjusting the + // speed scaler slider to the left, independent of this feature. if (ImGui::IsKeyPressed(ImGuiKey_Tab)) { m_turboEnabled = !m_turboEnabled; g_emulator->settings.get<Emulator::SettingScaler>() = m_turboEnabled ? 200 : 100; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui/gui.cc(2 hunks)src/gui/gui.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/gui/gui.cc (2)
src/main/main.cc (2)
g_emulator(103-107)g_emulator(109-113)src/core/gpu.h (2)
g_emulator(62-62)g_emulator(63-63)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: build
🔇 Additional comments (3)
src/gui/gui.h (1)
327-327: Clean addition of the turbo mode state variable.The new member variable is properly initialized to
falseand fits well with the existing code style.src/gui/gui.cc (2)
1512-1515: Good visual feedback for turbo mode.The addition of a "Turbo" indicator in the main menu bar provides clear feedback to the user when turbo mode is active.
1063-1066:❓ Verification inconclusive
Address potential emulator hanging issue with turbo mode.
The PR mentions a known issue where setting the speed scaler back to 100% can sometimes cause the emulator to hang. This suggests there might be a synchronization issue or timing problem when the speed changes.
To investigate this issue, it would be helpful to:
- Determine if the issue is related to the emulator's timing system
- Check if there's any interaction with frame counting or interrupts
🏁 Script executed:
#!/bin/bash # Look for timer or counter related code in the codebase rg -A 2 -B 2 "tick|counter|timer" --glob "*.cc" # Check for interrupt handling related to timing rg -A 3 -B 3 "interrupt.*scaler|scaler.*interrupt" --glob "*.cc"Length of output: 45780
Turbo Mode: Investigate Synchronization Issues with Scaler Reset
The turbo mode toggle in src/gui/gui.cc (lines 1063–1066) directly sets the speed scaler to 200 when enabled and back to 100 when disabled. However, given the known issue of potential emulator hangs when reverting to 100%, there is concern that this change may not be properly synchronized with the emulator’s timing and interrupt systems.
- Review Synchronization: Although our grep outputs on timing, counter, and interrupt code do not show an explicit link to scaler changes, it remains unclear if the absence of additional synchronization (e.g., a brief delay or barrier) could be causing missed handoffs or race conditions in the timing loop.
- Verify Integration Points: Please double-check that the scaler update is coordinated with the emulator’s timer/counter or interrupt handling systems. In particular, confirm whether the change from 200 to 100 might conflict with frame counting or the interrupt routine.
- Additional Logging/Testing: To better diagnose this intermittent hang issue, consider adding logging or a debug mode to capture timing discrepancies when toggling turbo mode. This will help verify whether additional synchronization measures are necessary.
| if (ImGui::IsKeyPressed(ImGuiKey_Tab)) { | ||
| m_turboEnabled = !m_turboEnabled; | ||
| g_emulator->settings.get<Emulator::SettingScaler>() = m_turboEnabled ? 200 : 100; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Turbo hotkey implementation needs improvement for maintainability.
While the implementation works, the turbo speed (200%) and the hotkey (Tab) are hardcoded. This would be better implemented as configurable settings, especially since the PR mentions this is tentative and there's a known issue where setting the speed back to 100% can occasionally cause the emulator to hang.
Consider refactoring this to use configurable settings for both the hotkey and speed value:
- if (ImGui::IsKeyPressed(ImGuiKey_Tab)) {
- m_turboEnabled = !m_turboEnabled;
- g_emulator->settings.get<Emulator::SettingScaler>() = m_turboEnabled ? 200 : 100;
+ // Check for turbo hotkey
+ if (ImGui::IsKeyPressed(g_emulator->settings.get<Emulator::SettingTurboHotkey>().value)) {
+ m_turboEnabled = !m_turboEnabled;
+ g_emulator->settings.get<Emulator::SettingScaler>() =
+ m_turboEnabled ? g_emulator->settings.get<Emulator::SettingTurboSpeed>().value : 100;
+ }This would require adding two new settings in the emulator settings class.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ImGui::IsKeyPressed(ImGuiKey_Tab)) { | |
| m_turboEnabled = !m_turboEnabled; | |
| g_emulator->settings.get<Emulator::SettingScaler>() = m_turboEnabled ? 200 : 100; | |
| } | |
| // Check for turbo hotkey | |
| if (ImGui::IsKeyPressed(g_emulator->settings.get<Emulator::SettingTurboHotkey>().value)) { | |
| m_turboEnabled = !m_turboEnabled; | |
| g_emulator->settings.get<Emulator::SettingScaler>() = | |
| m_turboEnabled ? g_emulator->settings.get<Emulator::SettingTurboSpeed>().value : 100; | |
| } |
|
So I need to rewrite the synchronization system between the CPU and the SPU, which is how the speed system currently works, and the reason messing with it too much in real time causes these hiccups. It's not exactly difficult to rewrite, really. Right now, the scale system changes this value, which can cause things to go desync when changing it while it's running: https://github.com/grumpycoders/pcsx-redux/blob/main/src/core/psxcounters.cc#L147 The right way to do it is instead to compress the audio stream here according to the scaling: https://github.com/grumpycoders/pcsx-redux/blob/main/src/spu/miniaudio.cc#L181-L220 As for the details of the implementations, it might be better to currently avoid changing the scaler value, and instead add a getter for it, multiplying it by the turbo factor when the key is pressed. This way, (1) the setting itself doesn't change, and (2) it'll be easier to move the code over to the right spot. |
This adds a simplistic turbo hotkey which toggles the speed scaler to 200% speed and back. This is a tentative implementation with one problem I haven't been able to fix.
The main issue is that setting the speed scaler back to 100% occasionally makes the emulator hang (which also happens when moving the speed scaler slider left, even without this patch). Probably the tick counter getting out of alignment? Is there a better way of implementing turbo mode I should consider? I'd really appreciate some guidance on this.
Aside from that, both the speed and hotkey are hardcoded for now.