ControllerInterface/Win32: Prevent devcies from losing their "id" on a hotplug event.

Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp

@@ -93,6 +93,9 @@ void ControllerInterface::RefreshDevices()
 
   m_is_populating_devices = true;
 
+  // Make sure shared_ptr<Device> objects are released before repopulating.
+  InvokeDevicesChangedCallbacks();
+
 #ifdef CIFACE_USE_WIN32
   ciface::Win32::PopulateDevices(m_wsi.render_surface);
 #endif
@@ -179,21 +182,29 @@ void ControllerInterface::AddDevice(std::shared_ptr<ciface::Core::Device> device
 
   {
     std::lock_guard<std::mutex> lk(m_devices_mutex);
-    // Try to find an ID for this device
+
-    int id = 0;
+    const auto is_id_in_use = [&device, this](int id) {
-    while (true)
+      return std::any_of(m_devices.begin(), m_devices.end(), [&device, &id](const auto& d) {
+        return d->GetSource() == device->GetSource() && d->GetName() == device->GetName() &&
+               d->GetId() == id;
+      });
+    };
+
+    const auto preferred_id = device->GetPreferredId();
+    if (preferred_id.has_value() && !is_id_in_use(*preferred_id))
     {
-      const auto it =
+      // Use the device's preferred ID if available.
-          std::find_if(m_devices.begin(), m_devices.end(), [&device, &id](const auto& d) {
+      device->SetId(*preferred_id);
-            return d->GetSource() == device->GetSource() && d->GetName() == device->GetName() &&
+    }
-                   d->GetId() == id;
+    else
-          });
+    {
-      if (it == m_devices.end())  // no device with the same name with this ID, so we can use it
+      // Find the first available ID to use.
-        break;
+      int id = 0;
-      else
+      while (is_id_in_use(id))
-        id++;
+        ++id;
+
+      device->SetId(id);
     }
-    device->SetId(id);
 
     NOTICE_LOG(SERIALINTERFACE, "Added device: %s", device->GetQualifiedName().c_str());
     m_devices.emplace_back(std::move(device));

Source/Core/InputCommon/ControllerInterface/DInput/DInput.cpp

@@ -3,9 +3,11 @@
 // Refer to the license.txt file included.
 
 #include "InputCommon/ControllerInterface/DInput/DInput.h"
-#include "Common/StringUtil.h"
-#include "InputCommon/ControllerInterface/ControllerInterface.h"
 
+#include "Common/Logging/Log.h"
+#include "Common/StringUtil.h"
+
+#include "InputCommon/ControllerInterface/ControllerInterface.h"
 #include "InputCommon/ControllerInterface/DInput/DInputJoystick.h"
 #include "InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h"
 
@@ -40,25 +42,32 @@ std::string GetDeviceName(const LPDIRECTINPUTDEVICE8 device)
   {
     result = StripSpaces(UTF16ToUTF8(str.wsz));
   }
+  else
+  {
+    ERROR_LOG(PAD, "GetProperty(DIPROP_PRODUCTNAME) failed.");
+  }
 
   return result;
 }
 
 void PopulateDevices(HWND hwnd)
 {
-  g_controller_interface.RemoveDevice([](const auto* dev) { return dev->GetSource() == "DInput"; });
+  // Remove unplugged devices.
+  g_controller_interface.RemoveDevice(
+      [](const auto* dev) { return dev->GetSource() == DINPUT_SOURCE_NAME && !dev->IsValid(); });
 
   IDirectInput8* idi8;
   if (FAILED(DirectInput8Create(GetModuleHandle(nullptr), DIRECTINPUT_VERSION, IID_IDirectInput8,
                                 (LPVOID*)&idi8, nullptr)))
   {
+    ERROR_LOG(PAD, "DirectInput8Create failed.");
     return;
   }
 
-  InitKeyboardMouse(idi8, hwnd);
+  InitKeyboardMouse(idi8);
   InitJoystick(idi8, hwnd);
 
   idi8->Release();
 }
-}
+}  // namespace DInput
-}
+}  // namespace ciface

Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp

@@ -4,8 +4,9 @@
 
 #include <algorithm>
 #include <limits>
-#include <map>
+#include <set>
 #include <sstream>
+#include <type_traits>
 
 #include "Common/Logging/Log.h"
 #include "InputCommon/ControllerInterface/ControllerInterface.h"
@@ -17,7 +18,19 @@ namespace ciface
 {
 namespace DInput
 {
-#define DATA_BUFFER_SIZE 32
+constexpr DWORD DATA_BUFFER_SIZE = 32;
+
+struct GUIDComparator
+{
+  bool operator()(const GUID& left, const GUID& right) const
+  {
+    static_assert(std::is_trivially_copyable_v<GUID>);
+
+    return memcmp(&left, &right, sizeof(left)) < 0;
+  }
+};
+
+static std::set<GUID, GUIDComparator> s_guids_in_use;
 
 void InitJoystick(IDirectInput8* const idi8, HWND hwnd)
 {
@@ -28,12 +41,18 @@ void InitJoystick(IDirectInput8* const idi8, HWND hwnd)
   std::unordered_set<DWORD> xinput_guids = GetXInputGUIDS();
   for (DIDEVICEINSTANCE& joystick : joysticks)
   {
-    // skip XInput Devices
+    // Skip XInput Devices
     if (xinput_guids.count(joystick.guidProduct.Data1))
     {
       continue;
     }
 
+    // Skip devices we are already using.
+    if (s_guids_in_use.count(joystick.guidInstance))
+    {
+      continue;
+    }
+
     LPDIRECTINPUTDEVICE8 js_device;
     if (SUCCEEDED(idi8->CreateDevice(joystick.guidInstance, &js_device, nullptr)))
     {
@@ -55,7 +74,9 @@ void InitJoystick(IDirectInput8* const idi8, HWND hwnd)
           }
         }
 
+        s_guids_in_use.insert(joystick.guidInstance);
         auto js = std::make_shared<Joystick>(js_device);
+
         // only add if it has some inputs/outputs
         if (js->Inputs().size() || js->Outputs().size())
           g_controller_interface.AddDevice(std::move(js));
@@ -161,6 +182,17 @@ Joystick::Joystick(/*const LPCDIDEVICEINSTANCE lpddi, */ const LPDIRECTINPUTDEVI
 
 Joystick::~Joystick()
 {
+  DIDEVICEINSTANCE info = {};
+  info.dwSize = sizeof(info);
+  if (SUCCEEDED(m_device->GetDeviceInfo(&info)))
+  {
+    s_guids_in_use.erase(info.guidInstance);
+  }
+  else
+  {
+    ERROR_LOG(PAD, "DInputJoystick: GetDeviceInfo failed.");
+  }
+
   DeInitForceFeedback();
 
   m_device->Unacquire();
@@ -177,7 +209,10 @@ std::string Joystick::GetSource() const
   return DINPUT_SOURCE_NAME;
 }
 
-// update IO
+bool Joystick::IsValid() const
+{
+  return SUCCEEDED(m_device->Acquire());
+}
 
 void Joystick::UpdateInput()
 {

Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.h

@@ -68,6 +68,8 @@ public:
   std::string GetName() const override;
   std::string GetSource() const override;
 
+  bool IsValid() const final override;
+
 private:
   const LPDIRECTINPUTDEVICE8 m_device;
 
@@ -75,5 +77,5 @@ private:
 
   bool m_buffered;
 };
-}
+}  // namespace DInput
-}
+}  // namespace ciface

Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp

@@ -29,12 +29,13 @@ static const struct
 #include "InputCommon/ControllerInterface/DInput/NamedKeys.h"  // NOLINT
 };
 
-// lil silly
+// Prevent duplicate keyboard/mouse devices.
-static HWND m_hwnd;
+static bool s_keyboard_mouse_exists = false;
 
-void InitKeyboardMouse(IDirectInput8* const idi8, HWND _hwnd)
+void InitKeyboardMouse(IDirectInput8* const idi8)
 {
-  m_hwnd = _hwnd;
+  if (s_keyboard_mouse_exists)
+    return;
 
   // mouse and keyboard are a combined device, to allow shift+click and stuff
   // if that's dumb, I will make a VirtualDevice class that just uses ranges of inputs/outputs from
@@ -63,6 +64,8 @@ void InitKeyboardMouse(IDirectInput8* const idi8, HWND _hwnd)
 
 KeyboardMouse::~KeyboardMouse()
 {
+  s_keyboard_mouse_exists = false;
+
   // kb
   m_kb_device->Unacquire();
   m_kb_device->Release();
@@ -75,6 +78,8 @@ KeyboardMouse::KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device,
                              const LPDIRECTINPUTDEVICE8 mo_device)
     : m_kb_device(kb_device), m_mo_device(mo_device)
 {
+  s_keyboard_mouse_exists = true;
+
   m_kb_device->Acquire();
   m_mo_device->Acquire();
 
@@ -237,5 +242,5 @@ ControlState KeyboardMouse::Cursor::GetState() const
 {
   return std::max(0.0, ControlState(m_axis) / (m_positive ? 1.0 : -1.0));
 }
-}
+}  // namespace DInput
-}
+}  // namespace ciface

Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h

@@ -13,7 +13,7 @@ namespace ciface
 {
 namespace DInput
 {
-void InitKeyboardMouse(IDirectInput8* const idi8, HWND _hwnd);
+void InitKeyboardMouse(IDirectInput8* const idi8);
 
 class KeyboardMouse : public Core::Device
 {
@@ -98,5 +98,5 @@ private:
   DWORD m_last_update;
   State m_state_in;
 };
-}
+}  // namespace DInput
-}
+}  // namespace ciface

Source/Core/InputCommon/ControllerInterface/Device.cpp

@@ -31,6 +31,11 @@ Device::~Device()
     delete output;
 }
 
+std::optional<int> Device::GetPreferredId() const
+{
+  return {};
+}
+
 void Device::AddInput(Device::Input* const i)
 {
   m_inputs.push_back(i);

Source/Core/InputCommon/ControllerInterface/Device.h

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -82,7 +83,7 @@ public:
   class Output : public Control
   {
   public:
-    virtual ~Output() {}
+    virtual ~Output() = default;
     virtual void SetState(ControlState state) = 0;
     Output* ToOutput() override { return this; }
   };
@@ -95,9 +96,17 @@ public:
   virtual std::string GetSource() const = 0;
   std::string GetQualifiedName() const;
   virtual void UpdateInput() {}
+
+  // May be overridden to implement hotplug removal.
+  // Currently handled on a per-backend basis but this could change.
   virtual bool IsValid() const { return true; }
+
+  // (e.g. Xbox 360 controllers have controller number LEDs which should match the ID we use.)
+  virtual std::optional<int> GetPreferredId() const;
+
   const std::vector<Input*>& Inputs() const { return m_inputs; }
   const std::vector<Output*>& Outputs() const { return m_outputs; }
+
   Input* FindInput(const std::string& name) const;
   Output* FindOutput(const std::string& name) const;
 

Source/Core/InputCommon/ControllerInterface/XInput/XInput.cpp

@@ -194,6 +194,11 @@ void Device::UpdateMotors()
   }
 }
 
+std::optional<int> Device::GetPreferredId() const
+{
+  return m_index;
+}
+
 // GET name/source/id
 
 std::string Device::Button::GetName() const

Source/Core/InputCommon/ControllerInterface/XInput/XInput.h

@@ -92,8 +92,9 @@ public:
 
   Device(const XINPUT_CAPABILITIES& capabilities, u8 index);
 
-  std::string GetName() const override;
+  std::string GetName() const final override;
-  std::string GetSource() const override;
+  std::string GetSource() const final override;
+  std::optional<int> GetPreferredId() const final override;
 
   void UpdateMotors();
 
@@ -104,5 +105,5 @@ private:
   const BYTE m_subtype;
   const u8 m_index;
 };
-}
+}  // namespace XInput
-}
+}  // namespace ciface