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

Input: UDP Client to provide motion and touch controls #4049

Merged
merged 2 commits into from Aug 8, 2018

Conversation

Projects
None yet
10 participants
@jroweboy
Member

jroweboy commented Aug 2, 2018

Overview

An implementation of the cemuhook motion/touch protocol, this adds the ability for users to connect several different devices to citra to send direct motion and touch data to citra. For more information on the available motion servers see https://cemuhook.sshnuke.net/padudpserver.html

Demo of the motion and touch controls using my ds4 controller

Rationale

As seen in issues like #4048, the current motion controls are rather unwieldy to use, often being to hard to figure out exactly where to move the cursor to get the kind of tilt you want. Using a real device for motion controls is simply much more intuitive and will be an all around better experience for users.

I chose to use this protocol because there really isn't many other options. It already has good device support, and since it uses UDP, it works on just about anything with a network stack and allows the server to be a remote network device even (useful for Android motion support).

Usage

To test motion controls, change the motion_device in the settings.ini to motion_device=engine:cemuhookudp and set udp_input_address and udp_input_port if needed. For touch, its a little more complicated as the input server does not provide any information about the bounding box of the screen, so you will need to figure out what the bounding box is (by using pad test) and then adding those as parameters to the param package string. As theres no visual feedback for touch location, I consider this feature pretty non functional at the moment, and only kept it around because it didn't take almost any new code to add it.

Differences between 3ds motion and cemuhookudp motion

The following image describes the motion vectors for ps3 controllers, which is the model cemuhook follows
ps3 motion controls

compared to the 3ds which looks like this

3ds motion controls

where the positive rotation values are determined by the right hand rule (simply point your right thumb in the positive accelerometer direction and then curl your hand and that is the positive direction for rotation)

This means there are the following differences:

Accelerometer: X and Z axis are inverted.
Gyro: pitch and yaw are inverted.

Code Design

Dependencies

Boost ASIO is added as a dependency for UDP support. citra-emu/ext-boost#20 must be merged first in order for this to even build. I specifically added Boost ASIO in as header only, and as long as we don't use the features that require libs, we can keep boost header only.

Protocol

Protocol defines the on-wire layout of each of the different kinds of packets that will be sent and received. Additionally it has methods to validate incoming packets, and create headers for outgoing packets. Protocol serves as a documentation of the format as well, describing what each byte means.

For a quick overview of how the protocol works, the server (a device that will provide data) will accept packets on a specific port, and waits for any of the 3 possible Request packets. As the protocol is designed to be stateless, any client can send a message to the server, and the server will send a response. There is no handshake, authentication, or any way to establish a connection.

  • PadData is the primary packet that contains the current controller state for the port that was requested. As the connection is stateless, when the server receives this packet, it will schedule itself to send PadData to the client for the next N number of seconds, after which it will timeout and stop sending updates. The client can send another PadData packet to refresh the timeout and keep the "connection" alive.
  • PortInfo is the way for the client to request updates about specific controller ports.
  • Version returns the protocol version. Not useful at the moment as theres only one version of the protocol.

Client

This file contains two classes, Client and Socket. Socket is designed to be an implementation detail of the client, and does the actual wire communication to the configured address and port. Client uses Protocol read and write data to the Socket and also writes the latest pad data into a DeviceStatus struct, which will be read from by the MotionDevice and the TouchDevice in order to pass the data to HID. The Socket is running on a new thread so it won't block anything related to emulation, and is hardcoded to send PortInfo and PadData requests every 3 seconds to keep the servers sending data.

UDP

The external interface for the whole system, which includes registering the InputFactorys and creating the InputDevices. When these devices are polled by HID, it will just read the internal client's DeviceStatus and return the most up-to-date value. The interface hides all implementation details behind a single State class. Currently, there isn't any good place to store this State, so its in a global static variable, but when the InputCommon::Init is rewritten to remove global state, this class is already ready for the rewrite

Not implemented

  • No UI. @zhaowenlan1779 said he was working on a UI, so I decided I should get the core code merged so the UI can be reviewed independently
  • No visual feedback for touch events. This can come in a separate PR if someone choose to add that
  • No sensitivity controls. The server will likely have sensitivity controls, and so I don't think its needed here (at least until we get enough complaints)

Acknowledgements

Super thanks and shoutouts to @rajkosto for their work on the protocol and the different servers for it as well. This client implementation was based off reading their open server ds4 server and some wireshark dumps of pad test <-> ds4windows communication.


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Aug 2, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-02T16:54:03Z: 5bf87e9...jroweboy:fbc741d92ac2f03f7339fb2b99c265073a77c4f8

@jroweboy

This comment has been minimized.

Member

jroweboy commented Aug 2, 2018

Reminder, this can't go into canary until the ext-boost pr is merged

@wwylele

Can we name it "Cemuhook/CemuhookUDP" instead of generic "UDP"? Because there can be other implementation also using UDP as the base protocol.

@@ -75,6 +75,10 @@ void Config::ReadValues() {
Settings::values.touch_device =
qt_config->value("touch_device", "engine:emu_window").toString().toStdString();
Settings::values.udp_input_address =
qt_config->value("udp_input_address", "127.0.0.1").toString().toStdString();
Settings::values.udp_input_port = qt_config->value("udp_input_port", 26760).toInt();

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

might need static_cast<u16> to silence msvc warning?

$<$<BOOL:${SDL2_FOUND}>:sdl/sdl.cpp sdl/sdl.h>
)
create_target_directory_groups(input_common)
target_link_libraries(input_common PUBLIC core PRIVATE common)
target_compile_definitions(input_common PRIVATE BOOST_ERROR_CODE_HEADER_ONLY BOOST_SYSTEM_NO_LIB BOOST_DATE_TIME_NO_LIB BOOST_REGEX_NO_LIB)

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

This probably should be promoted to the top level CMakeLists to have effect on all project components, so code in other places won't misuse it.

This comment has been minimized.

@jroweboy

jroweboy Aug 3, 2018

Member

Not sure which top level you are thinking of? Core or the two frontends?

This comment has been minimized.

@wwylele

wwylele Aug 3, 2018

Member

@jroweboy the project main CMakeLists.txt

public:
using clock = std::chrono::system_clock;
explicit Socket(const std::string host, const u16 port, const u32 client_id,

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

const std::string&, and do the other two parameters need to be const?

SocketCallback callback)
: client_id(client_id), io_service(), timer(io_service),
send_endpoint(udp::endpoint(address_v4::from_string(host), port)), receive_endpoint(),
socket(io_service, udp::endpoint(udp::v4(), 0)), callback(callback) {}

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

callback(std::move(callback))

auto pad_message = Request::Create(pad_data, client_id);
std::memcpy(send_buffer2.data(), &pad_message, PAD_DATA_SIZE);
size_t len2 = socket.send_to(boost::asio::buffer(send_buffer2), send_endpoint);
StartSend(timer.expires_at());

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

Boost doc says

(Deprecated: Use expiry().) Get the timer's expiry time as an absolute time.

Client::~Client() {
socket->Stop();
if (thread.joinable()) {

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

I don't see anywhere this object gets reset so I think there is no need to check joinable?

This comment has been minimized.

@jroweboy

jroweboy Aug 3, 2018

Member

I don't see much of a reason to not check if joinable before either /shrug

This comment has been minimized.

@wwylele

wwylele Aug 3, 2018

Member

@jroweboy checking it shows you are not confident with your code. It is like saying "well I might have made mistake somewhere and let's just pretend that didn't happen". And it makes the code less self-documenting: I have to go over the entire code when seeing this line, wondering where it gets destroyed.

// TODO: add a setting for invert axis
status->motion_status = {accel, gyro};
// TODO: add a setting for "click" touch

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

Could you elaborate what this mean? Is it that a click input on the device is sent in a different way that is not handled here?

This comment has been minimized.

@jroweboy

jroweboy Aug 3, 2018

Member

I'll update the comment. Its referring to how some devices can support tapping the screen and "clicking" the screen with a hard push on the screen.

static constexpr u32 CLIENT_MAGIC = 0x43555344; // DSUC (but flipped for LE)
static constexpr u32 SERVER_MAGIC = 0x53555344; // DSUS (but flipped for LE)
enum class Type : u32_le {

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

_le types don't really make sense as enum underlying type. On a BE machine this would simply fail to compile.

namespace Polling {
void GetPollers(InputCommon::Polling::DeviceType type,
std::vector<std::unique_ptr<InputCommon::Polling::DevicePoller>>& pollers) {}

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

Is this meant to be empty or you are going to implement in the future?

  • If this is left empty what's the point of this function and appending the result to the poller list?
  • If you are implementing this later, please add a TODO
namespace Polling {
/// Get all DevicePoller that use the SDL backend for a specific device type

This comment has been minimized.

@wwylele

wwylele Aug 2, 2018

Member

SDL cemuhook UDP

// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.
#include <algorithm>

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

<cstring> and <functional> as well

void StartReceive() {
socket.async_receive_from(boost::asio::buffer(receive_buffer), receive_endpoint,
boost::bind(&Socket::HandleReceive, this,

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

This can likely use a regular lambda which allows more optimization opportunities than bind.

[this](const boost::system::error_code& error, std::size_t bytes_transferred) {
  HandleReceive(error, bytes_transferred);
});
void StartSend(const clock::time_point& from) {
timer.expires_at(from + std::chrono::seconds(3));
timer.async_wait(boost::bind(&Socket::HandleSend, this, boost::asio::placeholders::error));

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

This can likely use a lambda like:

timer.async_wait([this](const boost::system::error_code& error) {
    HandleSend(error);
});
class Client {
public:
explicit Client(std::shared_ptr<DeviceStatus> status, const std::string host = "127.0.0.1",
const u16 port = 26760, const u32 client_id = 24872);

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

These const aren't necessary in the prototype (but can be left in the definition).

class Socket;
namespace Response {
struct Version;

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

These forward decls should be organized alphabetically

class UDPMotionDevice final : public Input::MotionDevice {
public:
explicit UDPMotionDevice(std::shared_ptr<DeviceStatus> status) : status(status) {}

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

Ditto

class UDPTouchFactory final : public Input::Factory<Input::TouchDevice> {
public:
explicit UDPTouchFactory(std::shared_ptr<DeviceStatus> status) : status(status) {}

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

Ditto

class UDPMotionFactory final : public Input::Factory<Input::MotionDevice> {
public:
explicit UDPMotionFactory(std::shared_ptr<DeviceStatus> status) : status(status) {}

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

Ditto

Client::Client(std::shared_ptr<DeviceStatus> status, const std::string host, const u16 port,
const u32 client_id)
: status(status) {
SocketCallback callback{std::bind(&Client::OnVersion, this, std::placeholders::_1),

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member
SocketCallback callback{[this](Response::Version version) { OnVersion(version); },
                        [this](Response::PortInfo info) { OnPortInfo(info); },
                        [this](Response::PadData data) { OnPadData(info); }};
explicit Socket(const std::string host, const u16 port, const u32 client_id,
SocketCallback callback)
: client_id(client_id), io_service(), timer(io_service),

This comment has been minimized.

@lioncash

lioncash Aug 2, 2018

Member

io_service() and receive_endpoint() can be omitted from the initializer list.

@@ -8,6 +8,8 @@
#include "input_common/keyboard.h"
#include "input_common/main.h"
#include "input_common/motion_emu.h"
#include "input_common/udp/udp.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

extra newline?
Someone told me we don't separate includes.

public:
using clock = std::chrono::system_clock;
explicit Socket(const std::string host, const u16 port, const u32 client_id,

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

explicit is not needed here, as you have more than one parameter

This comment has been minimized.

@lioncash

lioncash Aug 3, 2018

Member

It would still apply here as C++11 expanded the rule regarding converting constructors to just be any constructor that doesn't contain an explicit specifier (for an up to date location, see 15.3.1 paragraph 1 in the C++17 standard). The single parameter case is still easily the most error prone however (in my experience anyway)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

Oh, I see that. fine then.

{
std::lock_guard<std::mutex> guard(status->update_mutex);
// TODO: add a setting for invert axis

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

I don't think this is needed.
Most servers should have this option implemented

Client::Client(std::shared_ptr<DeviceStatus> status, const std::string host, const u16 port,
const u32 client_id)
: status(status) {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

this can be std::moved. The parameter should likely be status_ to avoid variable shadowing warning.

In short, it's a @lioncash 's ditto

@zhaowenlan1779

I had a few questions:

  1. This starts communication with the server as soon as Citra started. It does not sound very preferable, as I can see a constant 0.2 Mbps network usage even when I'm not playing any game and do not need it at all. Also, if I made the UI and allowed changing it, I would have to restart the client. I think you can just start the communication when a game is started. UPD: What's worse is it started the communication even when I did not use udp at all
  2. What do you do when the server is closed, or the network is so unstable that the connection is dropped? I can't even see a LOG_ERROR about it. I tested a bit, it's just a silent disconnection. Sorry, I was too impressed by TCP and didn't notice this is UDP.
  3. This is bad user experience - they would just find their motion control / touch stop working without any notification or indication: probably still applies
  4. How much does this impact performance? I know it is mostly async, but I think this still requires a bit of testing.
  5. How much latency is expected?
$<$<BOOL:${SDL2_FOUND}>:sdl/sdl.cpp sdl/sdl.h>
)
create_target_directory_groups(input_common)
target_link_libraries(input_common PUBLIC core PRIVATE common)
target_compile_definitions(input_common PRIVATE BOOST_ERROR_CODE_HEADER_ONLY BOOST_SYSTEM_NO_LIB BOOST_DATE_TIME_NO_LIB BOOST_REGEX_NO_LIB)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

This is somehow too long a line. (140 characters)
Can you split it into two or more lines?

// Refer to the license.txt file included.
#include <cstddef>

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

We do not split #includes

* Note: Modifies the buffer to zero out the crc (since thats the easiest way to check without
* copying the buffer)
*/
boost::optional<Type> Validate(u8* data, size_t size) {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

Sorry if I'm being dumb, but what's wrong with std::optional?

This comment has been minimized.

@lioncash

lioncash Aug 3, 2018

Member

I believe it's used because the toolchain provided by Apple with Xcode currently doesn't provide optional in the std:: namespace (it's still in the std::experimental:: namespace, ditto with variant. std::string_view is fine though, oddly enough)

This comment has been minimized.

@B3n30

B3n30 Aug 5, 2018

Contributor

i wonder if we just should map std::experimental::optional to std::optional for osx and use std::optional here?

*/
struct PadData {
enum class Flags : u8 {
ALL_PORTS,

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

TitleCase should be used.
AllPorts

"UDP Response PadData is not trivially copyable");
static_assert(sizeof(Message<PadData>) == MAX_PACKET_SIZE,
"UDP MAX_PACKET_SIZE is no longer larger than Message<PadData>");

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

The assert message is wrong?
Perhaps you should say "UDP Message<PadData> had the wrong size", or "UDP MAX_PACKET_SIZE does not match", or change the operator to <=

}
void Client::OnVersion(Response::Version data) {
LOG_TRACE(Input, "Version packet received: {}", data.version);

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

LOG_DEBUG sounds more preferable

This comment has been minimized.

@jroweboy

jroweboy Aug 3, 2018

Member

I prefer LOG_TRACE as it gets compiled out in release mode, and the server floods a ton of packets. No point in even processing them unless you are a programmer trying to debug an issue, and then you can just change log messages /shrug

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 3, 2018

Member

Fine. I just thought it may be useful and should not just be thrown away..

}
void Client::OnPortInfo(Response::PortInfo data) {
LOG_TRACE(Input, "PortInfo packet received: {}", data.model);

This comment has been minimized.

@zhaowenlan1779
}
void Client::OnPadData(Response::PadData data) {
LOG_TRACE(Input, "PadData packet received");

This comment has been minimized.

@zhaowenlan1779
@jroweboy

This comment has been minimized.

Member

jroweboy commented Aug 3, 2018

Addressing @zhaowenlan1779's questions

This starts communication with the server as soon as Citra started. It does not sound very preferable, as I can see a constant 0.2 Mbps network usage even when I'm not playing any game and do not need it at all.

As a reminder, the protocol is only a server/client in name. Theres no true connection made, meaning unless a server is actually listening on the specified address theres completely negligible network usage. The large bulk of network communication comes from the responses sent from the server and not from citra.

I'm not against starting it only when starting a Poller or when starting a game, but the current controller code doesn't really work like that. It would likely take some design work on the full input system, and so I'm calling that out of scope for this pull request, but also think this is debatable if others think I should include it here.

Also, if I made the UI and allowed changing it, I would have to restart the client. I think you can just start the connection when a game is started. UPD: What's worse is it started the communication even when I did not use udp at all

Restarting the client is as easy as resetting the State unique_ptr and calling Init again.

What do you do when the server is closed, or the network is so unstable that the connection is dropped? I can't even see a LOG_ERROR about it. I tested a bit, it's just a silent disconnection.

Once again there is no connection. For all i know they coulda turned off their controller. If thats the case then it stops working. Its also no different than what we do for regular controllers sadly >.<

This is bad user experience - they would just find their motion control / touch stop working without any notification or indication

Agreed. But its also something thats already an issue. We should instead fix it in the input common code instead of handling it on a per backend basis ;) Calling outta scope on this one.

How much does this impact performance? I know it is mostly async, but I think this still requires a bit of testing.

Should have no impact. Everything is run on two different threads. The worst case scenario is the DeviceStatus is locked on write and read, so the HID could theoretically be locked while the backend is updating, but I've done what I could to minimize the time it takes to write new data. Additionally, this is way less expensive compared to what the SDL backend is doing, so I don't think it'll be an issue in practice.

How much latency is expected?

Depends on the device. For remote connections from an android device, I'm expecting it to be noticeable but workable. For same machine connections, its basically unnoticeable in my testing.

@cluezbot

This comment has been minimized.

cluezbot commented Aug 3, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-03T04:27:25Z: 5bf87e9...jroweboy:d4114fd7433b2ad3966e9f00e7d5606a4cc7eafc

@jroweboy jroweboy added canary-merge and removed canary-merge labels Aug 3, 2018

static constexpr size_t PORT_INFO_SIZE = sizeof(Message<Request::PortInfo>);
static constexpr size_t PAD_DATA_SIZE = sizeof(Message<Request::PadData>);
std::array<u8, PORT_INFO_SIZE> send_buffer1;

This comment has been minimized.

@lioncash

lioncash Aug 3, 2018

Member

Missed this in the initial review, but perhaps these should be named send_port_buffer, and send_pad_buffer respectively to make them clearer (and prevent the off chance of a bug being introduced in the future by mistyping 1 instead of 2, or vice-versa during refactoring or something).

@legend800

This comment has been minimized.

legend800 commented Aug 3, 2018

From a functional standpoint, it seems to work great (used ROB and WarioWare Gold for testing). The only issue is that left/right tilt are swapped, so I have to face the controller the other way to have it work correctly. This is with Switch controller using BetterJoyForCemu. The most clear way of seeing this is the boss stage of the first tilt section in WWG.

2018-08-03_08_48_17-window

@jroweboy jroweboy force-pushed the jroweboy:udp-input branch from d4114fd to d09e6ec Aug 4, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Aug 4, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-04T16:29:02Z: 0233f32...jroweboy:d09e6eccb534389d9eb53126ae97acc7d09e594c

@jroweboy

This comment has been minimized.

Member

jroweboy commented Aug 4, 2018

Rebased off the latest master, and addressed the review comments again. Will look into the reverse controls at a later time

@Dartz150

This comment has been minimized.

Dartz150 commented Aug 5, 2018

I tried to test this feature in the latest canary build, but I can't seem to get it working with Android, despite this I can perfectly use this feature in Cemu (It's a true lifesaver). Maybe I'm setting my .ini file wrongly? Can someone provide me a quick copy paste of their settings? Thanks!

@jroweboy

This comment has been minimized.

Member

jroweboy commented Aug 5, 2018

@Dartz150 woulda been easier if you posted your settings and then compared imo.

motion_device\default=false
touch_device\default=false
motion_device=engine:cemuhookudp
touch_device=engine:cemuhookudp
udp_input_address=127.0.0.1
udp_input_port=26760

make sure you turn the default to false

@Dartz150

This comment has been minimized.

Dartz150 commented Aug 5, 2018

You're right, here are my settings so far:

motion_device=engine:cemuhookudp
udp_input_address=192.168.0.2
udp_input_port=26760
touch_device=engine:emu_window
motion_device\default=false
touch_device\default=true

I have the same IP address assigned in Cemu, also the padtest app shows that the motion feedback is being received correctly, maybe my problem is that I still hace the touch settings as default?. I'll try your settings once I get access to my PC, thanks!

@@ -61,12 +61,26 @@ c_stick=
# - "update_period": update period in milliseconds (default to 100)
# - "sensitivity": the coefficient converting mouse movement to tilting angle (default to 0.01)
# - "tilt_clamp": the max value of the tilt angle in degrees (default to 90)
# - "udp" reads motion input from a udp server that uses cemuhook's udp protocol

This comment has been minimized.

@valentinvanelslande

valentinvanelslande Aug 5, 2018

Contributor

cemuhookudp

motion_device=
# for touch input, the following devices are available:
# - "emu_window" (default) for emulating touch input from mouse input to the emulation window. No parameters required
# - "udp" reads touch input from a udp server that uses cemuhook's udp protocol

This comment has been minimized.

@valentinvanelslande

valentinvanelslande Aug 5, 2018

Contributor

cemuhookudp

Settings::values.udp_input_address =
ReadSetting("udp_input_address", "127.0.0.1").toString().toStdString();
Settings::values.udp_input_port =
static_cast<u16>(ReadSetting("udp_input_port", 26760).toInt());

This comment has been minimized.

@B3n30

B3n30 Aug 5, 2018

Contributor

shouldn't this some const inside the udp module

* Note: Modifies the buffer to zero out the crc (since thats the easiest way to check without
* copying the buffer)
*/
boost::optional<Type> Validate(u8* data, size_t size) {

This comment has been minimized.

@B3n30

B3n30 Aug 5, 2018

Contributor

i wonder if we just should map std::experimental::optional to std::optional for osx and use std::optional here?

static_assert(sizeof(Header) == 20, "UDP Message Header struct has wrong size");
static_assert(std::is_trivially_copyable_v<Header>, "UDP Message Header is not trivially copyable");
using MacAddress = std::array<u8, 6>;

This comment has been minimized.

@B3n30

B3n30 Aug 5, 2018

Contributor

At some point we should think about making this a common type. It's already defined 6 times within citra

This comment has been minimized.

@jroweboy

jroweboy Aug 7, 2018

Member

yeah. PRs welcome ;) i don't wanna make this one too invasive

This comment has been minimized.

@B3n30

B3n30 Aug 7, 2018

Contributor

That was my thought as well. It was just something that came into my mind while reviewing it here

@cluezbot

This comment has been minimized.

cluezbot commented Aug 7, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-07T16:01:20Z: 0233f32...jroweboy:7d5f2b4074e2362a7b128d45d42182dd95bc34c3

@cluezbot

This comment has been minimized.

cluezbot commented Aug 7, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-07T16:22:09Z: 0233f32...jroweboy:9b2032cc205aa3b9c52e25df7c605f5e5a4cfc66

@jroweboy

This comment has been minimized.

Member

jroweboy commented Aug 7, 2018

Addressed all review comments again. Fixed any differences between cemuhookudp server's coordinate system and the 3ds coordinate system. For more information about that, I've updated the original post

@legend800 Please retest and see if this fixes your reversed controls.

@B3n30

B3n30 approved these changes Aug 7, 2018

@@ -310,6 +318,8 @@ void Config::SaveValues() {
"engine:motion_emu,update_period:100,sensitivity:0.01,tilt_clamp:90.0");
WriteSetting("touch_device", QString::fromStdString(Settings::values.touch_device),
"engine:emu_window");
WriteSetting("udp_input_address", QString::fromStdString(Settings::values.udp_input_address));

This comment has been minimized.

@B3n30

B3n30 Aug 7, 2018

Contributor

WriteSetting with default value?

static_assert(sizeof(Header) == 20, "UDP Message Header struct has wrong size");
static_assert(std::is_trivially_copyable_v<Header>, "UDP Message Header is not trivially copyable");
using MacAddress = std::array<u8, 6>;

This comment has been minimized.

@B3n30

B3n30 Aug 7, 2018

Contributor

That was my thought as well. It was just something that came into my mind while reviewing it here

@cluezbot

This comment has been minimized.

cluezbot commented Aug 7, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-07T16:38:00Z: 0233f32...jroweboy:0193fc8da16f52aa7ab32f1b534424d694686da0

@legend800

This comment has been minimized.

legend800 commented Aug 7, 2018

Tilt is now correct - left/right movements = left/right in-game actions. Thanks jroweboy.

jroweboy added some commits Jan 18, 2018

Input: UDP Client to provide motion and touch controls
An implementation of the cemuhook motion/touch protocol, this adds the
ability for users to connect several different devices to citra to send
direct motion and touch data to citra.

@jroweboy jroweboy force-pushed the jroweboy:udp-input branch from 0193fc8 to 169bb29 Aug 8, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Aug 8, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-08T03:05:49Z: 1e724b0...jroweboy:169bb29a54190a373c7447e0b277e9a894be078a

@jroweboy jroweboy merged commit 9a0191d into citra-emu:master Aug 8, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jroweboy jroweboy deleted the jroweboy:udp-input branch Aug 8, 2018

@MarcoEstevez

This comment has been minimized.

MarcoEstevez commented Aug 8, 2018

First of all, big thanks for the great job everyone involved. Just for the record, tested with BetterJoyForCemu + 8bitdo motion capable controller, and it works like a charm 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment