[games] Mouse support #67

fetzerch · 2016-10-28T23:01:57Z

Adds mouse support for Emulators like ScummVM or Dosbox. Fixes #6

Related changes:

kodi: PR - retroplayer-mouse
game.libretro: PR - mouse
kodi-game-controllers: PR - mouse
game.libretro.scummvm: PR - mouse
game.libretro.dosbox: PR - mouse

Known issues:

Enable Joystick & Mouse at the same time (fixed by 4b3117e)
Dosbox crashes (fixed array of 0-15 ports, we call it with -2 (GAME_INPUT_PORT_MOUSE)) (fixed by kodi-game/game.libretro@c62477c)
Mouse button mapping doen't work (clicks are ignored in mapping dialog)

garbear · 2016-10-29T06:54:09Z

xbmc/input/InputManager.cpp

+      static int m_mouseStateY;
+      for (std::vector<MOUSE::IMouseHandler*>::iterator it = m_mouseHandlers.begin(); it != m_mouseHandlers.end(); ++it)
+      {
+        if ((*it)->OnMouseMotion(newEvent.motion.x - m_mouseStateX, newEvent.motion.y - m_mouseStateY))


The XBMC_Event struct has fields for relative coordinates, why don't we use those? If we are given relative coords, and libretro requires relative coords, there shouldn't be any need to ever touch absolute positions. It should be possible to transfer dx and dy to libretro without any addition or subtraction.

I couldn't find anthing that stores relative coordinates. there's xrel/yrel, but that's a different thing. From what I can see x/y are the absolute coordinate inside kodi window, xrel/yrel are the absolute coordinates relative to the screen 0,0 position. If kodi is running in fullscreen mode then they are the same. There are no other values set in https://github.com/xbmc/xbmc/blob/master/xbmc/windowing/WinEventsX11.cpp#L546. I could only find the relative position being calculated in MouseStat: https://github.com/xbmc/xbmc/blob/master/xbmc/input/MouseStat.cpp#L52.

OK, not a problem. Add a second mouse motion to IMouseHandler interface for absolute positions. Then create a IMouseHandler to wrap CGameClientMouse that accepts absolute coordinates and translates them to relative coordinates. The handler should be placed in xbmc/input/mouse/generic because this is a class that describes generic behavior of a mouse. Our goal is to isolate all mouse handling logic so that it doesn't become entangled with any other dependencies.

garbear · 2016-10-29T07:05:21Z

xbmc/input/InputManager.cpp

+            handled = true;
+        }
+      }
+    }


can you refactor the above code to only use 1 for loop?

fetzerch · 2016-10-29T15:22:23Z

xbmc/games/addons/GameClientMouse.cpp

+  game_input_event event;
+
+  event.type                   = GAME_INPUT_EVENT_DIGITAL_BUTTON;
+  event.port                   = -1;


Here i'm not sure if that's the correct event type / port. Because the code in https://github.com/kodi-game/game.libretro/pull/7/files#diff-97dc480240fc0f2c40a1732397d7adecR148 feels like a hack now. (checking for -1)

yup. total hack and something we should avoid

garbear · 2016-10-29T16:15:16Z

I've rethought our approach. Let's create mice controller profiles that can be mapped in the controller dialog.

Then we add OpenMouse() callback to the Game API similar to OpenPort(). That way, DOS can capture both the mouse and 2 controller ports. We can add #defines for GAME_PORT_MOUSE and GAME_PORT_KEYBOARD to identify if it is mouse, joystick or keyboard input.

garbear · 2016-10-29T16:20:02Z

Don't add OpenMouse() to the game API, i realized that mouse support is only specified in addon.xml so we should open the mouse in CGameClient like we're doing now. But add the port #defines to detect mouse or joystick. Then you can use the port as a key in CLibretroInputDevice.

garbear · 2016-10-29T18:58:28Z

It's a little weird having a mouse in controller configuration, we should probably modify the controller dialog to be able to show only controllers or only the mouse

fetzerch · 2016-10-30T19:22:40Z

Mouse movement is working again (with https://github.com/fetzerch/game.libretro.scummvm/tree/mouse, and https://github.com/fetzerch/game.libretro/tree/mouse). Couldn't yet get the buttons to work. They seem to be registered in game.libretro LibretroDeviceInput and also buttons are queried by the core but they don't trigger any action it seems.

I'll do the refactoring of xbmc/input/InputManager.cpp once it's working again.

One thing I noticed also is that only the first matching controller mapping is taken into account, hence I think that the joypad is no longer functioning. Not sure if there's a quick workaround.

Btw: In case you also want to try this, just load scummvm in standalone mode. It has the ingame menu where you can use the mouse already without any games registered.

garbear · 2016-10-31T07:54:51Z

@fetzerch I sent you a fix that refactors some stuff out of CInputManager

garbear · 2016-11-01T16:31:52Z

xbmc/input/InputManager.cpp

 }

 void CInputManager::UnregisterMouseHandler(MOUSE::IMouseInputHandler* handler)
 {
-  auto it = m_mouseHandlers.find(handler);
+  auto it = std::find_if(m_mouseHandlers.begin(), m_mouseHandlers.end(),
+    [handler](const std::pair<MOUSE::IMouseInputHandler*, std::unique_ptr<MOUSE::IMouseDriverHandler>>& element) { return element.first == handler; });


Can you break this up into multiple lines?

auto it = std::find_if(m_mouseHandlers.begin(), m_mouseHandlers.end(), [handler](const std::pair<MOUSE::IMouseInputHandler*, std::unique_ptr<MOUSE::IMouseDriverHandler>>& element) { return element.first == handler; });

garbear · 2016-11-01T16:33:23Z

xbmc/input/InputManager.cpp

  if (it == m_mouseHandlers.end())
-    m_mouseHandlers[handler].reset(new MOUSE::CMouseInputHandling(handler, m_mouseButtonMap.get()));
+    m_mouseHandlers.emplace_back(std::make_pair(handler, std::unique_ptr<MOUSE::IMouseDriverHandler>(new MOUSE::CMouseInputHandling(handler, m_mouseButtonMap.get()))));


Do we want mouse handlers to be processed in the same order as RegisterMouseHandler() is called, or should priority be given to the most-recently registered mouse handler?

I'd find it easier to understand if they are called in the registered order. If there's a reason for doing it the other way, we can change it.

garbear · 2016-11-01T16:35:44Z

xbmc/input/InputManager.h

@@ -284,7 +284,7 @@ class CInputManager : public ISettingCallback
  CCriticalSection     m_actionMutex;

  std::vector<KEYBOARD::IKeyboardHandler*> m_keyboardHandlers;
-  std::map<MOUSE::IMouseInputHandler*, std::unique_ptr<MOUSE::IMouseDriverHandler>> m_mouseHandlers;
+  std::vector<std::pair<MOUSE::IMouseInputHandler*, std::unique_ptr<MOUSE::IMouseDriverHandler>>> m_mouseHandlers;


I suggest:

typedef std::pair<MOUSE::IMouseInputHandler*, std::unique_ptr<MOUSE::IMouseDriverHandler>> MouseHandlerHandle; std::vector<MouseHandlerHandle> m_mouseHandlers;

garbear · 2016-11-01T16:38:26Z

xbmc/games/addons/GameClient.cpp

@@ -965,10 +965,12 @@ void CGameClient::CloseKeyboard(void)
 void CGameClient::OpenMouse(void)
 {
  m_mouse.reset(new CGameClientMouse(this, m_pStruct));
+  OpenPort(GAME_INPUT_PORT_MOUSE);


This opens a joystick handler on the mouse port

garbear · 2016-11-01T18:09:54Z

Can you link your PR description to all the other open PRs that have to do with mouse support? xbmc, game.libretro, game.libretro.scummvm, kodi-game-controllers... I think we're up to 4 repos now.

fetzerch · 2016-11-01T18:35:17Z

sure, Updated PR description

garbear · 2016-11-01T18:43:28Z

xbmc/games/addons/GameClient.cpp

+
+    if (controller->LoadLayout())
+    {
+      CloseMouse();


mouse port is guaranteed to be open by now due to the check at the beginning of this function, so this line isn't needed

garbear · 2016-11-01T18:58:05Z

Mouse button mapping doen't work (clicks are ignored in mapping dialog)

We can map mouse buttons the same way we map keyboard buttons. We need a layer similar to CJoystickEmulation in xbmc/input/mouse/generic. Then we have the mouse emulate a joystick, which will allow it to be mapped via peripheral.joystick.

We can add support for mapping mouse buttons by having CGUIConfigurationWizard::InstallHooks() register itself as a mouse handler. We could even add support for inverting the x and y axis or rotating the coordinate system.

Hedda · 2016-11-02T16:24:43Z

Guess this can also be copied to or used by the Trackball in the (Xgaming) X-Arcade Tankstick.

https://shop.xgaming.com/products/x-arcade-tankstick-trackball-usb-included

Trackball inside the X-Arcade Tankstick should really work like any PS2 or USB HID mouse.

https://github.com/kodi-game/peripheral.xarcade

Know that @garbear have a X-Arcade Tankstic but not sure if he tested the Trackball?

https://en.wikipedia.org/wiki/List_of_trackball_arcade_games

Tip is above link which lists classic trackball games, most of which works in MAME emulator.

garbear · 2016-11-02T18:25:10Z

Thanks for the links, the trackball works like a mouse so it's usable with Kodi, but the resolution is really high so it goes really slow. for it to be useful in games we might need a setting for speed gain

garbear · 2016-11-02T20:22:16Z

xbmc/games/addons/GameClientMouse.h

@@ -21,6 +21,8 @@

 #include "input/mouse/IMouseInputHandler.h"

+#define MOUSE_CONTROLLER_ID "game.controller.mouse"


Is there any way we can avoid defining this here and use the value from here? https://github.com/garbear/xbmc/blob/retroplayer-mouse/xbmc/input/mouse/MouseWindowingButtonMap.cpp#L28

Kind of like how DEFAULT_CONTROLLER_ID is done, but it'd be nice if we could keep this #define inside MouseWindowingButtonMap.cpp and access it through its ControllerID() function

garbear · 2016-11-02T20:25:41Z

xbmc/games/addons/GameClientMouse.cpp

@@ -114,7 +112,7 @@ void CGameClientMouse::OnButtonRelease(const std::string& button)

  event.type                   = GAME_INPUT_EVENT_DIGITAL_BUTTON;
  event.port                   = GAME_INPUT_PORT_MOUSE;
-  event.controller_id          = CONTROLLER_NAME;
+  event.controller_id          = MOUSE_CONTROLLER_ID;
  event.feature_name           = button.c_str();


In this case, button is the name of a feature of game.controller.default, so game.controller.default serves as the "context" for this feature name. e.g. "left" is useless unless we know it's "left" for game.controller.mouse.

As I mentioned in my comment below, it'd be nice if the controller ID could come from the same source as the button name (CMouseWindowingButtonMap)

Create a static MouseControllerID() function in CMouseWindowingButtonMap, or have a default constructed instance of CMouseWindowingButtonMap in CGameClientMouse?

both are undesirable. we need to pass the controller context to this class somehow. See CGameClientInput::ControllerID() and notice how the context comes from CGameClient. CGameClientInput::ControllerID() is then used to transfer the context to the port manager, which opens a port using that controller ID.

How about adding a return value to CInputManager::RegisterMouseHandler and let InputManager return m_mouseButtonMap->ControllerID(). That way we can easily keep CGameClientMouse independent from the button map. A getter would also do, but since we call register anyway, that's much cleaner.

This could work. Give it a try, and I'll see if it leads to any glaring problems.

Hedda · 2016-11-03T12:30:43Z

@garbear is it the same for mouse speed at different resolutions in all emulators or?

So on a 2160p (4K) display it might even be slower then.

Would then be nice if could set different default speed for mouse on different resolutions.

If detect a 240p display then it runs mouse at 1x speed by default.
If detect a 320p display then it runs mouse at 1,5x speed by default.
If detect a 480p/576p display then it runs mouse at 2x speed by default.
If detect a 720p display then it runs mouse at 3x speed by default.
If detect a 1080p display then it runs mouse at 4x speed by default.
If detect a 2160p display then it runs mouse at 8x speed by default.

garbear · 2016-11-03T18:22:32Z

@fetzerch what's left before we merge? do you want to add support for mapping the mouse and emulating the mouse with a joystick, or merge this first and add that later?

fetzerch · 2016-11-03T18:38:18Z

Wanted to ask you ;) For this PR, just the MOUSE_CONTROLLER_ID. Haven't yet found a nice way of doing it. If we pass in the controller when creating creating the gameclientmouse, we could access it from there. But we need to access it also when loading the addon and have to get it from buttonmap (which is created in inputmanager).

Mouse mapping, I'd say we do separately. (Is there an option to disable it in the dialog for the moment?)

For game.libretro, we have open the thing with the port number and the logging of the libretro key.

Then for the addons it's just removing the game.controller.default. I think we don't need that explicitly now that we load the mouse controller explicitly.

garbear · 2016-11-03T19:20:27Z

For this PR, just the MOUSE_CONTROLLER_ID

I guess for now we should just copy the default controller thing, define the mouse ID in MouseWindowingButtonMap.h, add this line to GameClient.cpp:

#include "input/mouse/MouseWindowingButtonMap.h" // for MOUSE_CONTROLLER_ID

then pass the ID to CGameClientMouse.

Is there an option to disable it in the dialog for the moment?

For now we can simply not ship game.controller.mouse. Then it won't show up in the GUI, but it'll still be usable because the Kodi -> mouse translations are hardcoded in MouseWindowingButtonMap.cpp.

fetzerch · 2016-11-03T20:19:53Z

Updated. If we don't ship the addon, it'll not work because in OpenMouse we need to call UpdatePort so that game.libretro gets informed.

fetzerch · 2016-11-03T20:20:36Z

Let me know if you want me to squash the changes

garbear · 2016-11-03T20:31:49Z

xbmc/input/InputManager.h

+  /*! \brief Registers a handler to be called on mouse input (e.g a game client).
+   *
+   * \param handler The handler to call on mouse input.
+   * \return[in] The controller ID of the button map used to translate from windowing


I would say \return The controller ID that serves as a context for incoming events

garbear · 2016-11-03T20:37:40Z

Yes, squash the changes and i'll do a review of the full patch

garbear · 2016-11-04T07:06:37Z

The changes here look good. I sent you a PR for build system fixes for xcode and autotools.

Game API v1.0.28: - Define keyboard and mouse ports - Use XBMCVKey for keyboard keys - Add XBMC_vkeys.h to addon bindings

garbear · 2016-11-05T13:51:12Z

Is that it? What's left for this PR?

fetzerch · 2016-11-05T13:52:57Z

I'll test why buttons are not working anymore, then i'll squash both PRs and push another version.

fetzerch · 2016-11-05T14:30:41Z

PRs updated. dosbox and scummvm work with mouse and joystick.

Still haven't fully figured out how game.controller.kodi is supposed to come into play. but maybe we can do this independently.

garbear · 2016-11-05T14:38:49Z

you mean game.controller.mouse? it's needed for button mapping, but it's ok that it's missing now because the feature names are hardcoded in MouseWindowingButtonMap.cpp

Ready for merge across the PRs?

fetzerch · 2016-11-05T14:43:29Z

from my side, yes. everything tested and pushed

garbear · 2016-11-05T15:17:36Z

xbmc/games/addons/GameClient.cpp

-  for (unsigned int i = 0; i < m_ports.size(); i++)
-    ClosePort(i);
+  for (const auto &it : m_ports)
+    ClosePort(it.first);


ClosePort() modifies m_ports, which i think invalidates the iterator. I'll fix once i merge this

this should do it: e762427

[addons] add initial base add-on C++ support

fetzerch mentioned this pull request Oct 28, 2016

Mouse support kodi-game/game.libretro#7

Merged

garbear reviewed Oct 29, 2016

View reviewed changes

xbmc/input/InputManager.cpp

handled = true;

}

}

}

Copy link

Owner

garbear Oct 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you refactor the above code to only use 1 for loop?

fetzerch commented Oct 29, 2016

View reviewed changes

garbear mentioned this pull request Oct 29, 2016

Add mouse button map kodi-game/game.libretro.scummvm#4

Closed

garbear mentioned this pull request Oct 31, 2016

Isolate mouse logic and button translation fetzerch/xbmc#9

Merged

fetzerch force-pushed the retroplayer-mouse branch from fbab078 to 713bdf4 Compare October 31, 2016 09:52

garbear reviewed Nov 1, 2016

View reviewed changes

fetzerch mentioned this pull request Nov 1, 2016

Add mouse support kodi-game/game.libretro.dosbox#2

Merged

Hedda mentioned this pull request Nov 2, 2016

Trackball (mouse) support implemented for X-Arcade Tankstick? kodi-game/peripheral.xarcade#1

Open

garbear reviewed Nov 2, 2016

View reviewed changes

garbear reviewed Nov 3, 2016

View reviewed changes

fetzerch force-pushed the retroplayer-mouse branch from ddc6645 to d024329 Compare November 3, 2016 20:49

garbear mentioned this pull request Nov 4, 2016

Sync Xcode and Autotools projects fetzerch/xbmc#10

Merged

garbear mentioned this pull request Nov 4, 2016

Remove code that treats mouse like joystick fetzerch/xbmc#11

Merged

fetzerch force-pushed the retroplayer-mouse branch from 9c309ed to 21964af Compare November 4, 2016 21:27

garbear mentioned this pull request Nov 5, 2016

Update game api to use signed port in UpdatePort() fetzerch/xbmc#12

Merged

[games] Mouse support

d002851

Game API v1.0.28: - Define keyboard and mouse ports - Use XBMCVKey for keyboard keys - Add XBMC_vkeys.h to addon bindings

fetzerch force-pushed the retroplayer-mouse branch from 850ea8e to d002851 Compare November 5, 2016 14:21

garbear reviewed Nov 5, 2016

View reviewed changes

garbear merged commit 4aef902 into garbear:retroplayer-17beta4 Nov 5, 2016

garbear mentioned this pull request Nov 5, 2016

Mouse support for game add-ons #66

Closed

fetzerch deleted the retroplayer-mouse branch November 6, 2016 13:31

garbear pushed a commit that referenced this pull request Jan 4, 2017

Merge pull request #67 from AlwinEsch/add_instance_ptr2

2115a52

[addons] add initial base add-on C++ support

		@@ -21,6 +21,8 @@

		#include "input/mouse/IMouseInputHandler.h"

		#define MOUSE_CONTROLLER_ID "game.controller.mouse"

[games] Mouse support #67

[games] Mouse support #67

Conversation

fetzerch commented Oct 28, 2016 • edited by garbear Loading

garbear Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fetzerch Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbear commented Oct 29, 2016 • edited Loading

garbear commented Oct 29, 2016

garbear commented Oct 29, 2016

fetzerch commented Oct 30, 2016

garbear commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbear commented Nov 1, 2016

fetzerch commented Nov 1, 2016

Choose a reason for hiding this comment

garbear commented Nov 1, 2016

Hedda commented Nov 2, 2016

garbear commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fetzerch Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

garbear Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hedda commented Nov 3, 2016 • edited Loading

garbear commented Nov 3, 2016

fetzerch commented Nov 3, 2016 • edited Loading

garbear commented Nov 3, 2016

fetzerch commented Nov 3, 2016

fetzerch commented Nov 3, 2016

Choose a reason for hiding this comment

garbear commented Nov 3, 2016

garbear commented Nov 4, 2016

garbear commented Nov 5, 2016

fetzerch commented Nov 5, 2016

fetzerch commented Nov 5, 2016

garbear commented Nov 5, 2016

fetzerch commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fetzerch commented Oct 28, 2016 •

edited by garbear

Loading

garbear Oct 29, 2016 •

edited

Loading

fetzerch Oct 29, 2016 •

edited

Loading

garbear commented Oct 29, 2016 •

edited

Loading

fetzerch Nov 2, 2016 •

edited

Loading

garbear Nov 2, 2016 •

edited

Loading

Hedda commented Nov 3, 2016 •

edited

Loading

fetzerch commented Nov 3, 2016 •

edited

Loading