From 83ea16f40238fa82981221ba65061a7094b2a64b Mon Sep 17 00:00:00 2001
From: Filoppi <filippotarpini@hotmail.it>
Date: Fri, 21 May 2021 01:33:38 +0300
Subject: [PATCH] Qt: Fix IOWindow keeping a shared ptr to devices even after
them being removed by the ControllerInterface this prevented some devices
from being recreated correctly, as they were exclusive (e.g. DInput
Joysticks)
This is achieved by calling Settings::ReleaseDevices(), which releases all the UI devices shared ptrs.
If we are the host (Qt) thread, DevicesChanged() is now called in line, to avoid devices being hanged onto by the UI.
For this, I had to add a method to check whether we are the Host Thread to Qt.
Avoid calling ControllerInterface::RefreshDevices() from the CPU thread if the emulation is running
and we manually refresh devices from Qt, as that is not necessary anymore.
Refactored the way IOWindow lists devices to make it clearer and hold onto disconnected devices.
There were so many issues with the previous code:
-Devices changes would not be reflected until the window was re-opened
-If there was no default device, it would fail to select the device at index 0
-It could have crashed if we had 0 devices
-The default device was not highlighted as such
---
.../DolphinQt/Config/Mapping/IOWindow.cpp | 94 ++++++++++++++++---
.../Core/DolphinQt/Config/Mapping/IOWindow.h | 9 +-
.../Config/Mapping/MappingWindow.cpp | 2 +-
Source/Core/DolphinQt/Host.cpp | 15 ++-
Source/Core/DolphinQt/Host.h | 3 +
Source/Core/DolphinQt/Main.cpp | 2 +
Source/Core/DolphinQt/MainWindow.cpp | 8 +-
Source/Core/DolphinQt/MainWindow.h | 2 +-
Source/Core/DolphinQt/Settings.cpp | 21 ++++-
Source/Core/DolphinQt/Settings.h | 1 +
10 files changed, 130 insertions(+), 27 deletions(-)
diff --git a/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp b/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp
index 27c17ce5fb..6d5c149024 100644
--- a/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp
+++ b/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp
@@ -9,8 +9,6 @@
#include <QComboBox>
#include <QDialogButtonBox>
-#include <QGroupBox>
-#include <QHBoxLayout>
#include <QHeaderView>
#include <QItemDelegate>
#include <QLabel>
@@ -31,6 +29,7 @@
#include "DolphinQt/Config/Mapping/MappingWindow.h"
#include "DolphinQt/QtUtils/BlockUserInputFilter.h"
#include "DolphinQt/QtUtils/ModalMessageBox.h"
+#include "DolphinQt/Settings.h"
#include "InputCommon/ControlReference/ControlReference.h"
#include "InputCommon/ControlReference/ExpressionParser.h"
@@ -220,6 +219,8 @@ IOWindow::IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* con
CreateMainLayout();
connect(parent, &MappingWidget::Update, this, &IOWindow::Update);
+ connect(parent->GetParent(), &MappingWindow::ConfigChanged, this, &IOWindow::ConfigChanged);
+ connect(&Settings::Instance(), &Settings::ConfigChanged, this, &IOWindow::ConfigChanged);
setWindowTitle(type == IOWindow::Type::Input ? tr("Configure Input") : tr("Configure Output"));
setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint);
@@ -229,7 +230,7 @@ IOWindow::IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* con
ConnectWidgets();
}
-std::shared_ptr<ciface::Core::Device> IOWindow::GetSelectedDevice()
+std::shared_ptr<ciface::Core::Device> IOWindow::GetSelectedDevice() const
{
return m_selected_device;
}
@@ -258,7 +259,7 @@ void IOWindow::CreateMainLayout()
m_expression_text->setFont(QFontDatabase::systemFont(QFontDatabase::FixedFont));
new ControlExpressionSyntaxHighlighter(m_expression_text->document());
- m_operators_combo = new QComboBoxWithMouseWheelDisabled();
+ m_operators_combo = new QComboBoxWithMouseWheelDisabled(this);
m_operators_combo->addItem(tr("Operators"));
m_operators_combo->insertSeparator(1);
if (m_type == Type::Input)
@@ -340,6 +341,7 @@ void IOWindow::CreateMainLayout()
m_option_list->horizontalHeader()->setSectionResizeMode(1, QHeaderView::Fixed);
m_option_list->setItemDelegate(new InputStateDelegate(this, 1, [&](int row) {
+ std::lock_guard lock(m_selected_device_mutex);
// Clamp off negative values but allow greater than one in the text display.
return std::max(GetSelectedDevice()->Inputs()[row]->GetState(), 0.0);
}));
@@ -400,7 +402,7 @@ void IOWindow::CreateMainLayout()
void IOWindow::ConfigChanged()
{
const QSignalBlocker blocker(this);
- const auto lock = m_controller->GetStateLock();
+ const auto lock = ControllerEmu::EmulatedController::GetStateLock();
// ensure m_parse_text is in the right state
UpdateExpression(m_reference->GetExpression(), UpdateMode::Force);
@@ -410,10 +412,10 @@ void IOWindow::ConfigChanged()
m_range_spinbox->setValue(m_reference->range * SLIDER_TICK_COUNT);
m_range_slider->setValue(m_reference->range * SLIDER_TICK_COUNT);
- m_devq = m_controller->GetDefaultDevice();
+ if (m_devq.ToString().empty())
+ m_devq = m_controller->GetDefaultDevice();
UpdateDeviceList();
- UpdateOptionList();
}
void IOWindow::Update()
@@ -425,6 +427,8 @@ void IOWindow::Update()
void IOWindow::ConnectWidgets()
{
connect(m_select_button, &QPushButton::clicked, [this] { AppendSelectedOption(); });
+ connect(&Settings::Instance(), &Settings::ReleaseDevices, this, &IOWindow::ReleaseDevices);
+ connect(&Settings::Instance(), &Settings::DevicesChanged, this, &IOWindow::UpdateDeviceList);
connect(m_detect_button, &QPushButton::clicked, this, &IOWindow::OnDetectButtonPressed);
connect(m_test_button, &QPushButton::clicked, this, &IOWindow::OnTestButtonPressed);
@@ -479,16 +483,19 @@ void IOWindow::ConnectWidgets()
void IOWindow::AppendSelectedOption()
{
- if (m_option_list->currentItem() == nullptr)
+ if (m_option_list->currentRow() < 0)
return;
m_expression_text->insertPlainText(MappingCommon::GetExpressionForControl(
- m_option_list->currentItem()->text(), m_devq, m_controller->GetDefaultDevice()));
+ m_option_list->item(m_option_list->currentRow(), 0)->text(), m_devq,
+ m_controller->GetDefaultDevice()));
}
-void IOWindow::OnDeviceChanged(const QString& device)
+void IOWindow::OnDeviceChanged()
{
- m_devq.FromString(device.toStdString());
+ const std::string device_name =
+ m_devices_combo->count() > 0 ? m_devices_combo->currentData().toString().toStdString() : "";
+ m_devq.FromString(device_name);
UpdateOptionList();
}
@@ -500,7 +507,7 @@ void IOWindow::OnDialogButtonPressed(QAbstractButton* button)
return;
}
- const auto lock = m_controller->GetStateLock();
+ const auto lock = ControllerEmu::EmulatedController::GetStateLock();
UpdateExpression(m_expression_text->toPlainText().toStdString());
m_original_expression = m_reference->GetExpression();
@@ -525,6 +532,7 @@ void IOWindow::OnDetectButtonPressed()
const auto list = m_option_list->findItems(expression, Qt::MatchFixedString);
+ // Try to select the first. If this fails, the last selected item would still appear as such
if (!list.empty())
m_option_list->setCurrentItem(list[0]);
}
@@ -541,8 +549,15 @@ void IOWindow::OnRangeChanged(int value)
m_range_slider->setValue(m_reference->range * SLIDER_TICK_COUNT);
}
+void IOWindow::ReleaseDevices()
+{
+ std::lock_guard lock(m_selected_device_mutex);
+ m_selected_device = nullptr;
+}
+
void IOWindow::UpdateOptionList()
{
+ std::lock_guard lock(m_selected_device_mutex);
m_selected_device = g_controller_interface.FindDevice(m_devq);
m_option_list->setRowCount(0);
@@ -575,13 +590,62 @@ void IOWindow::UpdateOptionList()
void IOWindow::UpdateDeviceList()
{
+ const QSignalBlocker blocker(m_devices_combo);
+
+ const auto previous_device_name = m_devices_combo->currentData().toString().toStdString();
+
m_devices_combo->clear();
+ // Default to the default device or to the first device if there isn't a default.
+ // Try to the keep the previous selected device, mark it as disconnected if it's gone, as it could
+ // reconnect soon after if this is a devices refresh and it would be annoying to lose the value.
+ const auto default_device_name = m_controller->GetDefaultDevice().ToString();
+ int default_device_index = -1;
+ int previous_device_index = -1;
for (const auto& name : g_controller_interface.GetAllDeviceStrings())
- m_devices_combo->addItem(QString::fromStdString(name));
+ {
+ QString qname = QString();
+ if (name == default_device_name)
+ {
+ default_device_index = m_devices_combo->count();
+ // Specify "default" even if we only have one device
+ qname.append(QLatin1Char{'['} + tr("default") + QStringLiteral("] "));
+ }
+ if (name == previous_device_name)
+ {
+ previous_device_index = m_devices_combo->count();
+ }
+ qname.append(QString::fromStdString(name));
+ m_devices_combo->addItem(qname, QString::fromStdString(name));
+ }
- m_devices_combo->setCurrentText(
- QString::fromStdString(m_controller->GetDefaultDevice().ToString()));
+ if (previous_device_index >= 0)
+ {
+ m_devices_combo->setCurrentIndex(previous_device_index);
+ }
+ else if (!previous_device_name.empty())
+ {
+ const QString qname = QString::fromStdString(previous_device_name);
+ QString adjusted_qname;
+ if (previous_device_name == default_device_name)
+ {
+ adjusted_qname.append(QLatin1Char{'['} + tr("default") + QStringLiteral("] "));
+ }
+ adjusted_qname.append(QLatin1Char{'['} + tr("disconnected") + QStringLiteral("] "))
+ .append(qname);
+ m_devices_combo->addItem(adjusted_qname, qname);
+ m_devices_combo->setCurrentIndex(m_devices_combo->count() - 1);
+ }
+ else if (default_device_index >= 0)
+ {
+ m_devices_combo->setCurrentIndex(default_device_index);
+ }
+ else if (m_devices_combo->count() > 0)
+ {
+ m_devices_combo->setCurrentIndex(0);
+ }
+ // The device object might have changed so we need to always refresh it
+ OnDeviceChanged();
}
void IOWindow::UpdateExpression(std::string new_expression, UpdateMode mode)
diff --git a/Source/Core/DolphinQt/Config/Mapping/IOWindow.h b/Source/Core/DolphinQt/Config/Mapping/IOWindow.h
index 320ebefde4..7d22ddbc0d 100644
--- a/Source/Core/DolphinQt/Config/Mapping/IOWindow.h
+++ b/Source/Core/DolphinQt/Config/Mapping/IOWindow.h
@@ -5,6 +5,7 @@
#pragma once
#include <memory>
+#include <mutex>
#include <string>
#include <QComboBox>
@@ -69,16 +70,16 @@ public:
explicit IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* m_controller,
ControlReference* ref, Type type);
- std::shared_ptr<ciface::Core::Device> GetSelectedDevice();
-
private:
+ std::shared_ptr<ciface::Core::Device> GetSelectedDevice() const;
+
void CreateMainLayout();
void ConnectWidgets();
void ConfigChanged();
void Update();
void OnDialogButtonPressed(QAbstractButton* button);
- void OnDeviceChanged(const QString& device);
+ void OnDeviceChanged();
void OnDetectButtonPressed();
void OnTestButtonPressed();
void OnRangeChanged(int range);
@@ -86,6 +87,7 @@ private:
void AppendSelectedOption();
void UpdateOptionList();
void UpdateDeviceList();
+ void ReleaseDevices();
enum class UpdateMode
{
@@ -135,4 +137,5 @@ private:
ciface::Core::DeviceQualifier m_devq;
Type m_type;
std::shared_ptr<ciface::Core::Device> m_selected_device;
+ std::mutex m_selected_device_mutex;
};
diff --git a/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp b/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp
index ac6230cec4..f3aed4eea0 100644
--- a/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp
+++ b/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp
@@ -328,7 +328,7 @@ bool MappingWindow::IsMappingAllDevices() const
void MappingWindow::RefreshDevices()
{
- Core::RunAsCPUThread([&] { g_controller_interface.RefreshDevices(); });
+ g_controller_interface.RefreshDevices();
}
void MappingWindow::OnGlobalDevicesChanged()
diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp
index b0cfb507bd..f85b0b7262 100644
--- a/Source/Core/DolphinQt/Host.cpp
+++ b/Source/Core/DolphinQt/Host.cpp
@@ -35,6 +35,8 @@
#include "VideoCommon/RenderBase.h"
#include "VideoCommon/VideoConfig.h"
+static thread_local bool tls_is_host_thread = false;
+
Host::Host()
{
State::SetOnAfterLoadCallback([] { Host_UpdateDisasmDialog(); });
@@ -51,6 +53,16 @@ Host* Host::GetInstance()
return s_instance;
}
+void Host::DeclareAsHostThread()
+{
+ tls_is_host_thread = true;
+}
+
+bool Host::IsHostThread()
+{
+ return tls_is_host_thread;
+}
+
void Host::SetRenderHandle(void* handle)
{
m_render_to_main = Config::Get(Config::MAIN_RENDER_TO_MAIN);
@@ -62,8 +74,7 @@ void Host::SetRenderHandle(void* handle)
if (g_renderer)
{
g_renderer->ChangeSurface(handle);
- if (g_controller_interface.IsInit())
- g_controller_interface.ChangeWindow(handle);
+ g_controller_interface.ChangeWindow(handle);
}
}
diff --git a/Source/Core/DolphinQt/Host.h b/Source/Core/DolphinQt/Host.h
index 6be28d5464..90c8b7042d 100644
--- a/Source/Core/DolphinQt/Host.h
+++ b/Source/Core/DolphinQt/Host.h
@@ -22,6 +22,9 @@ public:
static Host* GetInstance();
+ void DeclareAsHostThread();
+ bool IsHostThread();
+
bool GetRenderFocus();
bool GetRenderFullFocus();
bool GetRenderFullscreen();
diff --git a/Source/Core/DolphinQt/Main.cpp b/Source/Core/DolphinQt/Main.cpp
index 9efad48c35..1399a40391 100644
--- a/Source/Core/DolphinQt/Main.cpp
+++ b/Source/Core/DolphinQt/Main.cpp
@@ -120,6 +120,8 @@ int WINAPI wWinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, PWSTR pCmdLine
}
#endif
+ Host::GetInstance()->DeclareAsHostThread();
+
#ifdef __APPLE__
// On macOS, a command line option matching the format "-psn_X_XXXXXX" is passed when
// the application is launched for the first time. This is to set the "ProcessSerialNumber",
diff --git a/Source/Core/DolphinQt/MainWindow.cpp b/Source/Core/DolphinQt/MainWindow.cpp
index 3b01fec11b..265155f2b9 100644
--- a/Source/Core/DolphinQt/MainWindow.cpp
+++ b/Source/Core/DolphinQt/MainWindow.cpp
@@ -789,7 +789,7 @@ void MainWindow::TogglePause()
void MainWindow::OnStopComplete()
{
m_stop_requested = false;
- HideRenderWidget();
+ HideRenderWidget(true, m_exit_requested);
#ifdef USE_DISCORD_PRESENCE
if (!m_netplay_dialog->isVisible())
Discord::UpdateDiscordPresence();
@@ -1099,7 +1099,7 @@ void MainWindow::ShowRenderWidget()
}
}
-void MainWindow::HideRenderWidget(bool reinit)
+void MainWindow::HideRenderWidget(bool reinit, bool is_exit)
{
if (m_rendering_to_main)
{
@@ -1136,7 +1136,9 @@ void MainWindow::HideRenderWidget(bool reinit)
// The controller interface will still be registered to the old render widget, if the core
// has booted. Therefore, we should re-bind it to the main window for now. When the core
// is next started, it will be swapped back to the new render widget.
- g_controller_interface.ChangeWindow(GetWindowSystemInfo(windowHandle()).render_window);
+ g_controller_interface.ChangeWindow(GetWindowSystemInfo(windowHandle()).render_window,
+ is_exit ? ControllerInterface::WindowChangeReason::Exit :
+ ControllerInterface::WindowChangeReason::Other);
}
}
diff --git a/Source/Core/DolphinQt/MainWindow.h b/Source/Core/DolphinQt/MainWindow.h
index 79383b3541..7d9e97de28 100644
--- a/Source/Core/DolphinQt/MainWindow.h
+++ b/Source/Core/DolphinQt/MainWindow.h
@@ -142,7 +142,7 @@ private:
const std::optional<std::string>& savestate_path = {});
void StartGame(std::unique_ptr<BootParameters>&& parameters);
void ShowRenderWidget();
- void HideRenderWidget(bool reinit = true);
+ void HideRenderWidget(bool reinit = true, bool is_exit = false);
void ShowSettingsWindow();
void ShowGeneralWindow();
diff --git a/Source/Core/DolphinQt/Settings.cpp b/Source/Core/DolphinQt/Settings.cpp
index 7541a9087f..3f9fb663f0 100644
--- a/Source/Core/DolphinQt/Settings.cpp
+++ b/Source/Core/DolphinQt/Settings.cpp
@@ -25,6 +25,7 @@
#include "Core/NetPlayClient.h"
#include "Core/NetPlayServer.h"
+#include "DolphinQt/Host.h"
#include "DolphinQt/QtUtils/QueueOnObject.h"
#include "InputCommon/ControllerInterface/ControllerInterface.h"
@@ -44,8 +45,24 @@ Settings::Settings()
Config::AddConfigChangedCallback(
[this] { QueueOnObject(this, [this] { emit ConfigChanged(); }); });
- g_controller_interface.RegisterDevicesChangedCallback(
- [this] { QueueOnObject(this, [this] { emit DevicesChanged(); }); });
+ g_controller_interface.RegisterDevicesChangedCallback([this] {
+ if (Host::GetInstance()->IsHostThread())
+ {
+ emit DevicesChanged();
+ }
+ else
+ {
+ // Any device shared_ptr in the host thread needs to be released immediately as otherwise
+ // they'd continue living until the queued event has run, but some devices can't be recreated
+ // until they are destroyed.
+ // This is safe from any thread. Devices will be refreshed and re-acquired and in
+ // DevicesChanged(). Waiting on QueueOnObject() to have finished running was not feasible as
+ // it would cause deadlocks without heavy workarounds.
+ emit ReleaseDevices();
+
+ QueueOnObject(this, [this] { emit DevicesChanged(); });
+ }
+ });
}
Settings::~Settings() = default;
diff --git a/Source/Core/DolphinQt/Settings.h b/Source/Core/DolphinQt/Settings.h
index d5fd90989c..e2c5dbde9e 100644
--- a/Source/Core/DolphinQt/Settings.h
+++ b/Source/Core/DolphinQt/Settings.h
@@ -192,6 +192,7 @@ signals:
void AutoUpdateTrackChanged(const QString& mode);
void FallbackRegionChanged(const DiscIO::Region& region);
void AnalyticsToggled(bool enabled);
+ void ReleaseDevices();
void DevicesChanged();
void SDCardInsertionChanged(bool inserted);
void USBKeyboardConnectionChanged(bool connected);