Migration to SDL3.#2017
Conversation
Drops in SDL_AudioStream (pull callback model) as the SPU output backend, preserving the public API surface (feedStreamData, getCurrentFrames, waitForGoal, getBackends, getDevices, reinit) so the SPU and XA paths are unchanged. NullSync is preserved via a dedicated timer thread, since SDL3 only allows one audio driver loaded per process and we can't run a real device alongside a null one the way MiniAudio did. Backend and Device settings are read but the names won't round-trip from prior MiniAudio values (case differences, WinMM->wmme, SDL's device-name format). Settings silently reset to Default on first launch; users re-pick from SPU configuration as needed. Linux/macOS pull SDL3 from system packages (libsdl3-dev / brew); Windows builds it from the third_party/SDL submodule already wired in 2e945f6. The vsprojects side of this change is still pending. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Swaps the gamepad polling backend in core/pad.cc to SDL_Gamepad, inheriting SDL's gamecontrollerdb.txt mapping pipeline so controllers GLFW didn't recognize (PS5, recent Xbox revs, etc.) work out of the box. Hot-plug detection moves to draining SDL_EVENT_GAMEPAD_ADDED/ REMOVED on each poll, replacing GLFW's joystick callback. Settings defaults migrated from GLFW_GAMEPAD_BUTTON_* to SDL_GAMEPAD_BUTTON_*. Synthetic LEFT_TRIGGER/RIGHT_TRIGGER/INVALID sentinels move to PCSX_GAMEPAD_BUTTON_* values that don't collide with SDL's enum. The in-tree UI doesn't expose per-button rebinding, so the only users affected by the enum-format change are those who hand-edited their config JSON; their saved Controller_Pad* values will not round-trip and will need to be re-set to defaults. Keyboard input still flows through imgui_impl_glfw -> ImGui::IsKeyDown, so Keyboard_* settings continue to use GLFW_KEY_* constants. Keyboard moves with the window/ImGui-backend swap in Phase 3. Rumble plumbing deferred per directive. SDL event polling for gamepad hotplug currently lives inline in Pad::getButtons(); will move to the central event pump when Phase 3 takes ownership of the event loop. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
keyboard input, and high-DPI plumbing. Window/GL: SDL_CreateWindow + SDL_GL_CreateContext replace GLFW's combined glfwCreateWindow. The 3.2-core / 3.0-fallback retry now covers context creation since SDL splits window and context, and destroys+recreates both on first failure rather than just the window. SDL_WINDOW_HIGH_PIXEL_DENSITY makes the window/pixel split meaningful on macOS Retina and Windows per-monitor DPI. Event loop: GLFW callback installation (install_callbacks=true on the imgui_impl_glfw side) replaced with an explicit SDL_PollEvent loop in startFrame() that hands every event to ImGui_ImplSDL3_ProcessEvent first, then dispatches close/resize/drop/key locally. Fixes the recurring high-DPI bug: GLFW didn't fire when crossing monitors with different DPI on Windows; SDL3 fires SDL_EVENT_WINDOW_DISPLAY_CHANGED, which we now route to changeScale + a font rebuild. ImGui backend: imgui_impl_glfw -> imgui_impl_sdl3, viewport callback hooks (Platform_CreateWindow / OnChangedViewport / DestroyWindow) preserved; the per-window key callback that the GLFW backend needed is gone since the SDL3 backend dispatches via ProcessEvent. Input mode: SDL_SetWindowRelativeMouseMode collapses GLFW's two-step (CURSOR_DISABLED + RAW_MOUSE_MOTION) into one call. SDL3 fullscreen with no mode set produces borderless desktop, matching GLFW's glfwSetWindowMonitor(..., GLFW_DONT_CARE). Keyboard: pad bindings now store SDL_Scancode values; SdlScancodeToImGuiKey replaces GlfwKeyToImGuiKey, sdlScancodeToString replaces glfwKeyToString using SDL_GetScancodeName for the layout-aware label. The keyboard event signal carries SDL_Scancode in Events::Keyboard.scancode and the rebinding UI reads .scancode (not .key) so writes and reads agree on the integer's meaning. NanoVG: cleanup ordering corrected so sub-contexts and the main context are deleted while their owning GL context is still live; the GLFW-era code freed them after glfwDestroyWindow, technically leaking GPU resources. Migration impact - keyboard bindings: settings serialize every field including defaults, so all existing pcsx.json files carry seventeen Keyboard_* keys with GLFW_KEY_* integers saved. After this change those values get interpreted as SDL_Scancode integers, which produces nonsense bindings on first launch with the new build. Per directive, no in-place migration is provided; users will need to either delete the pad config block from their pcsx.json or rebind in the SPU/Pad configuration UI once. This is a one-time cost on the upgrade path. Stale GLFW includes removed from src/gui/widgets/assembly.cc and src/gui/widgets/zep.cc (no API calls were ever made through them, just stray includes). Makefile: glfw3 dropped from PACKAGES, imgui_impl_glfw.cpp swapped for imgui_impl_sdl3.cpp in SRCS. Phase 4 (cleaning up the unused miniaudio submodule and any final references) lands separately. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
This is the largest ImGui breaking-change set since 2015. The bump is motivated by a multi-viewport regression that surfaced in Phase 3: the main window stayed underneath all other viewports when focused. v1.92 includes fixes for several multi-viewport ordering issues and also reworks the font system in ways our pre-1.92 dynamic-DPI scaffold no longer needs. ImGui v1.91.5-docking -> v1.92.7-docking. Project-side adjustments: - forced-includes/imgui.h: define IM_ASSERT_USER_ERROR_RET and IM_ASSERT_USER_ERROR_RETV alongside the existing IM_ASSERT_USER_ERROR override. v1.92 added the new variants under the same #ifndef guard, so once we redirect the base macro we have to provide all three. - gui.cc / gui.h: PushFont takes a required size argument in v1.92. useMainFont/useMonoFont now pass the active font's LegacySize so the per-scale font map continues to drive rendering size, matching pre-1.92 PushFont(font) semantics. Dropped ImGui_ImplOpenGL3_Create /DestroyFontsTexture and ImFontAtlas::Build() which v1.92 retired in favor of the dynamic-texture protocol. The reload pass now also sets Style.FontSizeBase to the active main font's LegacySize so default-context frames render at the right size on DPI flip. changeScale refreshes io.FontDefault and Style.FontSizeBase on every scale change, not only the first one. - ram-viewer: ImFont::FindGlyph moved onto a per-size ImFontBaked, queried via font->GetFontBaked(GetFontSize()). Pre-fetch hex-glyph UVs and the font atlas TexRef in drawRAM() before ImGui::Render() rather than inside the AddCallback render closure, so the custom shader doesn't sample with stale UVs after a same-frame atlas grow. ImFont::ContainerAtlas was removed; we go through ImGui::GetIO() ->Fonts->TexRef.GetTexID() instead. - vram-viewer, ram-viewer: IM_OFFSETOF -> offsetof. - shader-editor: ImDrawCmd::TextureId -> TexRef.GetTexID(). Third-party touch-ups: - ImFileDialog: ImFont::FontSize -> LegacySize at the loaded-size call sites; ImGuiContext::FontSize (a different field, still extant) left alone. - imgui_lua_bindings: extend the perl generator's argument-type regex to accept ImTextureRef (constructs implicitly from ImTextureID, so the existing IM_TEXTURE_ID_ARG path still works at the call site). Regenerated imgui_iterator.inl from imgui.h via the project's documented procedure; restores Image / ImageWithBg / ImageButton bindings the prior generator dropped on v1.92's signatures. - third_party/imgui_md (forked to grumpycoders/imgui_md): TexID -> TexRef.GetTexID() on the font-atlas demo image; .gitmodules URL updated. - third_party/zep (already a grumpycoders fork): m_pFont->FontSize -> LegacySize. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR migrates windowing/input from GLFW to SDL3, replaces the MiniAudio backend with an SDL-based audio implementation, updates ImGui integration for v1.92, and adjusts build/project files and widget rendering to match the new APIs. ChangesSDL3 and Audio Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/pad.cc (3)
924-933:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't save scancodes that the runtime path can't resolve.
keyboardEvent()now stores anyevent.scancode, butgetKeyboardButtons()silently ignores bindings whoseSdlScancodeToImGuiKey()result isImGuiKey_None. That lets users save dead bindings for keys outside the manual map.💡 Minimal guard
void PadsImpl::Pad::keyboardEvent(const PCSX::Events::Keyboard& event) { if (m_buttonToWait == -1) { return; } + if (SdlScancodeToImGuiKey(event.scancode) == ImGuiKey_None) { + return; + } // Bindings are stored as SDL_Scancode values to match the runtime lookup // path (SdlScancodeToImGuiKey) and the SDL_SCANCODE_* setting defaults. // event.key is an SDL_Keycode (layout-aware codepoint); event.scancode is🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pad.cc` around lines 924 - 933, keyboardEvent currently writes event.scancode into the binding unconditionally, which allows saving scancodes that SdlScancodeToImGuiKey can't map (ImGuiKey_None). Before assigning to getButtonFromGUIIndex(m_buttonToWait) in PadsImpl::Pad::keyboardEvent, call SdlScancodeToImGuiKey(event.scancode) and only store the scancode if the result != ImGuiKey_None; otherwise clear m_buttonToWait without saving (or ignore the input) so dead bindings are not persisted. Ensure you reference keyboardEvent, getButtonFromGUIIndex, SdlScancodeToImGuiKey, ImGuiKey_None, and m_buttonToWait when implementing the guard.
1031-1094: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFactor the mapping UI out of
configure().
configure()is the third hotspot listed in the failing CodeScene gate. The repeated mapping-row rendering and the gamepad combo builder are good candidates for small helpers or a descriptor-driven loop so this stops blocking the PR on complexity alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pad.cc` around lines 1031 - 1094, The configure() function is too large; extract the repeated mapping-row code and the gamepad combo building into small helpers to reduce complexity. Create a member/helper like renderMappingRow(int guiIndex, int absIndex) that encapsulates the ImGui row creation (uses m_buttonToWait, getButtonFromGUIIndex, sdlScancodeToString and Push/PopStyleColor logic) and call it from configure() for both the button rows and D-pad rows, and create a helper like buildAndRenderGamepadCombo(int& id, const char*& preview) or renderGamepadCombo() that builds the gamepads vector (reuse Entry, SDL_GetGamepadName, s_pads->m_gamepads) and performs the ImGui::BeginCombo/Selectable logic while setting changed and id; keep all existing state uses (m_buttonToWait, m_settings.get<SettingControllerID>().value, selected) and ensure behavior and preview selection remain identical.
500-621: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit
getButtons()before the hotspot gate keeps blocking the PR.This function is also called out by the current CodeScene failure. Extracting the SDL hotplug drain, gamepad snapshot, and digital/analog post-processing into small helpers should get the complexity back under control without changing behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pad.cc` around lines 500 - 621, The getButtons() routine is too large and tripping the CodeScene hotspot check; extract the SDL hotplug/drain and gamepad snapshot logic (the SDL_PumpEvents/SDL_PeepEvents block and the m_gamepad capture/SDL_GetGamepad* reads) into a helper like drainHotplugEvents() / sampleGamepadState(GamepadState&), and extract the Digital/Analog post-processing blocks (the PadType::Digital dpad-angle math and the PadType::Analog axisToUint8 mapping and assignments) into helpers like processDigitalDpad(bool buttons[16], const GamepadState&) and processAnalogAxes(Pad& pad, const GamepadState&). Leave getKeyboardButtons() as-is, and modify getButtons() to call these new helpers in sequence (drainHotplugEvents(), sampleGamepadState(), then either processDigitalDpad() or processAnalogAxes()) so behavior is unchanged but complexity is reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/pad.cc`:
- Around line 222-329: The SdlScancodeToImGuiKey function has excessive
cyclomatic complexity due to the large switch; replace it with a data-driven
lookup (e.g., a constexpr std::array/std::pair table or two small grouped helper
maps) that maps SDL scancodes to ImGuiKey values, and have SdlScancodeToImGuiKey
perform a O(1) lookup (with a default ImGuiKey_None fallback). Locate the
mapping inside the src/core/pad.cc file (symbols: SdlScancodeToImGuiKey) and
refactor the switch into a compile-time table (or grouped tables for contiguous
ranges like letters, numbers, keypad, function keys) to preserve current
behavior and return values, then update the function to consult that table and
return ImGuiKey_None when not found.
- Around line 71-74: scanGamepads() currently rebuilds m_gamepads[] densely
which shifts indices and breaks persisted SettingControllerID; instead store a
stable device identity (e.g. device instance GUID/ID from the SDL handle) in
settings and change scanGamepads() to populate m_gamepads by matching scanned
devices to those stored identities (leave slots empty if a saved device isn't
present, and place newly seen devices into the first free slot) so indices
remain stable; update map() to resolve the current SDL_Gamepad* handle for a
given SettingControllerID by looking up the saved device identity and finding
the matching live device handle during rescans.
In `@src/gui/gui.cc`:
- Around line 210-224: PCSX::GUI::setFullscreen currently always restores
position/size when leaving fullscreen but ignores whether the window was
maximized; update the else branch to restore maximized state by checking
m_windowMaximized and calling SDL_MaximizeWindow(m_window) when true (otherwise
leave as-is or call SDL_RestoreWindow if you need to ensure unmaximized state).
Modify PCSX::GUI::setFullscreen to perform this check after
SDL_SetWindowSize/Position so the window returns to its prior maximized state
using the existing m_windowMaximized flag.
- Around line 956-967: On window move/resize/maximize/restore events refresh the
cached windowed geometry (m_windowPosX, m_windowPosY, m_windowSizeX,
m_windowSizeY, m_windowMaximized) so saveCfg() later writes the current geometry
instead of stale values; inside the SDL event handling cases that affect
geometry (e.g., SDL_EVENT_WINDOW_MOVED, SDL_EVENT_WINDOW_RESIZED,
SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED, SDL_EVENT_WINDOW_MAXIMIZED,
SDL_EVENT_WINDOW_RESTORED, and similar where event.window.windowID ==
SDL_GetWindowID(m_window)) call SDL_GetWindowPosition(m_window, &x, &y),
SDL_GetWindowSize(m_window, &w, &h) and update m_windowPosX/Y = x/y,
m_windowSizeX/Y = w/h and set m_windowMaximized = (SDL_GetWindowFlags(m_window)
& SDL_WINDOW_MAXIMIZED) != 0; avoid overwriting when the window is currently
minimized or fullscreen if that behavior must be preserved.
- Around line 969-973: The SDL drop-file handler must explicitly free SDL's
dynamically-allocated drop payload rather than assuming it is freed on event
consumption: in the SDL_EVENT_DROP_FILE case, keep the existing null check and
call magicOpen(event.drop.data) when non-null, then always call
SDL_CleanupEvent(&event) afterwards to release event.drop.data; ensure
SDL_CleanupEvent is invoked whether or not magicOpen succeeds so there is no
per-drop leak (refer to the SDL_EVENT_DROP_FILE case, event.drop.data,
magicOpen, and SDL_CleanupEvent).
In `@src/gui/widgets/ram-viewer.cc`:
- Around line 392-423: The drawRAM() function has grown past the complexity
threshold due to the glyph prefetch block; extract that block into a new private
helper method (e.g., PCSX::Widgets::RAMViewer::refreshGlyphCache()) that
performs the ImGui font lookup, fills m_glyphUVs, sets m_glyphAspect and
m_fontTexID, and then call refreshGlyphCache() from drawRAM() in the same
location the block was removed from; ensure the helper uses ImGui::GetFont(),
ImGui::GetFontSize(), ImFontBaked::FindGlyph(), and updates the same member
variables (m_glyphUVs, m_glyphAspect, m_fontTexID) so behavior is unchanged.
In `@src/gui/widgets/shader-editor.cc`:
- Around line 755-759: The current cast at the texture extraction (GLuint
textureID = static_cast<GLuint>(cmd->TexRef.GetTexID())) can break on platforms
where ImTextureID is a pointer-sized type; change the conversion to use the
intptr_t bridge used elsewhere by casting via intptr_t before GLuint (i.e. use
(GLuint)(intptr_t)cmd->TexRef.GetTexID()); update the assignment at the
textureID creation in the shader-editor render loop (the line using
cmd->TexRef.GetTexID()) to match the ram-viewer pattern to ensure correct
cross-backend behavior.
In `@src/spu/sdlaudio.cc`:
- Around line 148-164: Currently failures from SDL_SetAudioStreamGetCallback or
SDL_BindAudioStream abort startup by throwing; instead follow the same
degraded-safe fallback used earlier: on failure of SDL_SetAudioStreamGetCallback
or SDL_BindAudioStream capture SDL_GetError(), clean up
(SDL_DestroyAudioStream(m_stream); m_stream=nullptr;
SDL_CloseAudioDevice(m_device); m_device=0), then switch to the dummy backend by
invoking init(true) or the existing degraded initialization path rather than
throwing; ensure you reference trampoline, m_stream and m_device when locating
the code so the error path mirrors the earlier init(true) retry behavior.
- Around line 43-49: The resume-recovery path in the
m_listener.listen<Events::ExecutionFlow::Run> handler uses uninit() and
init(true) when SDL_ResumeAudioDevice(m_device) fails but does not resume the
new device, so the replacement backend stays paused; change the recovery to call
SDL_ResumeAudioDevice(m_device) after init(true) (or call init(false) if init
has a parameter to leave device running) and only startNullThread() when
m_settings.get<NullSync>() is true, ensuring that when NullSync is off the newly
initialized device is resumed; apply the identical fix to the other recovery
block referenced (the one around lines 187-195) so both failure paths resume the
replacement device.
- Around line 59-169: The init() method is too large and failing CodeScene;
split its logic into clear helpers: extract the backend selection block (the
loop over m_backends, SDL_SetHint calls and m_settings.get<Backend>() handling)
into a helper like selectAudioBackend(), extract the device enumeration and open
logic (SDL_GetAudioPlaybackDevices, filling m_devices, chosen/deviceFound,
m_settings.get<Device>(), SDL_OpenAudioDevice) into enumerateAndOpenDevice(),
and extract stream creation and recovery (SDL_CreateAudioStream,
SDL_SetAudioStreamGetCallback, SDL_BindAudioStream and their error cleanup
paths) into setupStreamAndBind(); have init() call these helpers in order and
preserve existing error handling and state mutations (m_audioInitialized,
m_device, m_stream), referencing symbols PCSX::SPU::SDLAudio::init, m_backends,
m_settings.get<Backend>(), m_settings.get<Device>(), SDL_OpenAudioDevice,
SDL_CreateAudioStream, SDL_SetAudioStreamGetCallback, and SDL_BindAudioStream so
the behavior remains identical but with smaller, testable functions.
- Around line 254-276: In PCSX::SPU::SDLAudio::advanceFrames the code uses auto
total = m_frames.fetch_add(frameCount) which returns the pre-increment value, so
the goal comparison uses the old total and can miss an exact hit; change the
comparison to use the post-increment total (compute newTotal = total +
frameCount or call fetch_add and then add frameCount) when comparing against
m_goalpost and deciding to notify (affecting the branches that update
m_previousGoalpost, m_triggered and call
m_triggered.notify_one()/m_cv.notify_one()) so the wake happens immediately when
the increment reaches the goal.
- Around line 223-239: The mixed float samples from m_mixBuffers (loop over
STREAMS) can exceed [-1.0f,1.0f] and must be clamped before writing to
m_outputBuffer to avoid distortion; modify the mixing block in sdlaudio.cc (the
loop that computes l and r and writes to m_outputBuffer) to clamp lr/l/r to the
range -1.0f..1.0f (e.g. with std::clamp or a simple min/max) right after
computing the summed float(s) and before assigning into m_outputBuffer[f * 2 +
0/1].
In `@third_party/imgui_lua_bindings/imgui_iterator.inl`:
- Around line 1799-1800: The iterator marks ImGuiItemFlags as unsupported
because the bindings generator doesn't recognize that type; update
third_party/imgui_lua_bindings/generate_imgui_bindings.pl to treat
ImGuiItemFlags as a supported "flags" type: add "ImGuiItemFlags" to the
return-type handling so IMGUI_API ImGuiItemFlags GetItemFlags() is emitted (not
"Unsupported return type"), and add "ImGuiItemFlags" to the argument-type
whitelist/parsing used for function signatures so IMGUI_API void
PushItemFlag(ImGuiItemFlags option, bool enabled) is generated (not "Unsupported
arg type"); ensure the generator emits the corresponding Lua push/convert logic
for flags consistent with other flag types already supported in the script.
In `@third_party/SDL_build_config.h`:
- Line 32: The include in third_party/SDL_build_config.h references
<SDL3/SDL_platform_defines.h> which is missing; either restore the
SDL3/SDL_platform_defines.h file under third_party/SDL/include/SDL3 or change
the include to point to the actual header that exists in the repo (e.g., the
non-SDL3 path used by the vendor copy), and update the VS project include
settings in vsprojects/SDL/SDL.vcxproj to add third_party/SDL/include (in
addition to third_party/SDL/src) so the compiler can find the vendored headers;
modify third_party/SDL_build_config.h and vsprojects/SDL/SDL.vcxproj
accordingly.
---
Outside diff comments:
In `@src/core/pad.cc`:
- Around line 924-933: keyboardEvent currently writes event.scancode into the
binding unconditionally, which allows saving scancodes that
SdlScancodeToImGuiKey can't map (ImGuiKey_None). Before assigning to
getButtonFromGUIIndex(m_buttonToWait) in PadsImpl::Pad::keyboardEvent, call
SdlScancodeToImGuiKey(event.scancode) and only store the scancode if the result
!= ImGuiKey_None; otherwise clear m_buttonToWait without saving (or ignore the
input) so dead bindings are not persisted. Ensure you reference keyboardEvent,
getButtonFromGUIIndex, SdlScancodeToImGuiKey, ImGuiKey_None, and m_buttonToWait
when implementing the guard.
- Around line 1031-1094: The configure() function is too large; extract the
repeated mapping-row code and the gamepad combo building into small helpers to
reduce complexity. Create a member/helper like renderMappingRow(int guiIndex,
int absIndex) that encapsulates the ImGui row creation (uses m_buttonToWait,
getButtonFromGUIIndex, sdlScancodeToString and Push/PopStyleColor logic) and
call it from configure() for both the button rows and D-pad rows, and create a
helper like buildAndRenderGamepadCombo(int& id, const char*& preview) or
renderGamepadCombo() that builds the gamepads vector (reuse Entry,
SDL_GetGamepadName, s_pads->m_gamepads) and performs the
ImGui::BeginCombo/Selectable logic while setting changed and id; keep all
existing state uses (m_buttonToWait,
m_settings.get<SettingControllerID>().value, selected) and ensure behavior and
preview selection remain identical.
- Around line 500-621: The getButtons() routine is too large and tripping the
CodeScene hotspot check; extract the SDL hotplug/drain and gamepad snapshot
logic (the SDL_PumpEvents/SDL_PeepEvents block and the m_gamepad
capture/SDL_GetGamepad* reads) into a helper like drainHotplugEvents() /
sampleGamepadState(GamepadState&), and extract the Digital/Analog
post-processing blocks (the PadType::Digital dpad-angle math and the
PadType::Analog axisToUint8 mapping and assignments) into helpers like
processDigitalDpad(bool buttons[16], const GamepadState&) and
processAnalogAxes(Pad& pad, const GamepadState&). Leave getKeyboardButtons()
as-is, and modify getButtons() to call these new helpers in sequence
(drainHotplugEvents(), sampleGamepadState(), then either processDigitalDpad() or
processAnalogAxes()) so behavior is unchanged but complexity is reduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b68c645-4c3d-4705-ae06-4a4e76a6c644
📒 Files selected for processing (42)
.gitmodulesMakefilesrc/core/pad.ccsrc/forced-includes/imgui.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/assembly.ccsrc/gui/widgets/ram-viewer.ccsrc/gui/widgets/ram-viewer.hsrc/gui/widgets/shader-editor.ccsrc/gui/widgets/vram-viewer.ccsrc/gui/widgets/zep.ccsrc/spu/interface.hsrc/spu/miniaudio.ccsrc/spu/sdlaudio.ccsrc/spu/sdlaudio.hsrc/spu/spu.ccsrc/spu/xa.ccthird_party/ImFileDialog/ImFileDialog.cppthird_party/SDLthird_party/SDL_build_config.hthird_party/imguithird_party/imgui_lua_bindings/generate_imgui_bindings.plthird_party/imgui_lua_bindings/imgui_iterator.inlthird_party/imgui_mdthird_party/zepvsprojects/SDL/SDL.vcxprojvsprojects/SDL/SDL.vcxproj.filtersvsprojects/SPU/SPU.vcxprojvsprojects/SPU/SPU.vcxproj.filtersvsprojects/core/core.vcxprojvsprojects/core/packages.configvsprojects/gui/gui.vcxprojvsprojects/gui/packages.configvsprojects/imgui-glfw-ogl3/packages.configvsprojects/imgui-sdl3-ogl3/imgui-sdl3-ogl3.vcxprojvsprojects/imgui-sdl3-ogl3/imgui-sdl3-ogl3.vcxproj.filtersvsprojects/mainthunk/mainthunk.vcxprojvsprojects/mainthunk/packages.configvsprojects/pcsx-redux.slnvsprojects/tests/pcsxrunner/packages.configvsprojects/tests/pcsxrunner/pcsxrunner.vcxproj
💤 Files with no reviewable changes (10)
- src/gui/widgets/assembly.cc
- vsprojects/gui/packages.config
- vsprojects/imgui-glfw-ogl3/packages.config
- src/gui/widgets/zep.cc
- vsprojects/core/packages.config
- src/spu/miniaudio.cc
- vsprojects/tests/pcsxrunner/packages.config
- vsprojects/mainthunk/packages.config
- vsprojects/core/core.vcxproj
- vsprojects/gui/gui.vcxproj
| // Open SDL gamepad handles, one per slot we've discovered. nullptr means the | ||
| // slot is unused. The slot index doubles as the user-visible "controller ID" | ||
| // saved in SettingControllerID. | ||
| SDL_Gamepad* m_gamepads[16] = {nullptr}; |
There was a problem hiding this comment.
Persisting the dense scan index will reshuffle pads after hotplug.
SettingControllerID now represents the m_gamepads[] index, but scanGamepads() rebuilds that array from zero on every rescan. If pad 0 disappears, every higher pad shifts left on the next scan, so saved assignments can silently jump to a different physical controller. This needs a stable device identity for persistence, with the current handle resolved from that identity during map().
Also applies to: 380-397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/pad.cc` around lines 71 - 74, scanGamepads() currently rebuilds
m_gamepads[] densely which shifts indices and breaks persisted
SettingControllerID; instead store a stable device identity (e.g. device
instance GUID/ID from the SDL handle) in settings and change scanGamepads() to
populate m_gamepads by matching scanned devices to those stored identities
(leave slots empty if a saved device isn't present, and place newly seen devices
into the first free slot) so indices remain stable; update map() to resolve the
current SDL_Gamepad* handle for a given SettingControllerID by looking up the
saved device identity and finding the matching live device handle during
rescans.
| static ImGuiKey SdlScancodeToImGuiKey(int scancode) { | ||
| switch (scancode) { | ||
| case SDL_SCANCODE_TAB: return ImGuiKey_Tab; | ||
| case SDL_SCANCODE_LEFT: return ImGuiKey_LeftArrow; | ||
| case SDL_SCANCODE_RIGHT: return ImGuiKey_RightArrow; | ||
| case SDL_SCANCODE_UP: return ImGuiKey_UpArrow; | ||
| case SDL_SCANCODE_DOWN: return ImGuiKey_DownArrow; | ||
| case SDL_SCANCODE_PAGEUP: return ImGuiKey_PageUp; | ||
| case SDL_SCANCODE_PAGEDOWN: return ImGuiKey_PageDown; | ||
| case SDL_SCANCODE_HOME: return ImGuiKey_Home; | ||
| case SDL_SCANCODE_END: return ImGuiKey_End; | ||
| case SDL_SCANCODE_INSERT: return ImGuiKey_Insert; | ||
| case SDL_SCANCODE_DELETE: return ImGuiKey_Delete; | ||
| case SDL_SCANCODE_BACKSPACE: return ImGuiKey_Backspace; | ||
| case SDL_SCANCODE_SPACE: return ImGuiKey_Space; | ||
| case SDL_SCANCODE_RETURN: return ImGuiKey_Enter; | ||
| case SDL_SCANCODE_ESCAPE: return ImGuiKey_Escape; | ||
| case SDL_SCANCODE_APOSTROPHE: return ImGuiKey_Apostrophe; | ||
| case SDL_SCANCODE_COMMA: return ImGuiKey_Comma; | ||
| case SDL_SCANCODE_MINUS: return ImGuiKey_Minus; | ||
| case SDL_SCANCODE_PERIOD: return ImGuiKey_Period; | ||
| case SDL_SCANCODE_SLASH: return ImGuiKey_Slash; | ||
| case SDL_SCANCODE_SEMICOLON: return ImGuiKey_Semicolon; | ||
| case SDL_SCANCODE_EQUALS: return ImGuiKey_Equal; | ||
| case SDL_SCANCODE_LEFTBRACKET: return ImGuiKey_LeftBracket; | ||
| case SDL_SCANCODE_BACKSLASH: return ImGuiKey_Backslash; | ||
| case SDL_SCANCODE_RIGHTBRACKET: return ImGuiKey_RightBracket; | ||
| case SDL_SCANCODE_GRAVE: return ImGuiKey_GraveAccent; | ||
| case SDL_SCANCODE_CAPSLOCK: return ImGuiKey_CapsLock; | ||
| case SDL_SCANCODE_SCROLLLOCK: return ImGuiKey_ScrollLock; | ||
| case SDL_SCANCODE_NUMLOCKCLEAR: return ImGuiKey_NumLock; | ||
| case SDL_SCANCODE_PRINTSCREEN: return ImGuiKey_PrintScreen; | ||
| case SDL_SCANCODE_PAUSE: return ImGuiKey_Pause; | ||
| case SDL_SCANCODE_KP_0: return ImGuiKey_Keypad0; | ||
| case SDL_SCANCODE_KP_1: return ImGuiKey_Keypad1; | ||
| case SDL_SCANCODE_KP_2: return ImGuiKey_Keypad2; | ||
| case SDL_SCANCODE_KP_3: return ImGuiKey_Keypad3; | ||
| case SDL_SCANCODE_KP_4: return ImGuiKey_Keypad4; | ||
| case SDL_SCANCODE_KP_5: return ImGuiKey_Keypad5; | ||
| case SDL_SCANCODE_KP_6: return ImGuiKey_Keypad6; | ||
| case SDL_SCANCODE_KP_7: return ImGuiKey_Keypad7; | ||
| case SDL_SCANCODE_KP_8: return ImGuiKey_Keypad8; | ||
| case SDL_SCANCODE_KP_9: return ImGuiKey_Keypad9; | ||
| case SDL_SCANCODE_KP_PERIOD: return ImGuiKey_KeypadDecimal; | ||
| case SDL_SCANCODE_KP_DIVIDE: return ImGuiKey_KeypadDivide; | ||
| case SDL_SCANCODE_KP_MULTIPLY: return ImGuiKey_KeypadMultiply; | ||
| case SDL_SCANCODE_KP_MINUS: return ImGuiKey_KeypadSubtract; | ||
| case SDL_SCANCODE_KP_PLUS: return ImGuiKey_KeypadAdd; | ||
| case SDL_SCANCODE_KP_ENTER: return ImGuiKey_KeypadEnter; | ||
| case SDL_SCANCODE_KP_EQUALS: return ImGuiKey_KeypadEqual; | ||
| case SDL_SCANCODE_LSHIFT: return ImGuiKey_LeftShift; | ||
| case SDL_SCANCODE_LCTRL: return ImGuiKey_LeftCtrl; | ||
| case SDL_SCANCODE_LALT: return ImGuiKey_LeftAlt; | ||
| case SDL_SCANCODE_LGUI: return ImGuiKey_LeftSuper; | ||
| case SDL_SCANCODE_RSHIFT: return ImGuiKey_RightShift; | ||
| case SDL_SCANCODE_RCTRL: return ImGuiKey_RightCtrl; | ||
| case SDL_SCANCODE_RALT: return ImGuiKey_RightAlt; | ||
| case SDL_SCANCODE_RGUI: return ImGuiKey_RightSuper; | ||
| case SDL_SCANCODE_MENU: return ImGuiKey_Menu; | ||
| case SDL_SCANCODE_0: return ImGuiKey_0; | ||
| case SDL_SCANCODE_1: return ImGuiKey_1; | ||
| case SDL_SCANCODE_2: return ImGuiKey_2; | ||
| case SDL_SCANCODE_3: return ImGuiKey_3; | ||
| case SDL_SCANCODE_4: return ImGuiKey_4; | ||
| case SDL_SCANCODE_5: return ImGuiKey_5; | ||
| case SDL_SCANCODE_6: return ImGuiKey_6; | ||
| case SDL_SCANCODE_7: return ImGuiKey_7; | ||
| case SDL_SCANCODE_8: return ImGuiKey_8; | ||
| case SDL_SCANCODE_9: return ImGuiKey_9; | ||
| case SDL_SCANCODE_A: return ImGuiKey_A; | ||
| case SDL_SCANCODE_B: return ImGuiKey_B; | ||
| case SDL_SCANCODE_C: return ImGuiKey_C; | ||
| case SDL_SCANCODE_D: return ImGuiKey_D; | ||
| case SDL_SCANCODE_E: return ImGuiKey_E; | ||
| case SDL_SCANCODE_F: return ImGuiKey_F; | ||
| case SDL_SCANCODE_G: return ImGuiKey_G; | ||
| case SDL_SCANCODE_H: return ImGuiKey_H; | ||
| case SDL_SCANCODE_I: return ImGuiKey_I; | ||
| case SDL_SCANCODE_J: return ImGuiKey_J; | ||
| case SDL_SCANCODE_K: return ImGuiKey_K; | ||
| case SDL_SCANCODE_L: return ImGuiKey_L; | ||
| case SDL_SCANCODE_M: return ImGuiKey_M; | ||
| case SDL_SCANCODE_N: return ImGuiKey_N; | ||
| case SDL_SCANCODE_O: return ImGuiKey_O; | ||
| case SDL_SCANCODE_P: return ImGuiKey_P; | ||
| case SDL_SCANCODE_Q: return ImGuiKey_Q; | ||
| case SDL_SCANCODE_R: return ImGuiKey_R; | ||
| case SDL_SCANCODE_S: return ImGuiKey_S; | ||
| case SDL_SCANCODE_T: return ImGuiKey_T; | ||
| case SDL_SCANCODE_U: return ImGuiKey_U; | ||
| case SDL_SCANCODE_V: return ImGuiKey_V; | ||
| case SDL_SCANCODE_W: return ImGuiKey_W; | ||
| case SDL_SCANCODE_X: return ImGuiKey_X; | ||
| case SDL_SCANCODE_Y: return ImGuiKey_Y; | ||
| case SDL_SCANCODE_Z: return ImGuiKey_Z; | ||
| case SDL_SCANCODE_F1: return ImGuiKey_F1; | ||
| case SDL_SCANCODE_F2: return ImGuiKey_F2; | ||
| case SDL_SCANCODE_F3: return ImGuiKey_F3; | ||
| case SDL_SCANCODE_F4: return ImGuiKey_F4; | ||
| case SDL_SCANCODE_F5: return ImGuiKey_F5; | ||
| case SDL_SCANCODE_F6: return ImGuiKey_F6; | ||
| case SDL_SCANCODE_F7: return ImGuiKey_F7; | ||
| case SDL_SCANCODE_F8: return ImGuiKey_F8; | ||
| case SDL_SCANCODE_F9: return ImGuiKey_F9; | ||
| case SDL_SCANCODE_F10: return ImGuiKey_F10; | ||
| case SDL_SCANCODE_F11: return ImGuiKey_F11; | ||
| case SDL_SCANCODE_F12: return ImGuiKey_F12; | ||
| default: return ImGuiKey_None; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Break this mapping into data instead of a 100+ branch switch.
This helper is one of the functions named in the current CodeScene quality-gate failure. A constexpr lookup table or a couple of grouped helpers would preserve behavior while dropping the cyclomatic complexity back under the hotspot threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/pad.cc` around lines 222 - 329, The SdlScancodeToImGuiKey function
has excessive cyclomatic complexity due to the large switch; replace it with a
data-driven lookup (e.g., a constexpr std::array/std::pair table or two small
grouped helper maps) that maps SDL scancodes to ImGuiKey values, and have
SdlScancodeToImGuiKey perform a O(1) lookup (with a default ImGuiKey_None
fallback). Locate the mapping inside the src/core/pad.cc file (symbols:
SdlScancodeToImGuiKey) and refactor the switch into a compile-time table (or
grouped tables for contiguous ranges like letters, numbers, keypad, function
keys) to preserve current behavior and return values, then update the function
to consult that table and return ImGuiKey_None when not found.
| void PCSX::GUI::setFullscreen(bool fullscreen) { | ||
| m_fullscreen = fullscreen; | ||
| if (fullscreen) { | ||
| glfwGetWindowPos(m_window, &m_glfwPosX, &m_glfwPosY); | ||
| glfwGetWindowSize(m_window, &m_glfwSizeX, &m_glfwSizeY); | ||
| const GLFWvidmode* mode = glfwGetVideoMode(glfwGetPrimaryMonitor()); | ||
| glfwSetWindowMonitor(m_window, glfwGetPrimaryMonitor(), 0, 0, mode->width, mode->height, GLFW_DONT_CARE); | ||
| // Snapshot windowed pos/size before the transition so we can restore them. | ||
| SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY); | ||
| SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY); | ||
| // No fullscreen mode set -> SDL gives us borderless desktop fullscreen, | ||
| // which is the shape glfwSetWindowMonitor(..., GLFW_DONT_CARE) produced. | ||
| SDL_SetWindowFullscreenMode(m_window, nullptr); | ||
| SDL_SetWindowFullscreen(m_window, true); | ||
| } else { | ||
| glfwSetWindowMonitor(m_window, nullptr, m_glfwPosX, m_glfwPosY, m_glfwSizeX, m_glfwSizeY, GLFW_DONT_CARE); | ||
| SDL_SetWindowFullscreen(m_window, false); | ||
| SDL_SetWindowPosition(m_window, m_windowPosX, m_windowPosY); | ||
| SDL_SetWindowSize(m_window, m_windowSizeX, m_windowSizeY); | ||
| } |
There was a problem hiding this comment.
Restore maximized state when leaving fullscreen.
If the window was maximized before entering fullscreen, this path always comes back as a normal window because it only restores position/size. m_windowMaximized is already persisted elsewhere, so the fullscreen toggle should preserve it too.
🪟 Suggested fix
void PCSX::GUI::setFullscreen(bool fullscreen) {
m_fullscreen = fullscreen;
if (fullscreen) {
+ m_windowMaximized = (SDL_GetWindowFlags(m_window) & SDL_WINDOW_MAXIMIZED) != 0;
// Snapshot windowed pos/size before the transition so we can restore them.
SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY);
SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY);
// No fullscreen mode set -> SDL gives us borderless desktop fullscreen,
// which is the shape glfwSetWindowMonitor(..., GLFW_DONT_CARE) produced.
SDL_SetWindowFullscreenMode(m_window, nullptr);
SDL_SetWindowFullscreen(m_window, true);
} else {
SDL_SetWindowFullscreen(m_window, false);
SDL_SetWindowPosition(m_window, m_windowPosX, m_windowPosY);
SDL_SetWindowSize(m_window, m_windowSizeX, m_windowSizeY);
+ if (m_windowMaximized) {
+ SDL_MaximizeWindow(m_window);
+ }
}
}📝 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.
| void PCSX::GUI::setFullscreen(bool fullscreen) { | |
| m_fullscreen = fullscreen; | |
| if (fullscreen) { | |
| glfwGetWindowPos(m_window, &m_glfwPosX, &m_glfwPosY); | |
| glfwGetWindowSize(m_window, &m_glfwSizeX, &m_glfwSizeY); | |
| const GLFWvidmode* mode = glfwGetVideoMode(glfwGetPrimaryMonitor()); | |
| glfwSetWindowMonitor(m_window, glfwGetPrimaryMonitor(), 0, 0, mode->width, mode->height, GLFW_DONT_CARE); | |
| // Snapshot windowed pos/size before the transition so we can restore them. | |
| SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY); | |
| SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY); | |
| // No fullscreen mode set -> SDL gives us borderless desktop fullscreen, | |
| // which is the shape glfwSetWindowMonitor(..., GLFW_DONT_CARE) produced. | |
| SDL_SetWindowFullscreenMode(m_window, nullptr); | |
| SDL_SetWindowFullscreen(m_window, true); | |
| } else { | |
| glfwSetWindowMonitor(m_window, nullptr, m_glfwPosX, m_glfwPosY, m_glfwSizeX, m_glfwSizeY, GLFW_DONT_CARE); | |
| SDL_SetWindowFullscreen(m_window, false); | |
| SDL_SetWindowPosition(m_window, m_windowPosX, m_windowPosY); | |
| SDL_SetWindowSize(m_window, m_windowSizeX, m_windowSizeY); | |
| } | |
| void PCSX::GUI::setFullscreen(bool fullscreen) { | |
| m_fullscreen = fullscreen; | |
| if (fullscreen) { | |
| m_windowMaximized = (SDL_GetWindowFlags(m_window) & SDL_WINDOW_MAXIMIZED) != 0; | |
| // Snapshot windowed pos/size before the transition so we can restore them. | |
| SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY); | |
| SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY); | |
| // No fullscreen mode set -> SDL gives us borderless desktop fullscreen, | |
| // which is the shape glfwSetWindowMonitor(..., GLFW_DONT_CARE) produced. | |
| SDL_SetWindowFullscreenMode(m_window, nullptr); | |
| SDL_SetWindowFullscreen(m_window, true); | |
| } else { | |
| SDL_SetWindowFullscreen(m_window, false); | |
| SDL_SetWindowPosition(m_window, m_windowPosX, m_windowPosY); | |
| SDL_SetWindowSize(m_window, m_windowSizeX, m_windowSizeY); | |
| if (m_windowMaximized) { | |
| SDL_MaximizeWindow(m_window); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 210-210: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING1024'
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gui/gui.cc` around lines 210 - 224, PCSX::GUI::setFullscreen currently
always restores position/size when leaving fullscreen but ignores whether the
window was maximized; update the else branch to restore maximized state by
checking m_windowMaximized and calling SDL_MaximizeWindow(m_window) when true
(otherwise leave as-is or call SDL_RestoreWindow if you need to ensure
unmaximized state). Modify PCSX::GUI::setFullscreen to perform this check after
SDL_SetWindowSize/Position so the window returns to its prior maximized state
using the existing m_windowMaximized flag.
| case SDL_EVENT_WINDOW_RESIZED: | ||
| case SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED: | ||
| if (event.window.windowID == SDL_GetWindowID(m_window)) m_setupScreenSize = true; | ||
| break; | ||
| case SDL_EVENT_WINDOW_DISPLAY_CHANGED: | ||
| // Force a font/scale rebuild when the window crosses to a display | ||
| // with different DPI. This is the case GLFW didn't fire and is the | ||
| // recurring high-DPI bug we're fixing in this phase. | ||
| if (event.window.windowID == SDL_GetWindowID(m_window)) { | ||
| changeScale(SDL_GetWindowDisplayScale(m_window)); | ||
| m_setupScreenSize = true; | ||
| } |
There was a problem hiding this comment.
Refresh the cached windowed geometry on window-state events.
saveCfg() preserves m_windowPosX/Y, m_windowSizeX/Y, and m_windowMaximized whenever the window is minimized or fullscreen, but this switch never updates that cache during normal move/resize/maximize/restore activity. If the user changes the window geometry and quits while minimized/fullscreen, the old geometry is what gets written back.
🧭 Suggested fix
switch (event.type) {
+ case SDL_EVENT_WINDOW_MOVED:
case SDL_EVENT_WINDOW_RESIZED:
case SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED:
+ case SDL_EVENT_WINDOW_MAXIMIZED:
+ case SDL_EVENT_WINDOW_RESTORED:
if (event.window.windowID == SDL_GetWindowID(m_window)) m_setupScreenSize = true;
+ if (event.window.windowID == SDL_GetWindowID(m_window) &&
+ (SDL_GetWindowFlags(m_window) & SDL_WINDOW_FULLSCREEN) == 0) {
+ SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY);
+ SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY);
+ m_windowMaximized = (SDL_GetWindowFlags(m_window) & SDL_WINDOW_MAXIMIZED) != 0;
+ }
break;📝 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.
| case SDL_EVENT_WINDOW_RESIZED: | |
| case SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED: | |
| if (event.window.windowID == SDL_GetWindowID(m_window)) m_setupScreenSize = true; | |
| break; | |
| case SDL_EVENT_WINDOW_DISPLAY_CHANGED: | |
| // Force a font/scale rebuild when the window crosses to a display | |
| // with different DPI. This is the case GLFW didn't fire and is the | |
| // recurring high-DPI bug we're fixing in this phase. | |
| if (event.window.windowID == SDL_GetWindowID(m_window)) { | |
| changeScale(SDL_GetWindowDisplayScale(m_window)); | |
| m_setupScreenSize = true; | |
| } | |
| case SDL_EVENT_WINDOW_MOVED: | |
| case SDL_EVENT_WINDOW_RESIZED: | |
| case SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED: | |
| case SDL_EVENT_WINDOW_MAXIMIZED: | |
| case SDL_EVENT_WINDOW_RESTORED: | |
| if (event.window.windowID == SDL_GetWindowID(m_window)) m_setupScreenSize = true; | |
| if (event.window.windowID == SDL_GetWindowID(m_window) && | |
| (SDL_GetWindowFlags(m_window) & SDL_WINDOW_FULLSCREEN) == 0) { | |
| SDL_GetWindowPosition(m_window, &m_windowPosX, &m_windowPosY); | |
| SDL_GetWindowSize(m_window, &m_windowSizeX, &m_windowSizeY); | |
| m_windowMaximized = (SDL_GetWindowFlags(m_window) & SDL_WINDOW_MAXIMIZED) != 0; | |
| } | |
| break; | |
| case SDL_EVENT_WINDOW_DISPLAY_CHANGED: | |
| // Force a font/scale rebuild when the window crosses to a display | |
| // with different DPI. This is the case GLFW didn't fire and is the | |
| // recurring high-DPI bug we're fixing in this phase. | |
| if (event.window.windowID == SDL_GetWindowID(m_window)) { | |
| changeScale(SDL_GetWindowDisplayScale(m_window)); | |
| m_setupScreenSize = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gui/gui.cc` around lines 956 - 967, On window
move/resize/maximize/restore events refresh the cached windowed geometry
(m_windowPosX, m_windowPosY, m_windowSizeX, m_windowSizeY, m_windowMaximized) so
saveCfg() later writes the current geometry instead of stale values; inside the
SDL event handling cases that affect geometry (e.g., SDL_EVENT_WINDOW_MOVED,
SDL_EVENT_WINDOW_RESIZED, SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED,
SDL_EVENT_WINDOW_MAXIMIZED, SDL_EVENT_WINDOW_RESTORED, and similar where
event.window.windowID == SDL_GetWindowID(m_window)) call
SDL_GetWindowPosition(m_window, &x, &y), SDL_GetWindowSize(m_window, &w, &h) and
update m_windowPosX/Y = x/y, m_windowSizeX/Y = w/h and set m_windowMaximized =
(SDL_GetWindowFlags(m_window) & SDL_WINDOW_MAXIMIZED) != 0; avoid overwriting
when the window is currently minimized or fullscreen if that behavior must be
preserved.
| case SDL_EVENT_DROP_FILE: | ||
| // SDL hands us a UTF-8 path that's owned by SDL and freed when | ||
| // the event is consumed; magicOpen copies what it needs. | ||
| if (event.drop.data) magicOpen(event.drop.data); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In SDL3, after handling SDL_EVENT_DROP_FILE / SDL_DropEvent.data, does the application need to call SDL_free on the path buffer, or is it freed automatically when the event is consumed?
💡 Result:
In SDL3, after you handle SDL_EVENT_DROP_FILE / SDL_DropEvent.data (the file path string), your application must call SDL_free on that path buffer; it is not freed automatically just because the event has been consumed. SDL3’s SDL_DropEvent documents that the drop data is a pointer to a C string (const char *data) for SDL_EVENT_DROP_FILE, and SDL’s event lifetime is managed via an explicit cleanup function rather than by event “consumption” alone [1][2]. SDL also provides (and discusses) an event cleanup API (SDL_CleanupEvent) specifically because the library otherwise won’t know when an application is done with an event’s dynamic memory [3].
Citations:
- 1: https://wiki.libsdl.org/SDL3/SDL_DropEvent
- 2: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_events.h
- 3: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant region around the reported lines
sed -n '920,1020p' src/gui/gui.cc | nl -ba | sed -n '40,120p'
# Find definition/usages of magicOpen in the repo
rg -n "magicOpen\\b" -S src/gui/gui.cc src/gui -g'*.cc' -g'*.h' || true
# Print surrounding definition if found
# (Use rg to get file:line)
MAGIC_DEF=$(rg -n "magicOpen\\b" -S src/gui -g'*.h' -g'*.cc' | head -n 20 | cut -d: -f1 | head -n 1 || true)
if [ -n "${MAGIC_DEF:-}" ]; then
echo "---- magicOpen candidate file: $MAGIC_DEF ----"
sed -n '1,200p' "$MAGIC_DEF" | nl -ba | head -n 200
fiRepository: grumpycoders/pcsx-redux
Length of output: 110
🌐 Web query:
SDL_EVENT_DROP_FILE SDL_DropEvent.data ownership must call SDL_free
💡 Result:
Yes—SDL’s drop events transfer ownership of the dropped filename/text string to the application, and you must free that memory with SDL_free once you’re done with it. SDL_DropEvent (SDL2) documents that the event contains a char* file “which should be freed with SDL_free” and that if the drop events are enabled “you must free the filename in the event.” [1] So, for SDL_EVENT_DROP_FILE/SDL_DROPFILE handling, treat event.drop.data (or the equivalent filename pointer in the drop event struct) as heap memory owned by SDL until you free it, then your code must call SDL_free(pointer) after processing the path. [1] Example pattern: - On receiving a drop-file event: copy event.drop.data / event.drop.file into your own buffer if needed, then call SDL_free on the SDL-provided pointer; do not keep using it after SDL_free. [1]
Citations:
🌐 Web query:
SDL_CleanupEvent SDL_EVENT_DROP_FILE data free
💡 Result:
In SDL3, the event type you care about is SDL_EVENT_DROP_FILE (part of SDL_DropEvent). The dropped filename is provided in SDL_DropEvent.data, and SDL has an API called SDL_CleanupEvent to release any dynamically allocated memory associated with events after you’re done processing them.[1][2][3] So to “SDL_CleanupEvent SDL_EVENT_DROP_FILE data free”: you generally do NOT manually free SDL_DropEvent.data yourself; instead, call SDL_CleanupEvent on the SDL_Event after handling SDL_EVENT_DROP_FILE, so SDL can free any internal allocations.[2][3] Practical pattern (SDL3): 1) Poll events. 2) When you get SDL_EVENT_DROP_FILE, read/use event.dropevent.data (the file name) immediately. 3) After processing that event, call SDL_CleanupEvent(&event) to clean up event-owned dynamic memory. Note: In SDL2, the older SDL_DropEvent has a char* file field that “should be freed with SDL_free” by the application, so the manual free behavior applies there, not SDL3.[4] This matches SDL’s documented reasoning: SDL3 introduced SDL_CleanupEvent specifically to free dynamically allocated memory in events for cases where applications don’t otherwise know when they’re done with event data.[5][2][3]
Citations:
- 1: https://wiki.libsdl.org/SDL3/SDL_DropEvent
- 2: https://discourse.libsdl.org/t/sdl-added-sdl-cleanupevent/47076
- 3: Added SDL_CleanupEvent() libsdl-org/SDL#8466
- 4: https://wiki.libsdl.org/SDL2/SDL_DropEvent
- 5: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code around the reported lines
echo "---- src/gui/gui.cc around 969-973 ----"
sed -n '940,1005p' src/gui/gui.cc | nl -ba
echo "---- Find magicOpen definition/usages ----"
rg -n "magicOpen\\b" -S src -g'*.cc' -g'*.h'Repository: grumpycoders/pcsx-redux
Length of output: 168
🌐 Web query:
SDL3 SDL_DropEvent data ownership application must call SDL_free
💡 Result:
In SDL3, for SDL drop events of type SDL_EVENT_DROP_FILE, the event’s file/path memory is owned by the caller only after you receive the event: SDL allocates the string for you, and the application must free it with SDL_free once it’s done processing the drop event data.[1][2] Note: SDL3’s SDL_DropEvent struct uses const char data (rather than a mutable char), but the ownership rule still applies for the file name string carried by the drop event: you should copy it if you need to keep it beyond the scope of the event handling, and still release the SDL-provided storage via SDL_free after handling the event.[1][3] SDL2’s documentation states the same ownership rule explicitly for SDL_DropEvent: the char* file “should be freed with SDL_free”.[2]
Citations:
- 1: https://wiki.libsdl.org/SDL3/SDL_DropEvent
- 2: https://wiki.libsdl.org/SDL2/SDL_DropEvent
- 3: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_events.h
🌐 Web query:
SDL3 SDL_CleanupEvent SDL_EVENT_DROP_FILE SDL_DropEvent.data
💡 Result:
In SDL3, SDL_EVENT_DROP_FILE is delivered as SDL_EVENT_DROP_FILE inside SDL_DropEvent, and the dropped filename (for DROP_FILE) is provided in SDL_DropEvent.data. Relevant SDL3 definitions 1) Event type SDL_EVENT_DROP_FILE is one of the “Drag and drop events” in SDL3’s event type enum.[1][2] 2) SDL_DropEvent struct and SDL_DropEvent.data meaning In SDL3, SDL_DropEvent has a const char *data field whose meaning depends on the specific drop event type: - For SDL_EVENT_DROP_TEXT: data is the text - For SDL_EVENT_DROP_FILE: data is the file name - For the other drop sub-events: data is NULL This is stated directly in the SDL3 SDL_events.h struct documentation.[2] 3) Cleanup: SDL_CleanupEvent SDL3 introduced SDL_CleanupEvent to free memory associated with certain event types after handling them. SDL maintainers explicitly recommend calling SDL_CleanupEvent after handling SDL_EVENT_DROP_FILE (and SDL_EVENT_DROP_TEXT, etc.), to clean up the memory that SDL allocated for drop event data.[3][4] Practical implication After you handle an SDL_EVENT_DROP_FILE event in your SDL3 event loop, call SDL_CleanupEvent(event) (typically immediately after processing the event) to avoid leaking the memory associated with event.drop.data.[3][4] If you want the exact C field name to read: event->drop.data (const char *).[2]
Citations:
- 1: https://wiki.libsdl.org/SDL3/SDL_EventType
- 2: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_events.h
- 3: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
- 4: https://discourse.libsdl.org/t/sdl-added-sdl-cleanupevent/47076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/gui/gui.cc around 940-1010 with line numbers (no nl) ----"
sed -n '940,1010p' src/gui/gui.cc | awk '{printf "%6d\t%s\n", NR+939, $0}'
echo "---- Look for SDL_CleanupEvent usage in repo ----"
rg -n "SDL_CleanupEvent\\b" -S src || true
echo "---- Look for SDL_EVENT_DROP_FILE handling & SDL_free in gui code ----"
rg -n "SDL_EVENT_DROP_FILE" -S src/gui || true
rg -n "SDL_free\\b" -S src/gui || trueRepository: grumpycoders/pcsx-redux
Length of output: 3929
🌐 Web query:
SDL3 SDL_DropEvent.data freed with SDL_free or via SDL_CleanupEvent
💡 Result:
For SDL3, SDL_DropEvent.data is dynamically allocated by SDL and should be released via SDL_CleanupEvent (not by calling SDL_free directly on the pointer). SDL_DropEvent in SDL3 exposes a const char *data field for SDL_EVENT_DROP_TEXT (text) and SDL_EVENT_DROP_FILE (file name) [1]. SDL added SDL_CleanupEvent specifically to free dynamically allocated memory associated with events [2][3]. The SDL3 drop-event documentation and header confirm that drop events carry text/filename data but do not indicate that the application should free that pointer manually; instead, the intended cleanup path for event-owned allocations is SDL_CleanupEvent [2][3]. Practical guidance: - After you finish processing an event (including drop events), call SDL_CleanupEvent(&event); this will free any heap memory SDL attached to that event [2][3]. - Only use SDL_free(event.drop.data) if SDL documentation for that specific field/event explicitly says it’s the app’s responsibility to free it; the SDL3 drop event field is covered by the general SDL_CleanupEvent mechanism [2][3].
Citations:
- 1: https://manpages.opensuse.org/Leap-16.0/SDL3-devel-doc/SDL_DropEvent.3type
- 2: Added SDL_CleanupEvent() libsdl-org/SDL#8466
- 3: https://discourse.libsdl.org/t/sdl-added-sdl-cleanupevent/47076
🌐 Web query:
SDL3 SDL_CleanupEvent frees memory for SDL_EVENT_DROP_FILE data
💡 Result:
Yes. In SDL3, SDL_CleanupEvent is intended to free any dynamically allocated memory associated with events, and this includes SDL_EVENT_DROP_FILE data. SDL_EVENT_DROP_FILE is one of the event types that can have heap-allocated memory associated with it when enabled (e.g., it allocates memory for each file path). [1][2] In response to the resulting memory leak, SDL3 introduced SDL_CleanupEvent, described as “used to free any dynamically allocated memory in events.” [3][1][4] Additionally, the SDL3 migration documentation explicitly notes that event memory is managed by SDL and that you should not free SDL_EVENT_DROP_FILE data yourself. [5] So, the correct pattern is: after you’ve finished processing an event (including SDL_EVENT_DROP_FILE), call SDL_CleanupEvent(&event) so SDL can release any internal allocations tied to that event. (If you never call it, you may leak memory for such events.) [2][1].
Citations:
- 1: Fix for event memory leaks libsdl-org/SDL#6524
- 2: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
- 3: Added SDL_CleanupEvent() libsdl-org/SDL#8466
- 4: https://discourse.libsdl.org/t/sdl-added-sdl-cleanupevent/47076
- 5: https://wiki.libsdl.org/SDL3/README-migration
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "magicOpen\\b" -S src | head -n 50
fd -t f "magicOpen*" src || trueRepository: grumpycoders/pcsx-redux
Length of output: 369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2670,2760p' src/gui/gui.cc | nl -baRepository: grumpycoders/pcsx-redux
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/gui/gui.cc around magicOpen (2670-2760) ----"
sed -n '2670,2760p' src/gui/gui.cc | cat -nRepository: grumpycoders/pcsx-redux
Length of output: 4506
🌐 Web query:
SDL3 SDL_DropEvent data const char *data freed with SDL_free or via SDL_CleanupEvent exact wording
💡 Result:
SDL3: The SDL_DropEvent.data pointer (const char *) is dynamically allocated by SDL, and is freed by calling SDL_CleanupEvent. SDL_DropEvent wording (SDL3): SDL_DropEvent has a member const char *data; described as “The text for SDL_EVENT_DROP_TEXT and the file name for SDL_EVENT_DROP_FILE, NULL for other events” [1][2]. SDL_CleanupEvent wording (SDL3): SDL3 added SDL_CleanupEvent “to free any dynamically allocated memory in events” [3]. The SDL3 event-memory-leak fix discussion also states that for SDL3, “we’ve added a function SDL_CleanupEvent … which will free the memory” and that apps can call it “after each event is processed and guarantee that no memory will be leaked” [4]. SDL_free vs SDL_CleanupEvent: There is no indication in the SDL3 drop-event docs that applications should free SDL_DropEvent.data with SDL_free; instead, SDL_CleanupEvent is the mechanism SDL provides to free dynamically allocated event payload memory [3][4]. So the exact guidance is: after handling an SDL event that may contain dynamically allocated payload (including SDL_DropEvent text/file data), call SDL_CleanupEvent to release that memory [3][4].
Citations:
- 1: https://manpages.opensuse.org/Leap-16.0/SDL3-devel-doc/SDL_DropEvent.3type
- 2: https://manpages.opensuse.org/Tumbleweed/SDL3-devel-doc/SDL_DropEvent.3type.en.html
- 3: Added SDL_CleanupEvent() libsdl-org/SDL#8466
- 4: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
🌐 Web query:
SDL3 SDL_CleanupEvent documentation SDL_EVENT_DROP_FILE frees data
💡 Result:
In SDL3, the memory associated with SDL_EVENT_DROP_FILE (and related drag-and-drop event payloads) is owned/managed by SDL, and you should not manually free the event’s data pointer. Instead, you should call SDL_CleanupEvent after handling the event so SDL can release the internal allocations. Evidence: - The SDL3 migration notes explicitly say “Event memory is now managed by SDL, so you should not free the data in SDL_EVENT_DROP_FILE”. It also notes the event data is dynamically allocated. [1] - SDL3 introduced SDL_CleanupEvent specifically “to… free the memory” for SDL3 events, and applications that call it after each event is processed “guarantee that no memory will be leaked” (with SDL_EVENT_DROP_FILE called out as part of that cleanup set). [2] - The SDL drop event structure shows that SDL_EVENT_DROP_FILE uses SDL_DropEvent::data as “the file name”. [3] So, for a typical SDL3 event loop: - When you receive an SDL_EVENT_DROP_FILE, read event.drop.data (const char*) for the file path. - Do not free it yourself. - After you finish processing the event, call SDL_CleanupEvent(&event) (or equivalent SDL3 API usage) to let SDL release the allocation tied to that event.
Citations:
- 1: https://github.com/libsdl-org/SDL/blob/main/docs/README-migration.md
- 2: SDL_DROPFILE Memory Leak libsdl-org/SDL#3540
- 3: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_events.h
Fix SDL3 drop-path lifetime handling (call SDL_CleanupEvent, don’t assume event “consumption” frees it).
In src/gui/gui.cc, the SDL_EVENT_DROP_FILE handler assumes SDL frees event.drop.data when the event is consumed, but SDL3’s dynamically allocated drop payload is released via SDL_CleanupEvent(&event). Without calling it, SDL_EVENT_DROP_FILE can leak memory per drop.
Minimal fix
while (SDL_PollEvent(&event)) {
ImGui_ImplSDL3_ProcessEvent(&event);
switch (event.type) {
@@
case SDL_EVENT_DROP_FILE:
// SDL hands us a UTF-8 path that's owned by SDL and freed when
// the event is consumed; magicOpen copies what it needs.
if (event.drop.data) magicOpen(event.drop.data);
break;
@@
default:
break;
}
+ SDL_CleanupEvent(&event);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gui/gui.cc` around lines 969 - 973, The SDL drop-file handler must
explicitly free SDL's dynamically-allocated drop payload rather than assuming it
is freed on event consumption: in the SDL_EVENT_DROP_FILE case, keep the
existing null check and call magicOpen(event.drop.data) when non-null, then
always call SDL_CleanupEvent(&event) afterwards to release event.drop.data;
ensure SDL_CleanupEvent is invoked whether or not magicOpen succeeds so there is
no per-drop leak (refer to the SDL_EVENT_DROP_FILE case, event.drop.data,
magicOpen, and SDL_CleanupEvent).
| if (!SDL_SetAudioStreamGetCallback(m_stream, trampoline, this)) { | ||
| const std::string err = SDL_GetError(); | ||
| SDL_DestroyAudioStream(m_stream); | ||
| m_stream = nullptr; | ||
| SDL_CloseAudioDevice(m_device); | ||
| m_device = 0; | ||
| throw std::runtime_error("Unable to set SDL audio stream callback: " + err); | ||
| } | ||
|
|
||
| if (!SDL_BindAudioStream(m_device, m_stream)) { | ||
| const std::string err = SDL_GetError(); | ||
| SDL_DestroyAudioStream(m_stream); | ||
| m_stream = nullptr; | ||
| SDL_CloseAudioDevice(m_device); | ||
| m_device = 0; | ||
| throw std::runtime_error("Unable to bind SDL audio stream: " + err); | ||
| } |
There was a problem hiding this comment.
Keep the dummy-backend fallback for stream setup failures.
The non-safe path retries with init(true) for SDL init, device open, and stream creation failures, but callback registration and binding still throw immediately. That turns backend-specific setup failures here into startup aborts instead of the same degraded-safe behavior used earlier in init().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spu/sdlaudio.cc` around lines 148 - 164, Currently failures from
SDL_SetAudioStreamGetCallback or SDL_BindAudioStream abort startup by throwing;
instead follow the same degraded-safe fallback used earlier: on failure of
SDL_SetAudioStreamGetCallback or SDL_BindAudioStream capture SDL_GetError(),
clean up (SDL_DestroyAudioStream(m_stream); m_stream=nullptr;
SDL_CloseAudioDevice(m_device); m_device=0), then switch to the dummy backend by
invoking init(true) or the existing degraded initialization path rather than
throwing; ensure you reference trampoline, m_stream and m_device when locating
the code so the error path mirrors the earlier init(true) retry behavior.
| for (uint32_t f = 0; f < chunk; f++) { | ||
| float l = 0.0f, r = 0.0f; | ||
| for (unsigned i = 0; i < STREAMS; i++) { | ||
| l += static_cast<float>(m_mixBuffers[i][f].L) / | ||
| static_cast<float>(std::numeric_limits<int16_t>::max()); | ||
| r += static_cast<float>(m_mixBuffers[i][f].R) / | ||
| static_cast<float>(std::numeric_limits<int16_t>::max()); | ||
| } | ||
|
|
||
| if (mono) { | ||
| const float lr = (l + r) * 0.5f; | ||
| m_outputBuffer[f * 2 + 0] = lr; | ||
| m_outputBuffer[f * 2 + 1] = lr; | ||
| } else { | ||
| m_outputBuffer[f * 2 + 0] = l; | ||
| m_outputBuffer[f * 2 + 1] = r; | ||
| } |
There was a problem hiding this comment.
Clamp the mixed float output before passing it to SDL.
Each source is normalized and then summed, so voice + CDDA can exceed [-1.0f, 1.0f]. That will emit out-of-range float PCM and clip/distort on loud mixes.
Suggested fix
for (uint32_t f = 0; f < chunk; f++) {
float l = 0.0f, r = 0.0f;
for (unsigned i = 0; i < STREAMS; i++) {
l += static_cast<float>(m_mixBuffers[i][f].L) /
static_cast<float>(std::numeric_limits<int16_t>::max());
r += static_cast<float>(m_mixBuffers[i][f].R) /
static_cast<float>(std::numeric_limits<int16_t>::max());
}
if (mono) {
- const float lr = (l + r) * 0.5f;
+ const float lr = std::clamp((l + r) * 0.5f, -1.0f, 1.0f);
m_outputBuffer[f * 2 + 0] = lr;
m_outputBuffer[f * 2 + 1] = lr;
} else {
- m_outputBuffer[f * 2 + 0] = l;
- m_outputBuffer[f * 2 + 1] = r;
+ m_outputBuffer[f * 2 + 0] = std::clamp(l, -1.0f, 1.0f);
+ m_outputBuffer[f * 2 + 1] = std::clamp(r, -1.0f, 1.0f);
}
}📝 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.
| for (uint32_t f = 0; f < chunk; f++) { | |
| float l = 0.0f, r = 0.0f; | |
| for (unsigned i = 0; i < STREAMS; i++) { | |
| l += static_cast<float>(m_mixBuffers[i][f].L) / | |
| static_cast<float>(std::numeric_limits<int16_t>::max()); | |
| r += static_cast<float>(m_mixBuffers[i][f].R) / | |
| static_cast<float>(std::numeric_limits<int16_t>::max()); | |
| } | |
| if (mono) { | |
| const float lr = (l + r) * 0.5f; | |
| m_outputBuffer[f * 2 + 0] = lr; | |
| m_outputBuffer[f * 2 + 1] = lr; | |
| } else { | |
| m_outputBuffer[f * 2 + 0] = l; | |
| m_outputBuffer[f * 2 + 1] = r; | |
| } | |
| for (uint32_t f = 0; f < chunk; f++) { | |
| float l = 0.0f, r = 0.0f; | |
| for (unsigned i = 0; i < STREAMS; i++) { | |
| l += static_cast<float>(m_mixBuffers[i][f].L) / | |
| static_cast<float>(std::numeric_limits<int16_t>::max()); | |
| r += static_cast<float>(m_mixBuffers[i][f].R) / | |
| static_cast<float>(std::numeric_limits<int16_t>::max()); | |
| } | |
| if (mono) { | |
| const float lr = std::clamp((l + r) * 0.5f, -1.0f, 1.0f); | |
| m_outputBuffer[f * 2 + 0] = lr; | |
| m_outputBuffer[f * 2 + 1] = lr; | |
| } else { | |
| m_outputBuffer[f * 2 + 0] = std::clamp(l, -1.0f, 1.0f); | |
| m_outputBuffer[f * 2 + 1] = std::clamp(r, -1.0f, 1.0f); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spu/sdlaudio.cc` around lines 223 - 239, The mixed float samples from
m_mixBuffers (loop over STREAMS) can exceed [-1.0f,1.0f] and must be clamped
before writing to m_outputBuffer to avoid distortion; modify the mixing block in
sdlaudio.cc (the loop that computes l and r and writes to m_outputBuffer) to
clamp lr/l/r to the range -1.0f..1.0f (e.g. with std::clamp or a simple min/max)
right after computing the summed float(s) and before assigning into
m_outputBuffer[f * 2 + 0/1].
| void PCSX::SPU::SDLAudio::advanceFrames(uint32_t frameCount) { | ||
| m_frameCount.store(frameCount); | ||
|
|
||
| auto total = m_frames.fetch_add(frameCount); | ||
|
|
||
| #if HAS_ATOMIC_WAIT | ||
| auto goalpost = m_goalpost.load(); | ||
| if (goalpost == m_previousGoalpost) return; | ||
|
|
||
| if (((int32_t)(goalpost - total)) > 0) return; | ||
| m_previousGoalpost = goalpost; | ||
| m_triggered++; | ||
| m_triggered.notify_one(); | ||
| #else | ||
| std::unique_lock<std::mutex> l(m_mu); | ||
| auto goalpost = m_goalpost; | ||
| if (goalpost == m_previousGoalpost) return; | ||
|
|
||
| if (((int32_t)(goalpost - total)) > 0) return; | ||
| m_previousGoalpost = goalpost; | ||
| m_triggered++; | ||
| m_cv.notify_one(); | ||
| #endif |
There was a problem hiding this comment.
Wake waitForGoal() on the post-increment frame total.
fetch_add(frameCount) returns the old value, but the goal comparison still uses that pre-increment total. Hitting the goal exactly therefore wakes one chunk/tick late.
Suggested fix
void PCSX::SPU::SDLAudio::advanceFrames(uint32_t frameCount) {
m_frameCount.store(frameCount);
- auto total = m_frames.fetch_add(frameCount);
+ auto total = m_frames.fetch_add(frameCount) + frameCount;
`#if` HAS_ATOMIC_WAIT
auto goalpost = m_goalpost.load();
if (goalpost == m_previousGoalpost) return;
- if (((int32_t)(goalpost - total)) > 0) return;
+ if (static_cast<int32_t>(goalpost - total) > 0) return;
m_previousGoalpost = goalpost;
m_triggered++;
m_triggered.notify_one();
`#else`
std::unique_lock<std::mutex> l(m_mu);
auto goalpost = m_goalpost;
if (goalpost == m_previousGoalpost) return;
- if (((int32_t)(goalpost - total)) > 0) return;
+ if (static_cast<int32_t>(goalpost - total) > 0) return;
m_previousGoalpost = goalpost;
m_triggered++;
m_cv.notify_one();
`#endif`
}📝 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.
| void PCSX::SPU::SDLAudio::advanceFrames(uint32_t frameCount) { | |
| m_frameCount.store(frameCount); | |
| auto total = m_frames.fetch_add(frameCount); | |
| #if HAS_ATOMIC_WAIT | |
| auto goalpost = m_goalpost.load(); | |
| if (goalpost == m_previousGoalpost) return; | |
| if (((int32_t)(goalpost - total)) > 0) return; | |
| m_previousGoalpost = goalpost; | |
| m_triggered++; | |
| m_triggered.notify_one(); | |
| #else | |
| std::unique_lock<std::mutex> l(m_mu); | |
| auto goalpost = m_goalpost; | |
| if (goalpost == m_previousGoalpost) return; | |
| if (((int32_t)(goalpost - total)) > 0) return; | |
| m_previousGoalpost = goalpost; | |
| m_triggered++; | |
| m_cv.notify_one(); | |
| #endif | |
| void PCSX::SPU::SDLAudio::advanceFrames(uint32_t frameCount) { | |
| m_frameCount.store(frameCount); | |
| auto total = m_frames.fetch_add(frameCount) + frameCount; | |
| `#if` HAS_ATOMIC_WAIT | |
| auto goalpost = m_goalpost.load(); | |
| if (goalpost == m_previousGoalpost) return; | |
| if (static_cast<int32_t>(goalpost - total) > 0) return; | |
| m_previousGoalpost = goalpost; | |
| m_triggered++; | |
| m_triggered.notify_one(); | |
| `#else` | |
| std::unique_lock<std::mutex> l(m_mu); | |
| auto goalpost = m_goalpost; | |
| if (goalpost == m_previousGoalpost) return; | |
| if (static_cast<int32_t>(goalpost - total) > 0) return; | |
| m_previousGoalpost = goalpost; | |
| m_triggered++; | |
| m_cv.notify_one(); | |
| `#endif` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spu/sdlaudio.cc` around lines 254 - 276, In
PCSX::SPU::SDLAudio::advanceFrames the code uses auto total =
m_frames.fetch_add(frameCount) which returns the pre-increment value, so the
goal comparison uses the old total and can miss an exact hit; change the
comparison to use the post-increment total (compute newTotal = total +
frameCount or call fetch_add and then add frameCount) when comparing against
m_goalpost and deciding to notify (affecting the branches that update
m_previousGoalpost, m_triggered and call
m_triggered.notify_one()/m_cv.notify_one()) so the wake happens immediately when
the increment reaches the goal.
| // IMGUI_API ImGuiItemFlags GetItemFlags(); // get generic flags of last item | ||
| // Unsupported return type ImGuiItemFlags |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Unsupported ItemFlags bindings in generated iterator =="
rg -n "GetItemFlags|PushItemFlag|Unsupported return type ImGuiItemFlags|Unsupported arg type ImGuiItemFlags" third_party/imgui_lua_bindings/imgui_iterator.inl
echo
echo "== Current generator support for ImGuiItemFlags =="
rg -n "ImGuiItemFlags" third_party/imgui_lua_bindings/generate_imgui_bindings.plRepository: grumpycoders/pcsx-redux
Length of output: 844
Add ImGuiItemFlags handling to Lua bindings generator (GetItemFlags/PushItemFlag)
The generated iterator still marks both IMGUI_API ImGuiItemFlags GetItemFlags() (lines 1799-1800) and IMGUI_API void PushItemFlag(ImGuiItemFlags option, bool enabled) (lines ~396-397) as unsupported (Unsupported return type/arg type ImGuiItemFlags), and generate_imgui_bindings.pl contains no ImGuiItemFlags support, so these APIs are missing from Lua.
💡 Proposed fix (generator side)
- Teach
third_party/imgui_lua_bindings/generate_imgui_bindings.plto recognizeImGuiItemFlagsas a supported “flags” type:- Add
ImGuiItemFlagsto the return-type handling (soGetItemFlagsno longer hits “Unsupported return type ImGuiItemFlags”). - Add
ImGuiItemFlagsto the argument-type parsing/whitelist used for functions likePushItemFlag(so it no longer hits “Unsupported arg type ImGuiItemFlags”).
- Add
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/imgui_lua_bindings/imgui_iterator.inl` around lines 1799 - 1800,
The iterator marks ImGuiItemFlags as unsupported because the bindings generator
doesn't recognize that type; update
third_party/imgui_lua_bindings/generate_imgui_bindings.pl to treat
ImGuiItemFlags as a supported "flags" type: add "ImGuiItemFlags" to the
return-type handling so IMGUI_API ImGuiItemFlags GetItemFlags() is emitted (not
"Unsupported return type"), and add "ImGuiItemFlags" to the argument-type
whitelist/parsing used for function signatures so IMGUI_API void
PushItemFlag(ImGuiItemFlags option, bool enabled) is generated (not "Unsupported
arg type"); ensure the generator emits the corresponding Lua push/convert logic
for flags consistent with other flag types already supported in the script.
| #define SDL_build_config_h_ | ||
|
|
||
| /* General platform specific identifiers */ | ||
| #include <SDL3/SDL_platform_defines.h> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where SDL_platform_defines.h exists and how includes are written nearby.
set -euo pipefail
echo "== Locate SDL_platform_defines.h candidates =="
fd -i 'SDL_platform_defines.h'
echo
echo "== Check include usage patterns for this header =="
rg -n --iglob '*.{h,hpp,c,cc,cpp,vcxproj,txt,mk}' 'SDL_platform_defines\.h|<SDL3/SDL_'
echo
echo "== Inspect SDL-related include dirs in project files =="
rg -n --iglob '*.vcxproj' --iglob 'Makefile' --iglob '*.mk' 'IncludePath|AdditionalIncludeDirectories|-I|SDL3'Repository: grumpycoders/pcsx-redux
Length of output: 35965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect third_party/SDL_build_config.h =="
nl -ba third_party/SDL_build_config.h | sed -n '1,120p'
echo
echo "== Locate SDL_platform_defines.h on disk (exact path check) =="
ls -la third_party/SDL/include/SDL3/SDL_platform_defines.h || true
test -f third_party/SDL/include/SDL3/SDL_platform_defines.h && echo "FOUND: third_party/SDL/include/SDL3/SDL_platform_defines.h" || echo "MISSING: third_party/SDL/include/SDL3/SDL_platform_defines.h"
echo
echo "== Find any SDL_platform_defines.h candidates =="
find third_party -type f -name 'SDL_platform_defines.h' -print | head -n 20
find . -type f -name 'SDL_platform_defines.h' -print | head -n 20
echo
echo "== Check include dirs in vsprojects/SDL/SDL.vcxproj =="
rg -n "AdditionalIncludeDirectories|IncludePath|-I" vsprojects/SDL/SDL.vcxproj || trueRepository: grumpycoders/pcsx-redux
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect third_party/SDL_build_config.h (show lines 25-45) =="
sed -n '25,45p' third_party/SDL_build_config.h | cat -n
echo
echo "== Locate SDL_platform_defines.h on disk (exact path check) =="
ls -la third_party/SDL/include/SDL3/SDL_platform_defines.h 2>/dev/null || true
test -f third_party/SDL/include/SDL3/SDL_platform_defines.h && echo "FOUND: third_party/SDL/include/SDL3/SDL_platform_defines.h" || echo "MISSING: third_party/SDL/include/SDL3/SDL_platform_defines.h"
echo
echo "== Find any SDL_platform_defines.h candidates (limit output) =="
find third_party -type f -name 'SDL_platform_defines.h' -print | head -n 20
find . -type f -name 'SDL_platform_defines.h' -print | head -n 20
echo
echo "== Check include dirs in vsprojects/SDL/SDL.vcxproj =="
rg -n "AdditionalIncludeDirectories|IncludePath|-I" vsprojects/SDL/SDL.vcxproj || trueRepository: grumpycoders/pcsx-redux
Length of output: 1527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ls third_party/SDL/include/SDL3/ (if present) =="
ls -la third_party/SDL/include/SDL3 2>/dev/null || echo "NO: third_party/SDL/include/SDL3"
echo
echo "== Any SDL_platform_defines.h anywhere in repo? (limit) =="
find . -type f -name 'SDL_platform_defines.h' -print | head -n 50
echo
echo "== Any SDL/include include dirs in vsprojects/SDL/SDL.vcxproj? =="
rg -n "third_party\\\\SDL\\\\include|third_party/SDL/include" vsprojects/SDL/SDL.vcxproj || true
echo
echo "== SDL_platform_defines.h references in SDL.vcxproj =="
rg -n "SDL_platform_defines\\.h" vsprojects/SDL/SDL.vcxproj || trueRepository: grumpycoders/pcsx-redux
Length of output: 6422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "IncludePath|AdditionalIncludeDirectories|third_party\\\\SDL\\\\include" vsprojects/SDL/SDL.vcxproj || trueRepository: grumpycoders/pcsx-redux
Length of output: 6623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "`#include` <SDL3/SDL_platform_defines\.h>" third_party/SDL_build_config.hRepository: grumpycoders/pcsx-redux
Length of output: 109
Build blocker: missing SDL3/SDL_platform_defines.h at third_party/SDL_build_config.h:32
#include <SDL3/SDL_platform_defines.h> fails because third_party/SDL/include/SDL3/SDL_platform_defines.h is not present in the repo (empty find results). vsprojects/SDL/SDL.vcxproj still references that header under third_party/SDL/include/SDL3/, so SDL header vendoring/check-in is inconsistent.
- Restore the missing SDL3 header (or update
SDL_build_config.hto the actual header location/source of truth). - Add
third_party/SDL/includeto the compiler include paths (vcxproj currently adds onlythird_party/SDL/src).
🧰 Tools
🪛 Clang (14.0.6)
[error] 32-32: 'SDL3/SDL_platform_defines.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/SDL_build_config.h` at line 32, The include in
third_party/SDL_build_config.h references <SDL3/SDL_platform_defines.h> which is
missing; either restore the SDL3/SDL_platform_defines.h file under
third_party/SDL/include/SDL3 or change the include to point to the actual header
that exists in the repo (e.g., the non-SDL3 path used by the vendor copy), and
update the VS project include settings in vsprojects/SDL/SDL.vcxproj to add
third_party/SDL/include (in addition to third_party/SDL/src) so the compiler can
find the vendored headers; modify third_party/SDL_build_config.h and
vsprojects/SDL/SDL.vcxproj accordingly.
# Conflicts: # Makefile # src/gui/gui.cc # vsprojects/pcsx-redux.sln
1 participant
Blocked on waiting for Ubuntu 26.04.1 so we get SDL3 in LTS.