HID: Complete refactor of pad/touch input to fix threading issues.

src/citra/emu_window/emu_window_glfw.cpp

@@ -23,18 +23,15 @@ void EmuWindow_GLFW::OnMouseButtonEvent(GLFWwindow* win, int button, int action,
         double x, y;
         glfwGetCursorPos(win, &x, &y);
 
-        if (action == GLFW_PRESS) {
+        if (action == GLFW_PRESS)
-            emu_window->TouchPressed(layout, static_cast<u16>(x), static_cast<u16>(y));
+            emu_window->TouchPressed(static_cast<unsigned>(x), static_cast<unsigned>(y));
-        } else if (action == GLFW_RELEASE) {
+        else if (action == GLFW_RELEASE)
-            emu_window->TouchReleased(layout, static_cast<u16>(x), static_cast<u16>(y));
+            emu_window->TouchReleased();
-        }
     }
 }
 
 void EmuWindow_GLFW::OnCursorPosEvent(GLFWwindow* win, double x, double y) {
-    auto emu_window = GetEmuWindow(win);
+    GetEmuWindow(win)->TouchMoved(static_cast<unsigned>(x), static_cast<unsigned>(y));
-    auto layout = emu_window->GetFramebufferLayout();
-    emu_window->TouchMoved(layout, static_cast<u16>(x), static_cast<u16>(y));
 }
 
 /// Called by GLFW when a key event occurs
@@ -45,10 +42,8 @@ void EmuWindow_GLFW::OnKeyEvent(GLFWwindow* win, int key, int scancode, int acti
     if (action == GLFW_PRESS) {
         emu_window->KeyPressed({key, keyboard_id});
     } else if (action == GLFW_RELEASE) {
-        emu_window->KeyReleased({ key, keyboard_id });
+        emu_window->KeyReleased({key, keyboard_id});
     }
-
-    Service::HID::PadUpdateComplete();
 }
 
 /// Whether the window is still open, and a close request hasn't yet been sent

src/citra_qt/bootmanager.cpp

@@ -268,39 +268,33 @@ QByteArray GRenderWindow::saveGeometry()
 
 void GRenderWindow::keyPressEvent(QKeyEvent* event)
 {
-    EmuWindow::KeyPressed({event->key(), keyboard_id});
+    this->KeyPressed({event->key(), keyboard_id});
-    Service::HID::PadUpdateComplete();
 }
 
 void GRenderWindow::keyReleaseEvent(QKeyEvent* event)
 {
-    EmuWindow::KeyReleased({event->key(), keyboard_id});
+    this->KeyReleased({event->key(), keyboard_id});
-    Service::HID::PadUpdateComplete();
 }
 
 void GRenderWindow::mousePressEvent(QMouseEvent *event)
 {
-    if (event->button() == Qt::LeftButton)  {
+    if (event->button() == Qt::LeftButton)
+    {
         auto pos = event->pos();
-        EmuWindow::TouchPressed(GetFramebufferLayout(), static_cast<u16>(pos.x()),
+        this->TouchPressed(static_cast<unsigned>(pos.x()), static_cast<unsigned>(pos.y()));
-            static_cast<u16>(pos.y()));
     }
 }
 
 void GRenderWindow::mouseMoveEvent(QMouseEvent *event)
 {
     auto pos = event->pos();
-    EmuWindow::TouchMoved(GetFramebufferLayout(), static_cast<u16>(pos.x()),
+    this->TouchMoved(static_cast<unsigned>(pos.x()), static_cast<unsigned>(pos.y()));
-        static_cast<u16>(pos.y()));
 }
 
 void GRenderWindow::mouseReleaseEvent(QMouseEvent *event)
 {
-    if (event->button() == Qt::LeftButton)  {
+    if (event->button() == Qt::LeftButton)
-        auto pos = event->pos();
+        this->TouchReleased();
-        EmuWindow::TouchReleased(GetFramebufferLayout(), static_cast<u16>(pos.x()),
-            static_cast<u16>(pos.y()));
-    }
 }
 
 void GRenderWindow::ReloadSetKeymaps()

src/common/emu_window.cpp

@@ -6,15 +6,11 @@
 #include "video_core/video_core.h"
 
 void EmuWindow::KeyPressed(KeyMap::HostDeviceKey key) {
-    Service::HID::PadState mapped_key = KeyMap::GetPadKey(key);
+    pad_state.hex |= KeyMap::GetPadKey(key).hex;
-
-    Service::HID::PadButtonPress(mapped_key);
 }
 
 void EmuWindow::KeyReleased(KeyMap::HostDeviceKey key) {
-    Service::HID::PadState mapped_key = KeyMap::GetPadKey(key);
+    pad_state.hex &= ~KeyMap::GetPadKey(key).hex;
-
-    Service::HID::PadButtonRelease(mapped_key);
 }
 
 /**
@@ -25,55 +21,41 @@ void EmuWindow::KeyReleased(KeyMap::HostDeviceKey key) {
  * @return True if the coordinates are within the touchpad, otherwise false
  */
 static bool IsWithinTouchscreen(const EmuWindow::FramebufferLayout& layout, unsigned framebuffer_x,
-    unsigned framebuffer_y) {
+                                unsigned framebuffer_y) {
-
+    return (framebuffer_y >= layout.bottom_screen.top    &&
-    return (framebuffer_y >= layout.bottom_screen.top &&
+            framebuffer_y <  layout.bottom_screen.bottom &&
-        framebuffer_y < layout.bottom_screen.bottom &&
+            framebuffer_x >= layout.bottom_screen.left   &&
-        framebuffer_x >= layout.bottom_screen.left &&
+            framebuffer_x <  layout.bottom_screen.right);
-        framebuffer_x < layout.bottom_screen.right);
 }
 
-void EmuWindow::TouchPressed(const FramebufferLayout& layout, unsigned framebuffer_x,
+void EmuWindow::TouchPressed(unsigned framebuffer_x, unsigned framebuffer_y) {
-    unsigned framebuffer_y) {
+    if (!IsWithinTouchscreen(framebuffer_layout, framebuffer_x, framebuffer_y))
+        return;
 
-    if (IsWithinTouchscreen(layout, framebuffer_x, framebuffer_y)) {
+    touch_x = VideoCore::kScreenBottomWidth * (framebuffer_x - framebuffer_layout.bottom_screen.left) /
-        u16 touch_x = VideoCore::kScreenBottomWidth * (framebuffer_x - layout.bottom_screen.left) /
+        (framebuffer_layout.bottom_screen.right - framebuffer_layout.bottom_screen.left);
-            (layout.bottom_screen.right - layout.bottom_screen.left);
+    touch_y = VideoCore::kScreenBottomHeight * (framebuffer_y - framebuffer_layout.bottom_screen.top) /
-        u16 touch_y = VideoCore::kScreenBottomHeight * (framebuffer_y - layout.bottom_screen.top) /
+        (framebuffer_layout.bottom_screen.bottom - framebuffer_layout.bottom_screen.top);
-            (layout.bottom_screen.bottom - layout.bottom_screen.top);
 
-        Service::HID::TouchPress(touch_x, touch_y);
+    touch_pressed = true;
-        Service::HID::TouchUpdateComplete();
+    pad_state.touch = 1;
-
-        touch_pressed = true;
-    }
 }
 
-void EmuWindow::TouchReleased(const FramebufferLayout& layout, unsigned framebuffer_x,
+void EmuWindow::TouchReleased() {
-    unsigned framebuffer_y) {
+    touch_pressed = false;
-
+    touch_x = 0;
-    if (IsWithinTouchscreen(layout, framebuffer_x, framebuffer_y)) {
+    touch_y = 0;
-
+    pad_state.touch = 0;
-        Service::HID::TouchRelease();
-        Service::HID::TouchUpdateComplete();
-
-        touch_pressed = false;
-    }
 }
 
-void EmuWindow::TouchMoved(const FramebufferLayout& layout, unsigned framebuffer_x,
+void EmuWindow::TouchMoved(unsigned framebuffer_x, unsigned framebuffer_y) {
-    unsigned framebuffer_y) {
+    if (!touch_pressed)
+        return;
 
-    if (touch_pressed) {
+    if (IsWithinTouchscreen(framebuffer_layout, framebuffer_x, framebuffer_y))
-        if (IsWithinTouchscreen(layout, framebuffer_x, framebuffer_y)) {
+        TouchPressed(framebuffer_x, framebuffer_y);
-            EmuWindow::TouchPressed(layout, framebuffer_x, framebuffer_y);
+    else
-        } else {
+        TouchReleased();
-            Service::HID::TouchRelease();
-            Service::HID::TouchUpdateComplete();
-
-            touch_pressed = false;
-        }
-    }
 }
 
 EmuWindow::FramebufferLayout EmuWindow::FramebufferLayout::DefaultScreenLayout(unsigned width,

src/common/emu_window.h

@@ -78,27 +78,41 @@ public:
 
     /**
      * Signal that a touch pressed event has occurred (e.g. mouse click pressed)
-     * @param layout FramebufferLayout object describing the framebuffer size and screen positions
      * @param framebuffer_x Framebuffer x-coordinate that was pressed
      * @param framebuffer_y Framebuffer y-coordinate that was pressed
      */
-    void TouchPressed(const FramebufferLayout& layout, unsigned framebuffer_x, unsigned framebuffer_y);
+    void TouchPressed(unsigned framebuffer_x, unsigned framebuffer_y);
 
-    /**
+    /// Signal that a touch released event has occurred (e.g. mouse click released)
-     * Signal that a touch released event has occurred (e.g. mouse click released)
+    void TouchReleased();
-     * @param layout FramebufferLayout object describing the framebuffer size and screen positions
-     * @param framebuffer_x Framebuffer x-coordinate that was released
-     * @param framebuffer_y Framebuffer y-coordinate that was released
-     */
-    void TouchReleased(const FramebufferLayout& layout, unsigned framebuffer_x, unsigned framebuffer_y);
 
     /**
      * Signal that a touch movement event has occurred (e.g. mouse was moved over the emu window)
-     * @param layout FramebufferLayout object describing the framebuffer size and screen positions
      * @param framebuffer_x Framebuffer x-coordinate
      * @param framebuffer_y Framebuffer y-coordinate
      */
-    void TouchMoved(const FramebufferLayout& layout, unsigned framebuffer_x, unsigned framebuffer_y);
+    void TouchMoved(unsigned framebuffer_x, unsigned framebuffer_y);
+
+    /**
+     * Gets the current pad state (which buttons are pressed and the circle pad direction).
+     * @note This should be called by the core emu thread to get a state set by the window thread.
+     * @todo Fix this function to be thread-safe.
+     * @return PadState object indicating the current pad state
+     */
+    const Service::HID::PadState GetPadState() const {
+        return pad_state;
+    }
+
+    /**
+     * Gets the current touch screen state (touch X/Y coordinates and whether or not it is pressed).
+     * @note This should be called by the core emu thread to get a state set by the window thread.
+     * @todo Fix this function to be thread-safe.
+     * @return std::tuple of (x, y, pressed) where `x` and `y` are the touch coordinates and
+     *         `pressed` is true if the touch screen is currently being pressed
+     */
+    const std::tuple<u16, u16, bool>& GetTouchState() const {
+        return std::make_tuple(touch_x, touch_y, touch_pressed);
+    }
 
     /**
      * Returns currently active configuration.
@@ -124,21 +138,15 @@ public:
         return framebuffer_layout;
     }
 
-    /**
-     * Gets window client area width in logical coordinates.
-     * @note For high-DPI systems, this is smaller than the framebuffer size.
-     * @note This method is thread-safe
-     */
-    std::pair<unsigned,unsigned> GetClientAreaSize() const {
-        return std::make_pair(client_area_width, client_area_height);
-    }
-
 protected:
-    EmuWindow()
+    EmuWindow() {
-    {
         // TODO: Find a better place to set this.
         config.min_client_area_size = std::make_pair(400u, 480u);
         active_config = config;
+        pad_state.hex = 0;
+        touch_x = 0;
+        touch_y = 0;
+        touch_pressed = false;
     }
     virtual ~EmuWindow() {}
 
@@ -194,4 +202,9 @@ private:
     WindowConfig active_config;  ///< Internal active configuration
 
     bool touch_pressed;          ///< True if touchpad area is currently pressed, otherwise false
+
+    u16 touch_x;    ///< Touchpad X-position in native 3DS pixel coordinates (0-320)
+    u16 touch_y;    ///< Touchpad Y-position in native 3DS pixel coordinates (0-240)
+
+    Service::HID::PadState pad_state;
 };

src/core/hle/service/hid/hid.cpp

@@ -12,6 +12,8 @@
 #include "core/hle/kernel/shared_memory.h"
 #include "core/hle/hle.h"
 
+#include "video_core/video_core.h"
+
 namespace Service {
 namespace HID {
 
@@ -23,15 +25,8 @@ Kernel::SharedPtr<Kernel::Event> g_event_accelerometer;
 Kernel::SharedPtr<Kernel::Event> g_event_gyroscope;
 Kernel::SharedPtr<Kernel::Event> g_event_debug_pad;
 
-// Next Pad state update information
-static PadState next_state = {{0}};
 static u32 next_pad_index = 0;
-static s16 next_pad_circle_x = 0;
-static s16 next_pad_circle_y = 0;
-
 static u32 next_touch_index = 0;
-static u16 next_touch_x = 0;
-static u16 next_touch_y = 0;
 
 /**
  * Gets a pointer to the PadData structure inside HID shared memory
@@ -55,38 +50,15 @@ static inline SharedMem* GetSharedMem() {
 //     * Set PadData.current_state.circle_left = 1 if current PadEntry.circle_pad_x <= -41
 //     * Set PadData.current_state.circle_right = 1 if current PadEntry.circle_pad_y <= -41
 
-/**
+void HIDUpdate() {
- * Circle Pad from keys.
- *
- * This is implemented as "pushed all the way to an edge (max) or centered (0)".
- *
- * Indicate the circle pad is pushed completely to the edge in 1 of 8 directions.
- */
-static void UpdateNextCirclePadState() {
-    static const s16 max_value = 0x9C;
-    next_pad_circle_x = next_state.circle_left ? -max_value : 0x0;
-    next_pad_circle_x += next_state.circle_right ? max_value : 0x0;
-    next_pad_circle_y = next_state.circle_down ? -max_value : 0x0;
-    next_pad_circle_y += next_state.circle_up ? max_value : 0x0;
-}
-
-void PadButtonPress(const PadState& pad_state) {
-    next_state.hex |= pad_state.hex;
-    UpdateNextCirclePadState();
-}
-
-void PadButtonRelease(const PadState& pad_state) {
-    next_state.hex &= ~pad_state.hex;
-    UpdateNextCirclePadState();
-}
-
-void PadUpdateComplete() {
     SharedMem* shared_mem = GetSharedMem();
 
     if (shared_mem == nullptr)
         return;
 
-    shared_mem->pad.current_state.hex = next_state.hex;
+    const PadState& state = VideoCore::g_emu_window->GetPadState();
+
+    shared_mem->pad.current_state.hex = state.hex;
     shared_mem->pad.index = next_pad_index;
     ++next_touch_index %= shared_mem->pad.entries.size();
 
@@ -95,28 +67,19 @@ void PadUpdateComplete() {
     PadState old_state = shared_mem->pad.entries[last_entry_index].current_state;
 
     // Compute bitmask with 1s for bits different from the old state
-    PadState changed;
+    PadState changed = { { (state.hex ^ old_state.hex) } };
-    changed.hex = (next_state.hex ^ old_state.hex);
-
-    // Compute what was added
-    PadState additions;
-    additions.hex = changed.hex & next_state.hex;
-
-    // Compute what was removed
-    PadState removals;
-    removals.hex = changed.hex & old_state.hex;
 
     // Get the current Pad entry
-    PadDataEntry* current_pad_entry = &shared_mem->pad.entries[shared_mem->pad.index];
+    PadDataEntry* pad_entry = &shared_mem->pad.entries[shared_mem->pad.index];
 
     // Update entry properties
-    current_pad_entry->current_state.hex = next_state.hex;
+    pad_entry->current_state.hex = state.hex;
-    current_pad_entry->delta_additions.hex = additions.hex;
+    pad_entry->delta_additions.hex = changed.hex & state.hex;
-    current_pad_entry->delta_removals.hex = removals.hex;
+    pad_entry->delta_removals.hex = changed.hex & old_state.hex;;
 
     // Set circle Pad
-    current_pad_entry->circle_pad_x = next_pad_circle_x;
+    pad_entry->circle_pad_x = state.circle_left ? -0x9C : state.circle_right ? 0x9C : 0x0;
-    current_pad_entry->circle_pad_y = next_pad_circle_y;
+    pad_entry->circle_pad_y = state.circle_down ? -0x9C : state.circle_up ? 0x9C : 0x0;
 
     // If we just updated index 0, provide a new timestamp
     if (shared_mem->pad.index == 0) {
@@ -124,39 +87,15 @@ void PadUpdateComplete() {
         shared_mem->pad.index_reset_ticks = (s64)Core::g_app_core->GetTicks();
     }
 
-    // Signal both handles when there's an update to Pad or touch
-    g_event_pad_or_touch_1->Signal();
-    g_event_pad_or_touch_2->Signal();
-}
-
-void TouchPress(u16 x, u16 y) {
-    next_touch_x = x;
-    next_touch_y = y;
-}
-
-void TouchRelease() {
-    next_touch_x = 0;
-    next_touch_y = 0;
-}
-
-void TouchUpdateComplete() {
-    SharedMem* shared_mem = GetSharedMem();
-
-    if (shared_mem == nullptr)
-        return;
-
     shared_mem->touch.index = next_touch_index;
     ++next_touch_index %= shared_mem->touch.entries.size();
 
     // Get the current touch entry
-    TouchDataEntry* current_touch_entry = &shared_mem->touch.entries[shared_mem->touch.index];
+    TouchDataEntry* touch_entry = &shared_mem->touch.entries[shared_mem->touch.index];
+    bool pressed = false;
 
-    // Set touchpad position
+    std::tie(touch_entry->x, touch_entry->y, pressed) = VideoCore::g_emu_window->GetTouchState();
-    current_touch_entry->x = next_touch_x;
+    touch_entry->valid = pressed ? 1 : 0;
-    current_touch_entry->y = next_touch_y;
-
-    // TODO(bunnei): Verify this behavior on real hardware
-    current_touch_entry->valid = (next_touch_x || next_touch_y) ? 1 : 0;
 
     // TODO(bunnei): We're not doing anything with offset 0xA8 + 0x18 of HID SharedMemory, which
     // supposedly is "Touch-screen entry, which contains the raw coordinate data prior to being
@@ -194,6 +133,9 @@ void HIDInit() {
 
     g_shared_mem = SharedMemory::Create("HID:SharedMem");
 
+    next_pad_index = 0;
+    next_touch_index = 0;
+
     // Create event handles
     g_event_pad_or_touch_1 = Event::Create(RESETTYPE_ONESHOT, "HID:EventPadOrTouch1");
     g_event_pad_or_touch_2 = Event::Create(RESETTYPE_ONESHOT, "HID:EventPadOrTouch2");
@@ -203,8 +145,8 @@ void HIDInit() {
 }
 
 void HIDShutdown() {
-
 }
 
-}
+} // namespace HID
-}
+
+} // namespace Service

src/core/hle/service/hid/hid.h

@@ -176,38 +176,13 @@ const PadState PAD_CIRCLE_DOWN  = {{1u << 31}};
  */
 void GetIPCHandles(Interface* self);
 
-/**
+/// Checks for user input updates
- * Sets a Pad state (button or button combo) as pressed
+void HIDUpdate();
- * @param pad_state PadState data indicating which buttons have been pressed
- */
-void PadButtonPress(const PadState& pad_state);
-
-/**
- * Sets a Pad state (button or button combo) as released
- * @param pad_state PadState data indicating which buttons have been released
- */
-void PadButtonRelease(const PadState& pad_state);
-
-/**
- * Called after all Pad changes to be included in this update have been made, including both Pad
- * key changes and analog circle Pad changes.
- */
-void PadUpdateComplete();
-
-/**
- * Signal that the touchpad has been pressed
- * @param x Touchpad x-coordinate in bottom screen pixels (between 0 and 320)
- * @param y Touchpad y-coordinate in bottom screen pixels (between 0 and 240)
- */
-void TouchPress(u16 x, u16 y);
-
-/// Signal that touchpad has been released
-void TouchRelease();
-
-/// Signal that touchpad updates have been completed
-void TouchUpdateComplete();
 
+/// Initialize HID service
 void HIDInit();
+
+/// Shutdown HID service
 void HIDShutdown();
 
 }

src/core/hw/gpu.cpp

@@ -14,6 +14,7 @@
 #include "core/hle/hle.h"
 #include "core/hle/service/gsp_gpu.h"
 #include "core/hle/service/dsp_dsp.h"
+#include "core/hle/service/hid/hid.h"
 
 #include "core/hw/gpu.h"
 
@@ -294,6 +295,9 @@ static void VBlankCallback(u64 userdata, int cycles_late) {
     // this. Certain games expect this to be periodically signaled.
     DSP_DSP::SignalInterrupt();
 
+    // Check for user input updates
+    Service::HID::HIDUpdate();
+
     // Reschedule recurrent event
     CoreTiming::ScheduleEvent(frame_ticks - cycles_late, vblank_event);
 }