Conversation
This reverts commit 13e81a9.
There was a problem hiding this comment.
Pull request overview
This pull request implements a reconnection grace period for wireless (Bluetooth) controllers to prevent them from losing their player slot during temporary disconnections.
Changes:
- Added a 3-second grace period for wireless controller disconnections
- Reserved player slots are maintained during the grace period and reused when the same device reconnects
- Changed event loop from blocking indefinitely to using a 200ms timeout to enable periodic grace period expiration checks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/input2/sdl/sdl_input_service.hpp | Added data structures for pending removals tracking, reserved slots, and preferred player index; included necessary headers (chrono, optional, set) |
| src/app/input2/sdl/sdl_input_service.cpp | Implemented grace period mechanism with device matching, slot reservation, and reconnection logic; modified event loop to support periodic cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int i = 0; i < MAX_PLAYERS; ++i) { | ||
| if (m_playerSlots.contains(i) && m_playerSlots[i] == *it) { | ||
| m_playerSlots.erase(i); | ||
| spdlog::info("Removed player slot for player {}", i + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code at lines 141-145 is duplicating cleanup logic that's already performed in the else branch (lines 130-135). While this doesn't cause a functional bug (since the wireless branch doesn't clear m_playerSlots before this point), the duplication makes the code harder to maintain and understand. Consider restructuring to make it clearer which cleanup applies to which branch, for example by extracting a common cleanup method or reorganizing the conditional logic.
| void SDLInputService::prunePendingRemovals() { | ||
| if (m_pendingRemovals.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| const auto now = std::chrono::steady_clock::now(); | ||
| for (auto it = m_pendingRemovals.begin(); it != m_pendingRemovals.end();) { | ||
| if (now - it->removedAt < RECONNECT_GRACE) { | ||
| ++it; | ||
| continue; | ||
| } | ||
|
|
||
| if (it->playerIndex >= 0) { | ||
| spdlog::info("Removing gamepad after reconnect grace: {}", | ||
| it->deviceName); | ||
| spdlog::info("Removed player slot for player {}", it->playerIndex + 1); | ||
| EventDispatcher::instance().publish( | ||
| GamepadDisconnectedEvent{it->playerIndex}); | ||
| m_reservedPlayerSlots.erase(it->playerIndex); | ||
| } | ||
|
|
||
| it = m_pendingRemovals.erase(it); | ||
| } | ||
| } | ||
|
|
||
| std::optional<int> | ||
| SDLInputService::consumeReconnectSlot(const DeviceIdentifier &identifier) { | ||
| const auto now = std::chrono::steady_clock::now(); | ||
| for (auto it = m_pendingRemovals.begin(); it != m_pendingRemovals.end(); | ||
| ++it) { | ||
| if (now - it->removedAt >= RECONNECT_GRACE) { | ||
| continue; | ||
| } | ||
| if (!isSameDevice(identifier, it->identifier)) { | ||
| continue; | ||
| } | ||
|
|
||
| const auto playerIndex = it->playerIndex; | ||
| m_reservedPlayerSlots.erase(playerIndex); | ||
| m_pendingRemovals.erase(it); | ||
| return playerIndex; | ||
| } | ||
|
|
||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
The new wireless controller reconnection grace period functionality lacks test coverage. This includes the prunePendingRemovals method, consumeReconnectSlot method, and the grace period behavior in removeGamepadByInstanceId. Given that the existing codebase has comprehensive test coverage for this module (tests/app/input/input_service_impl_test.cpp), tests should be added to verify:
- Wireless controllers enter grace period on disconnect and don't immediately trigger GamepadDisconnectedEvent
- Reconnecting the same wireless device within the grace period reuses the same player slot
- The grace period expires correctly and triggers cleanup after RECONNECT_GRACE duration
- Reserved player slots prevent new controllers from using them during grace period
- Wired controllers continue to work as before without grace period
| m_reservedPlayerSlots.erase(preferred); | ||
| } |
There was a problem hiding this comment.
If a reconnecting wireless controller's preferred slot is not available (line 62 check fails), the reservation in m_reservedPlayerSlots is never removed. The entry was already removed from m_pendingRemovals by consumeReconnectSlot (line 734 in openSdlGamepad), so prunePendingRemovals won't clean it up later. This leaves the slot permanently reserved and unavailable for future controllers.
The reservation should be removed even when the preferred slot is not used. Move line 64 outside the if (slotAvailable) block, or add an else clause that also erases the reservation.
| m_reservedPlayerSlots.erase(preferred); | |
| } | |
| } | |
| m_reservedPlayerSlots.erase(preferred); |
No description provided.