Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle touch input #4310

Merged
merged 22 commits into from Oct 9, 2018

Conversation

@NeatNit
Copy link
Contributor

NeatNit commented Oct 6, 2018

Fixes #1437.

In both SDL and Qt, this will fix how Citra behaves when it's interacted with using a touchscreen.

In Qt only, using multiple fingers (multi-touch) will make the average position of them be sent to the emulation, making it behave exactly like a real 3DS. In SDL, this is trickier to implement, so what happens instead is that the last finger to move is detected. This doesn't really matter, because users shouldn't be using multi-touch in the first place.

In the process, it improves some mouse input code (DRY), and fixes a minor bug where dragging the mouse from the touchscreen to the left of the emulator window or above it would make the touch be registered at the right or bottom of the touchscreen.

I've tested on a Windows 10 laptop with a built-in touchscreen.

This is my first time using C++ so I'm all ears for what can be done better!


This change is Reviewable

@NeatNit NeatNit force-pushed the NeatNit:touch_input branch from 45ab3cd to f0faaf0 Oct 6, 2018

Show resolved Hide resolved src/citra/emu_window/emu_window_sdl2.cpp Outdated
Show resolved Hide resolved src/citra/emu_window/emu_window_sdl2.cpp Outdated
Show resolved Hide resolved src/citra/emu_window/emu_window_sdl2.cpp Outdated
int px, py;
TouchToPixelPos(x, y, &px, &py);

TouchMoved((unsigned)std::max(px, 0), (unsigned)std::max(py, 0));

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

static_cast

// 3DS does

int px, py;
TouchToPixelPos(x, y, &px, &py);

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

This can be changed to:

const auto [px, py] = TouchToPixelPos(x, y);

If the return value is changed to a std::pair like indicated above.

if (event->button() == Qt::LeftButton)
this->TouchReleased();
else if (event->button() == Qt::RightButton)
InputCommon::GetMotionEmu()->EndTilt();
}

void GRenderWindow::touchBeginEvent(QTouchEvent* event) {

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

The parameter can be a const pointer.

auto pos = tp.pos();

// Copied from mousePressEvent:
qreal pixelRatio = windowPixelRatio();

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

pixel_ratio also this can be const.

auto tp = points.first(); // there should only be 1 point in TouchBegin
auto pos = tp.pos();

// Copied from mousePressEvent:

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

This should probably be put in its own member function like:

void ScaledTouch(const QTouchEvent::TouchPoint& point) {
    const qreal pixel_ratio = windowPixelRatio();
    const auto pos = point.pos();

    TouchPressed(static_cast<u32>(pos.x() * pixel_ratio),
                 static_cast<u32>(pos.y() * pixel_ratio));
}

and then simply use that in the several places it's required, instead of copying it verbatim into places.

This comment has been minimized.

Copy link
@NeatNit

NeatNit Oct 6, 2018

Author Contributor

oh. yes.

issue is it's used in mouse events as well, and what's more, touch produces float QPointF while mouse produces int QPoint.

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

This is trivially resolvable with something along the lines of:

template <typename T>
void ScaledTouch(const T& point) {
    static_assert(std::is_same_v<T, QTouchEvent::TouchPoint> ||
                  std::is_same_v<T, QMouseEvent>);

    const qreal pixel_ratio = windowPixelRatio();
    const auto pos = point.pos();

    TouchPressed(static_cast<u32>(pos.x() * pixel_ratio),
                 static_cast<u32>(pos.y() * pixel_ratio));
}

Where auto will deduce the type automatically, depending on the T given.

static_cast<unsigned>(pos.y() * pixelRatio));
}

void GRenderWindow::touchUpdateEvent(QTouchEvent* event) {

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

event can be a const pointer.

std::max(static_cast<unsigned>(y * pixelRatio), 0u));
}

void GRenderWindow::touchEndEvent(QTouchEvent* event) {

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

This can be a const pointer, or be removed, since it's not used.

@@ -130,6 +131,12 @@ class GRenderWindow : public QWidget, public EmuWindow {
void mouseMoveEvent(QMouseEvent* event) override;
void mouseReleaseEvent(QMouseEvent* event) override;

void touchBeginEvent(QTouchEvent* event);

This comment has been minimized.

Copy link
@jroweboy

jroweboy Oct 6, 2018

Member

Style: capitalize the first letter of methods that you add that aren't overriding a base class

@NeatNit

This comment has been minimized.

Copy link
Contributor Author

NeatNit commented Oct 6, 2018

The original pull request had some review that got destroyed. I got at least some of them in an email, so here they are (formatting has gone to shit - next morning edit: fixed formatting):

@zhaowenlan1779 commented on this pull request.


In src/citra/emu_window/emu_window_sdl2.cpp: DONE

@@ -41,6 +41,39 @@ void EmuWindow\_SDL2::OnMouseButton(u32 button, u8 state, s32 x, s32 y) {
     }
 }
 
+void EmuWindow\_SDL2::TouchToPixelPos(float touch\_x, float touch\_y, int\* pixel\_x, int\* pixel\_y) {

Avoid using pointers.
Either:

std::tuple<int, int> EmuWindow_SDL2::TouchToPixelPos(float touch_x, float touch_y)

and return a {..., ...}.
In this way, in OnFingerDown and OnFingerMotion you can just do

auto [px, py] = TouchToPixelPos(x, y);

or make pixel_x and pixel_y references (int&).


In src/citra/emu_window/emu_window_sdl2.cpp: DONE

@@ -41,6 +41,39 @@ void EmuWindow\_SDL2::OnMouseButton(u32 button, u8 state, s32 x, s32 y) {
     }
 }
 
+void EmuWindow\_SDL2::TouchToPixelPos(float touch\_x, float touch\_y, int\* pixel\_x, int\* pixel\_y) {
+    int w, h;
+    SDL\_GetWindowSize(render\_window, &w, &h);
+
+    touch\_x \*= w;
+    touch\_y \*= h;
+
+    \*pixel\_x = (int)(touch\_x + 0.5);
+    \*pixel\_y = (int)(touch\_y + 0.5);

Use static_cast<int>(...).
To make the logic clearer, you could use std::ceil


In src/citra_qt/bootmanager.cpp: DONE (i.e. changed such that it's no longer relevant)

     if (event->button() == Qt::LeftButton)
         this->TouchReleased();
     else if (event->button() == Qt::RightButton)
         InputCommon::GetMotionEmu()->EndTilt();
 }
 
+void GRenderWindow::touchBeginEvent(QTouchEvent\* event) {
+    auto points = event->touchPoints();
+    auto tp = points.first(); // there should only be 1 point in TouchBegin
+    auto pos = tp.pos();
+
+    // Copied from mousePressEvent:
+    qreal pixelRatio = windowPixelRatio();
+    this->TouchPressed(static\_cast<unsigned>(pos.x() \* pixelRatio),
+                       static\_cast<unsigned>(pos.y() \* pixelRatio));
+}
+
+void GRenderWindow::touchUpdateEvent(QTouchEvent\* event) {
+    qreal x = 0.0, y = 0.0;

why not just double?
Also, you can just make them = 0.


In src/citra_qt/bootmanager.cpp: DONE

+    auto tp = points.first(); // there should only be 1 point in TouchBegin
+    auto pos = tp.pos();
+
+    // Copied from mousePressEvent:
+    qreal pixelRatio = windowPixelRatio();
+    this->TouchPressed(static\_cast<unsigned>(pos.x() \* pixelRatio),
+                       static\_cast<unsigned>(pos.y() \* pixelRatio));
+}
+
+void GRenderWindow::touchUpdateEvent(QTouchEvent\* event) {
+    qreal x = 0.0, y = 0.0;
+    int activePoints = 0;
+    auto points = event->touchPoints();
+
+    for (int i = 0; i < points.count(); i++) {
+        auto tp = points\[i\];

As far as I'm aware you can just use

for (auto tp : event->touchPoints()) {


In src/citra_qt/bootmanager.h: DONE

@@ -8,6 +8,7 @@
 #include <condition\_variable>
 #include <mutex>
 #include <QGLWidget>
+#include <QTouchEvent>

This can be probably a forward declaration as you only used it as pointer


In src/citra_qt/bootmanager.cpp: DONE

     if (event->button() == Qt::LeftButton)
         this->TouchReleased();
     else if (event->button() == Qt::RightButton)
         InputCommon::GetMotionEmu()->EndTilt();
 }
 
+void GRenderWindow::touchBeginEvent(QTouchEvent\* event) {
+    auto points = event->touchPoints();
+    auto tp = points.first(); // there should only be 1 point in TouchBegin
+    auto pos = tp.pos();
+
+    // Copied from mousePressEvent:

Please remove this comment as well as the ones below that say Copied from ...


In src/citra/emu_window/emu_window_sdl2.cpp: DONE

+    touch\_x \*= w;
+    touch\_y \*= h;
+
+    \*pixel\_x = (int)(touch\_x + 0.5);
+    \*pixel\_y = (int)(touch\_y + 0.5);
+}
+
+void EmuWindow\_SDL2::OnFingerDown(SDL\_FingerID finger, float x, float y) {
+    // To do: keep track of multitouch using the fingerID and a dictionary of some kind
+    // This isn't critical because the best we can do when we have that is to average them, like the
+    // 3DS does
+
+    int px, py;
+    TouchToPixelPos(x, y, &px, &py);
+
+    TouchPressed((unsigned)std::max(px, 0), (unsigned)std::max(py, 0));

use static_cast.
Also, I think px and py can't be negative here. Why not make them unsigned in TouchToPixelPos somehow?

NeatNit added a commit to NeatNit/citra that referenced this pull request Oct 6, 2018

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.cpp, line 44 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be modified to return a pair of the components instead of using out parameters.

std::pair<int, int> EmuWindow_SDL2::TouchToPixelPos(float touch_x, float touch_y) const {
    int w, h;
    SDL_GetWindowSize(render_window, &w, &h);

    touch_x *= w;
    touch_y *= h;

    return {static_cast<int>(touch_x + 0.5), static_cast<int>(touch_y + 0.5)};
}

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.cpp, line 61 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be changed to:

const auto [px, py] = TouchToPixelPos(x, y);

If the return value is changed to a std::pair like indicated above.

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.cpp, line 63 at r1 (raw file):

Previously, FearlessTobi (Tobias) wrote…

We should change the document :P We always use static_cast in such a case.

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.cpp, line 68 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be changed to:

const auto [px, py] = TouchToPixelPos(x, y);

If the return value is changed to a std::pair like indicated above.

Done.


src/citra/emu_window/emu_window_sdl2.cpp, line 70 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

static_cast

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra_qt/bootmanager.h, line 134 at r1 (raw file):

Previously, jroweboy (James Rowe) wrote…

Style: capitalize the first letter of methods that you add that aren't overriding a base class

Done.

NeatNit added some commits Oct 6, 2018

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.h, line 45 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be a const member function.

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash, @NeatNit, and @jroweboy)


src/citra_qt/bootmanager.cpp, line 260 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

These should be declared on their own line.

Done.


src/citra_qt/bootmanager.cpp, line 261 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

active_points

Done.


src/citra_qt/bootmanager.cpp, line 265 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be const

Done.


src/citra_qt/bootmanager.cpp, line 284 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This can be a const pointer, or be removed, since it's not used.

I feel better leaving it in to keep in line with the rest of the events.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra/emu_window/emu_window_sdl2.cpp, line 221 at r1 (raw file):

Previously, NeatNit wrote…

Does it? It's just a function call to OnMouseMotion.

Done.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 18 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra_qt/bootmanager.cpp, line 248 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

The parameter can be a const pointer.

Done.


src/citra_qt/bootmanager.cpp, line 259 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

event can be a const pointer.

Done. Did I do it right?


src/citra_qt/bootmanager.cpp, line 284 at r1 (raw file):

Previously, NeatNit wrote…

I feel better leaving it in to keep in line with the rest of the events.

Done.

static_cast<unsigned>(pos.y() * pixelRatio));
}

void GRenderWindow::TouchUpdateEvent(QTouchEvent* const event) {

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

By const pointer, I had meant const QTouchEvent*. I guess I should have been more clear with pointer-to-const instead.

if (event->button() == Qt::LeftButton)
this->TouchReleased();
else if (event->button() == Qt::RightButton)
InputCommon::GetMotionEmu()->EndTilt();
}

void GRenderWindow::TouchBeginEvent(QTouchEvent* const event) {

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

Ditto with regards to pointer-to-const.

auto tp = points.first(); // there should only be 1 point in TouchBegin
auto pos = tp.pos();

// Copied from mousePressEvent:

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 6, 2018

Member

This is trivially resolvable with something along the lines of:

template <typename T>
void ScaledTouch(const T& point) {
    static_assert(std::is_same_v<T, QTouchEvent::TouchPoint> ||
                  std::is_same_v<T, QMouseEvent>);

    const qreal pixel_ratio = windowPixelRatio();
    const auto pos = point.pos();

    TouchPressed(static_cast<u32>(pos.x() * pixel_ratio),
                 static_cast<u32>(pos.y() * pixel_ratio));
}

Where auto will deduce the type automatically, depending on the T given.

Show resolved Hide resolved src/citra_qt/bootmanager.cpp Outdated

NeatNit added some commits Oct 6, 2018

FearlessTobi added a commit to FearlessTobi/yuzu that referenced this pull request Oct 6, 2018

NeatNit added some commits Oct 6, 2018

DRY - High-DPI scaled touch put in separate function
also fixes a bug where if you start touching (with either mouse or touchscreen) and drag the mouse to the LEFT of the emulator window, the touch point jumps to the RIGHT side of the touchscreen; draggin to above the window would make it jump to the bottom.
@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 21 unresolved discussions (waiting on @lioncash, @jroweboy, and @NeatNit)


src/citra_qt/bootmanager.cpp, line 249 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

All of these variables can be const.

Done, by making it a one-liner.


src/citra_qt/bootmanager.cpp, line 253 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This is trivially resolvable with something along the lines of:

template <typename T>
void ScaledTouch(const T& point) {
    static_assert(std::is_same_v<T, QTouchEvent::TouchPoint> ||
                  std::is_same_v<T, QMouseEvent>);

    const qreal pixel_ratio = windowPixelRatio();
    const auto pos = point.pos();

    TouchPressed(static_cast<u32>(pos.x() * pixel_ratio),
                 static_cast<u32>(pos.y() * pixel_ratio));
}

Where auto will deduce the type automatically, depending on the T given.

Done, a bit differently than yours. Also, apparently QPoint can be implicitly converted to QPointF.


src/citra_qt/bootmanager.cpp, line 254 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

pixel_ratio also this can be const.

Done.


src/citra_qt/bootmanager.cpp, line 279 at r1 (raw file):

Previously, NeatNit wrote…

this was from the mouse events, should I fix it there as well? or should I actually split it off into a separate function, since it's used like 4 times?

Done.


src/citra_qt/bootmanager.cpp, line 259 at r7 (raw file):

Previously, lioncash (Mat M.) wrote…

By const pointer, I had meant const QTouchEvent*. I guess I should have been more clear with pointer-to-const instead.

Done.


src/citra_qt/bootmanager.cpp, line 248 at r8 (raw file):

Previously, lioncash (Mat M.) wrote…

Ditto with regards to pointer-to-const.

Done.


src/citra_qt/bootmanager.cpp, line 283 at r9 (raw file):

Previously, lioncash (Mat M.) wrote…

Replying to:

I feel better leaving it in to keep in line with the rest of the events.

Please don't. If it's not currently used, it should be removed; it's unnecessary. If it were a virtual function that required the parameter whether or not it was used, it would be a different story.

Done. I guess the same should apply to the unused FingerID parameters in SDL.

@NeatNit
Copy link
Contributor Author

NeatNit left a comment

Reviewable status: 0 of 4 files reviewed, 21 unresolved discussions (waiting on @lioncash and @jroweboy)


src/citra_qt/bootmanager.cpp, line 283 at r9 (raw file):

Previously, NeatNit wrote…

Done. I guess the same should apply to the unused FingerID parameters in SDL.

Done.

NeatNit added some commits Oct 6, 2018

@NeatNit

This comment has been minimized.

Copy link
Contributor Author

NeatNit commented Oct 7, 2018

Sorry for the giant mess this was. I'm pretty sure it's all good now.

sickc added a commit to sickc/citra that referenced this pull request Oct 8, 2018

@wwylele

wwylele approved these changes Oct 8, 2018

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 8, 2018

did you test that this works in and not in single window mode?

@NeatNit

This comment has been minimized.

Copy link
Contributor Author

NeatNit commented Oct 8, 2018

I did now! 😅 It works perfectly in both cases.

@B3n30

B3n30 approved these changes Oct 9, 2018

touch_x *= w;
touch_y *= h;

return {static_cast<unsigned>(std::max(std::round(touch_x), 0.0f)),

This comment has been minimized.

Copy link
@B3n30

B3n30 Oct 9, 2018

Contributor

I'm not sure, but wouldn't it be better to std::max(static_cast<unsigned>(std::round(touch_x)), 0) ?

This comment has been minimized.

Copy link
@NeatNit

NeatNit Oct 9, 2018

Author Contributor

No. This was the bug I mentioned in my edited description:

... fixes a minor bug where dragging the mouse from the touchscreen to the left of the emulator window or above it would make the touch be registered at the right or bottom of the touchscreen.

touch_x can be negative - which is the whole reason the std::max is there - but if we cast it to unsigned it will become a very large positive.

You can still see this on Citra Nightly.

@MerryMage MerryMage merged commit d6ad61c into citra-emu:master Oct 9, 2018

2 of 3 checks passed

code-review/reviewable 4 files, 23 discussions left (B3n30, FearlessTobi, jroweboy, lioncash)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bunnei added a commit to yuzu-emu/yuzu that referenced this pull request Oct 9, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Oct 10, 2018

For the record, this PR is pushed to master at 4ee914c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.