Added MFi Controller Support #225

jamesalbert · 2018-11-13T07:25:28Z

Summary

I've written out a basic implementation for handling MFi controller events and providing Go callbacks that can be used to interact with the application.

As per CONTRIBUTING.md:

use gofmt ✅
use govet ✅
use golint ✅
test coverage for . stay at 100% ✅
test coverage for ./internal/appjs stays at 100% ✅
test coverage for ./internal/bridge stays at 100% ✅
test coverage for ./internal/html stays at 100% ✅
test coverage for ./internal/core stays at 100% ✅
try to keep consistent coding style ✅
avoid naked returns (if you deal with a part of the code that have some, please refactor). ✅ (I tried my best at least 😄)
run goreportcard with your branch, everything that is not gocyclo must be 100%. ✅

Fixes

Using GameController/GCController.h, I setup handlers for the various buttons and dpads that comply with MFi. I've tested this with the Steelseries Nimbus and all keys register with their respective button.

I wanted to add DOM events, but the contributing doc said not to get too crazy. Happy to add if this is good.

Example

This is how I've sandboxed the following changes:

package main

import (
	"github.com/murlokswarm/app"
	"github.com/murlokswarm/app/drivers/mac"
)

type MainWindow struct {
	Controller app.Controller
}

func (mw *MainWindow) Config() app.HTMLConfig {

	mw.Controller = app.NewController(app.ControllerConfig{
		OnConnected: func() {
			app.Log("OnConnected")
		},
		OnDisconnected: func() {
			app.Log("OnDisconnected")
		},
		OnPause: func() {
			app.Log("OnPause")
		},
		OnDpadChange: func(input app.ControllerInput, x float64, y float64) {
			switch input {
			case app.DirectionalPad:
				app.Logf("OnDpad -- directional pad, x: %f, y: %f", x, y)
			case app.LeftThumbstick:
				app.Logf("OnDpad -- left thumbstick, x: %f, y: %f", x, y)
			case app.RightThumbstick:
				app.Logf("OnDpad -- right thumbstick, x: %f, y: %f", x, y)
			}
		},
		OnButtonChange: func(input app.ControllerInput, value float64, pressed bool) {
			switch input {
			case app.A:
				app.Logf("OnButtonChange -- a button, value: %f, pressed: %t", value, pressed)
			case app.B:
				app.Logf("OnButtonChange -- b button, value: %f, pressed: %t", value, pressed)
			case app.X:
				app.Logf("OnButtonChange -- x button, value: %f, pressed: %t", value, pressed)
			case app.Y:
				app.Logf("OnButtonChange -- y button, value: %f, pressed: %t", value, pressed)
			case app.L1:
				app.Logf("OnButtonChange -- l1 button, value: %f, pressed: %t", value, pressed)
			case app.L2:
				app.Logf("OnButtonChange -- l2 button, value: %f, pressed: %t", value, pressed)
			case app.R1:
				app.Logf("OnButtonChange -- r1 button, value: %f, pressed: %t", value, pressed)
			case app.R2:
				app.Logf("OnButtonChange -- r2 button, value: %f, pressed: %t", value, pressed)
			}
		},
		OnClose: func() {
			app.Log("OnClose")
		},
	})
	// mw.Controller.Close()
	return app.HTMLConfig{}
}

func (mw *MainWindow) Render() string {
	return `
	<div class="MainWindow">Example</div>
	`
}

func init() {
	mw := &MainWindow{}
	app.Import(mw)
}

func main() {
	app.Run(&mac.Driver{
		URL: "/mainwindow",
	})
}

edit: updated sandbox example

maxence-charriere

Hello, this looks good! I'm amazed to see someone that pick up this as good as you did.

Just a few comments to keep the style consistent.
There is a Close call missing. It would remove the controller from the elems. (have to be done in Go and Objc).
What should happen if a controller is disconnected? Should it be closed or a controller can be reopened.
Did you take a look on what is done on other platform? Look enough generic for me but just in case.
Logs wrapper should be added too.
After logs wrapper is added, ensure test coverage is still at 100% on app and app/internal/core.

Again very good job on this!

maxence-charriere · 2018-11-13T07:30:51Z

controller.go

+// ControllerConfig is a struct that describes a controller
+type ControllerConfig struct {
+	// event handlers
+	OnControllerDpadChange   func(button string, xValue float64, yValue float64) `json:"-"`


No need to prefix each handler with controller since it already belongs to ControllerConfig: OnControllerDpadChange => OnDpadChange

Would be nice to have a description for each handlers.

xValue and yValue should be x and y

Would it make sens to have an enum or some const that map to the button?

maxence-charriere · 2018-11-13T07:35:19Z

drivers/mac/controller.go

+	id string
+
+	// event handlers
+	onControllerDpadChange   func(string, float64, float64)


same as above for the Controller prefix.

I changed all the other ones prefixed with 'controller.', but I kept the other literals the same as to keep the same convention for windows methods (e.g. OnWindow...)

this one should be changed too.
The ones with prefix kept are the one that are objc handler called outside of the controller struct.

Ah I got you now

maxence-charriere · 2018-11-13T07:39:07Z

drivers/mac/controller.go

+}
+
+// Contains satistfies the app.ElemWithCompo interface.
+func (c *Controller) Contains(compo app.Compo) bool {


This does not need to be set since it does not contain a dom.

maxence-charriere · 2018-11-13T07:46:35Z

drivers/mac/controller.go

+	if c.onControllerDisconnected != nil {
+		c.onControllerDisconnected()
+	}
+	return nil


Would like to keep the code style consistent with the rest of the codebase.
Please put an endline between the bracket and the return.

changed 👍

maxence-charriere · 2018-11-13T07:48:34Z

drivers/mac/controller.m

+    Driver *driver = [Driver current];
+    NSString *ID = in[@"ID"];
+
+    // create blank controller, track it for future connection


Keep the comment style consistent with the rest of the codebase:
create blank controller, track it for future connection => Create blank controller, track it for future connection.

maxence-charriere · 2018-11-13T07:51:38Z

drivers/mac/controller.m

+    NSDictionary *in = @{
+        @"ID": ID,
+        @"Button": button,
+        @"XValue": @(xValue),


same as above for x and y.

jamesalbert · 2018-11-13T20:07:29Z

@maxence-charriere Hey, thanks! This was actually my first attempt at objective-c, apologies for my lack of super cool idioms 😄 To address your message above:

Just a few comments to keep the style consistent.

Sounds good. I tried my best to keep it consistent, but I'll go ahead and fix up the addressed issues.

There is a Close call missing. It would remove the controller from the elems. (have to be done in Go and Objc).

Ah! I knew there was something to clear up. I tried dealloc'ing them at first, then realized I couldn't do that with arc.

What should happen if a controller is disconnected? Should it be closed or a controller can be reopened.

So I tried putting a lot of thought into this. A lot of apps are annoying to use with a controller when it gets disconnected. But if you instantiate an app.Controller instance, it subscribes to notifications about controller connections/disconnections. While the instance sticks around, you can connect and disconnect the controller as many times as you want (sometimes it dies, sometimes you turn it off by mistake). My preference is to keep this logic so users don't have to restart the app to get re-connected in the case of accidental disconnection.

Did you take a look on what is done on other platform? Look enough generic for me but just in case.

Definitely something to look into. I'm willing to take a wack at linux, but I'll be honest, I'm not gonna be too helpful on the Windows front 😄

Logs wrapper should be added too.
After logs wrapper is added, ensure test coverage is still at 100% on app and app/internal/core.

Sounds good 👍

maxence-charriere · 2018-11-13T20:22:41Z

I tried dealloc'ing them at first

Just setting stuff to nil should make the job ;).

Also when I say removing the elems:

objc: [driver.elements removeObjectForKey:self.ID];
Go: driver.elems.Delete(YOUR_CONTROLLER)

jamesalbert · 2018-11-14T05:01:25Z

@maxence-charriere I've added core.Controller and did the requested changes. Coverage is 100% on my end. Let me know if there's anything else I need to do / forgot to do. I'm getting tired 😴

maxence-charriere

Hello,
Thanks for modifying :).

I spotted a few other things:

We should use enum to identify the button/pad that is the source of an event.
Left/Right stick button are not handled.
Just some little typo to modify.
I m wondering if we should rename Controller by GamePad. Let me know what you think.

I would like that the api look like the following:

// ControllerInput describes a controller input.
type ControllerInput int

// Constants that describe the controller inputs.
const (
	DirectionalPad ControllerInput = iota
	LeftThumbstick
	RightThumbstick
	A
	B
	X
	Y
	L1
	L2
	R1
	R2
	Pause
)

// ControllerConfig is a struct that describes a controller.
type ControllerConfig struct {
	// The function that is called when when the directional pad is pressed.
	OnControllerDpadChange func(in ControllerInput, x float64, y float64) `json:"-"`

	// The function that is called when a button in pressed.
	OnControllerButtonChange func(in ControllerInput, value float64, pressed bool) `json:"-"`

	// The function that is called when the controller is connected.
	OnControllerConnected func() `json:"-"`

	// The function that is called when the controller is disconnected.
	OnControllerDisconnected func() `json:"-"`

	// The function that is called when the pause button is pressed.
	OnControllerPause func() `json:"-"`

	// The function that is called when the controller is closed.
	OnControllerClose func() `json:"-"`
}

It would allow to auto document what pad and button are available and allow strict check of the values for when we use the api.

Could you modify the code to make it work within what is described above?

I know I m very strict with the requirement but I really want this package to be highest quality as I can.
Thanks you so much for this. Again this is an amazing job :).

maxence-charriere · 2018-11-14T19:04:29Z

app.go

@@ -131,6 +131,14 @@ func NewContextMenu(c MenuConfig) Menu {
 	return driver.NewContextMenu(c)
 }

+// NewController creates a controller described by the given


creates a controller => creates the controller

maxence-charriere · 2018-11-14T19:05:03Z

controller.go

@@ -0,0 +1,18 @@
+package app
+
+// ControllerConfig is a struct that describes a controller


Typo => describes a controller.

maxence-charriere · 2018-11-14T19:19:37Z

controller.go

+
+// ControllerConfig is a struct that describes a controller
+type ControllerConfig struct {
+	// event handlers


I meant a comment that describes every functions.

maxence-charriere · 2018-11-14T19:56:23Z

drivers/mac/controller.go

+	id string
+
+	// event handlers
+	onControllerDpadChange   func(string, float64, float64)


this one should be changed too.
The ones with prefix kept are the one that are objc handler called outside of the controller struct.

maxence-charriere · 2018-11-14T19:57:43Z

drivers/mac/controller.m

+        [driver.goRPC call:@"controller.OnPause" withInput:in onUI:YES];
+    };
+
+    // diamond buttons + triggers + shoulders


Look like you forgot to add the thumbstick buttons.

those are down below with the dpad. I separated them because their event payloads are similar

Its when you click on the stick. They are their own button different than the position.

Oh I see, the Nimbus I've been testing on doesn't have click-able thumbsticks, but I can add the code for it like the rest.

jamesalbert · 2018-11-15T03:42:30Z

@maxence-charriere Added a new round of changes (I like the enum idea btw, will allow the user to more easily dispatch based on button). The only thing to my knowledge I didn't do was add the thumbstick click buttons. I'm not seeing anything in Apple's documentation outlining those buttons. GCExtendedGamepad which is their most featured gamepad object doesn't seem to have a specific handler for that. And since I don't have a controller with those kind of thumbsticks, I can't code reliably for it.

jamesalbert · 2018-11-15T03:45:30Z

I've also updated the sandbox example in case you want to try out the changes

jamesalbert · 2018-11-15T03:47:41Z

full disclosure: your example for the enum had a bit about: onControllerButtonChange(in app.ControllerInput.

I've changed in app.ControllerInput to input app.ControllerInput to avoid naming conflicts with reserved words in both go (the interface parameter) and objective-c (the NSDictionary being returned)

jamesalbert · 2018-11-15T04:04:01Z

I know I m very strict with the requirement but I really want this package to be highest quality as I can.

Absolutely, I strive for the same, but sometimes I forget those little details 😄 This is an awesome library, seeing some cool stuff from #21 and #141

maxence-charriere · 2018-11-15T04:49:37Z

Here for the stick button https://developer.apple.com/documentation/gamecontroller/gcextendedgamepad/3034770-leftthumbstickbutton?language=objc.

maxence-charriere · 2018-11-15T04:51:34Z

For the in rather than input, just keep it in go. Its a common pattern to use shortname especially when the type next to have the full name. Totally agree for objc.

jamesalbert · 2018-11-15T05:38:47Z

I apparently can't do documentation either, must have missed that

jamesalbert · 2018-11-15T05:40:06Z

@maxence-charriere what about cases like:

func onControllerDpadChange(c *Controller, in map[string]interface{}) interface{} {
	if c.onDpadChange != nil {
		input := app.ControllerInput(in["Input"].(float64))
		x := in["X"].(float64)
		y := in["Y"].(float64)
		c.onDpadChange(input, x, y)
	}

	return nil
}

maxence-charriere · 2018-11-15T05:41:32Z

It is fine, I care more in the API declaration.

jamesalbert · 2018-11-15T05:44:49Z

api declaration updated (I'll squash these commits at the end btw 😄 )

jamesalbert · 2018-11-15T06:49:04Z

I'm not necessarily against renaming Controller to GamePad. I just figured since gamepad is under GCController I'd go with that hierarchy. Is that something you still want to change?

maxence-charriere

Great job!

I think keep with controller is fine.
I gave some other feedbacks that are detailed below.
We should also add the log wrapper for the controller:

in logs.go, add the controllerWithLogs struct:

// Controller logs.
type controllerWithLogs struct {
	Controller
}

func (c *controllerWithLogs) Close() {
	WhenDebug(func() {
		Logf("controller %s is closing", c.ID())
	})

	c.Controller.Close()
	if c.Err() != nil {
		Logf("controller %s failed to close: %s",
			c.ID(),
			c.Err(),
		)
	}
}

Still in logs.go, add the NewController func to driverWithLogs:

func (d *driverWithLogs) NewController(c ControllerConfig) Controller {
	WhenDebug(func() {
		config, _ := json.MarshalIndent(c, "", "  ")
		Logf("creating controller: %s", config)
	})

	d.Driver.NewController(c)
}

Check that test are still 100%.

I think we are close. Thank you so much for your work.

maxence-charriere · 2018-11-16T18:51:38Z

driver.go

@@ -34,6 +34,8 @@ type Driver interface {
 	// given configuration.
 	NewContextMenu(MenuConfig) Menu

+	NewController(ControllerConfig) Controller


Doc is missing here.

maxence-charriere · 2018-11-16T18:55:06Z

drivers/mac/controller.go

+
+// newController creates an *app.Controller from
+// the specified app.ControllerConfig
+func newController(cc app.ControllerConfig) *Controller {


I see why you used cc.
I would prefer you keep c for the config and name the controller variable with its full name.

changed 👍

maxence-charriere · 2018-11-16T19:00:42Z

drivers/mac/controller.go

+//		leftShoulder, rightShoulder
+//		leftTrigger, rightTrigger
+func onControllerButtonChange(c *Controller, in map[string]interface{}) interface{} {
+	if c.onButtonChange != nil {


here you can put everything in the function call:

c.onButtonChange( app.ControllerInput(in["Input"].(float64)), in["Value"].(float64), in["Pressed"].(bool), )

It will avoid to create temporary variables.

made them inline params 👍

maxence-charriere · 2018-11-16T19:07:31Z

drivers/mac/controller.h

+ (void) close:(NSDictionary *)in return:(NSString *)returnID;
+ (void) listen:(NSDictionary *)in return:(NSString *)returnID;
+
+- (void) controllerConnected;


No need to prefix by controller here since it is already in a Controller object.

- (void ) connected; - (void ) disconnected;

changed 👍

maxence-charriere · 2018-11-16T19:08:13Z

drivers/mac/controller.m

+    [[NSNotificationCenter defaultCenter] addObserver:controller selector:@selector(controllerDisconnected) name:GCControllerDidDisconnectNotification object:nil];
+
+    // Check if controller was connected before application started
+    if ([[GCController controllers] count] > 0) {


thank you 😎

maxence-charriere · 2018-11-16T19:10:39Z

drivers/mac/controller.m

+
+// Called when a controller becomes connected
+- (void) controllerConnected {
+    NSLog(@"info - controller connected");


remove NSLog

maxence-charriere · 2018-11-16T19:10:53Z

drivers/mac/controller.m

+
+// Called when a controller becomes disconnected
+- (void) controllerDisconnected {
+    NSLog(@"info - controller disconnected");


maxence-charriere · 2018-11-16T19:17:22Z

drivers/mac/controller.m

+    // joysticks + dpad
+    controller.profile.dpad.valueChangedHandler = ^(GCControllerDirectionPad *dpad, float x, float y) {
+        // Directional pad has been changed
+        [Controller emitDpad:ID Input:0 X:x Y:y];


Lets redeclare the button type in the .h:

typedef enum ControllerInput : NSUInteger { // equivalent to Go ... } ControllerInput;

jamesalbert · 2018-11-17T03:20:07Z

@maxence-charriere Alright, I think I tackled all the things you suggested, plus I was getting these warnings:

controller.m:131:24: warning: 'rightThumbstickButton' is only available on macOS 10.14.1 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/GameController.framework/Headers/GCExtendedGamepad.h:109:68: note: 'rightThumbstickButton' has been explicitly marked partial here
controller.m:131:24: note: enclose 'rightThumbstickButton' in an @available check to silence this warning

so I added checks for macOS 10.14.1 around those two handlers and that got rid of them

jamesalbert · 2018-11-17T03:22:06Z

Also, thanks for the sample code, definitely expedites the corrections 👍

maxence-charriere

Looks good. Almost there :)

maxence-charriere · 2018-11-17T04:30:07Z

driver.go

@@ -34,6 +34,8 @@ type Driver interface {
 	// given configuration.
 	NewContextMenu(MenuConfig) Menu

+	// NewController creates and listens for controller input described by the


NewController creates the controller described by the given configuration.

maxence-charriere · 2018-11-17T04:31:26Z

drivers/mac/controller.go

@@ -26,28 +26,28 @@ type Controller struct {

 // newController creates an *app.Controller from
 // the specified app.ControllerConfig
-func newController(cc app.ControllerConfig) *Controller {
-	c := &Controller{
+func newController(c app.ControllerConfig) *Controller {


also you can remove the comment here since this is not exported.

maxence-charriere · 2018-11-17T04:33:27Z

drivers/mac/controller.m

@@ -113,56 +111,58 @@ + (void) listen:(NSDictionary *)in return:(NSString *)returnID {
    // joysticks + dpad
    controller.profile.dpad.valueChangedHandler = ^(GCControllerDirectionPad *dpad, float x, float y) {
        // Directional pad has been changed
-        [Controller emitDpad:ID Input:0 X:x Y:y];
+        [Controller emitDpad:ID Input:DirectionalPad X:x Y:y];


I think you can remove all these comments about what it does since the code self explain.

maxence-charriere · 2018-11-17T04:36:33Z

logs.go

+	}
+}
+
+func (d *driverWithLogs) NewController(c ControllerConfig) Controller {


This should be moved with the other functions related to the driverWithLogs. Please keep the same order as where you declared it in driver.

I see, I swapped NewController and Close. Is that all you meant? Or do you want me to move the overall placement?

Oh! I see it now ha

Driver with logs is a different struct than controllerwithlogs

yeah that was a subtle difference that went over my head 😄

jamesalbert · 2018-11-17T05:00:30Z

updated 👍

maxence-charriere

Looks good to me, just fix the last feedback and i ll merge it in the weekend. Thank you so much for this, I did not even think about game controller at all.

Also thanks for your patience and the modifications in the review.

maxence-charriere · 2018-11-17T07:27:36Z

logs.go

@@ -73,6 +73,20 @@ func (d *driverWithLogs) NewWindow(c WindowConfig) Window {
 	return &windowWithLogs{Window: w}
 }

+func (d *driverWithLogs) NewController(c ControllerConfig) Controller {


Please keep the same func order as in driver. I think its after newcontextmenu

jamesalbert · 2018-11-17T07:38:45Z

Looks good to me, just fix the last feedback and i ll merge it in the weekend. Thank you so much for this, I did not even think about game controller at all. Also thanks for your patience and the modifications in the review

No problem at all, I needed to grow my go skills and objective-c is a whole new world 😆

jamesalbert · 2018-11-17T21:36:00Z

squashed commits

maxence-charriere requested changes Nov 13, 2018

View reviewed changes

maxence-charriere requested changes Nov 14, 2018

View reviewed changes

maxence-charriere requested changes Nov 16, 2018

View reviewed changes

maxence-charriere requested changes Nov 17, 2018

View reviewed changes

maxence-charriere reviewed Nov 17, 2018

View reviewed changes

Added MFi Controller Support

331ce47

jamesalbert force-pushed the master branch from 1bcb431 to 331ce47 Compare November 17, 2018 21:35

maxence-charriere approved these changes Nov 19, 2018

View reviewed changes

maxence-charriere merged commit f49f807 into maxence-charriere:master Nov 19, 2018

		@@ -0,0 +1,18 @@
		package app

		// ControllerConfig is a struct that describes a controller

Added MFi Controller Support #225

Added MFi Controller Support #225

Conversation

jamesalbert commented Nov 13, 2018 • edited

Summary

Fixes

Example

maxence-charriere left a 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

maxence-charriere Nov 14, 2018 • edited

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

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

jamesalbert commented Nov 13, 2018

maxence-charriere commented Nov 13, 2018

jamesalbert commented Nov 14, 2018

maxence-charriere left a comment • edited

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

maxence-charriere Nov 14, 2018 • edited

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

jamesalbert commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

maxence-charriere commented Nov 15, 2018

maxence-charriere commented Nov 15, 2018 • edited

jamesalbert commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

maxence-charriere commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

jamesalbert commented Nov 15, 2018

maxence-charriere left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxence-charriere Nov 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxence-charriere Nov 16, 2018 • edited

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

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

Choose a reason for hiding this comment

jamesalbert commented Nov 17, 2018 • edited

jamesalbert commented Nov 17, 2018

maxence-charriere left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxence-charriere Nov 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesalbert commented Nov 13, 2018 •

edited

maxence-charriere Nov 14, 2018 •

edited

maxence-charriere left a comment •

edited

maxence-charriere Nov 14, 2018 •

edited

maxence-charriere commented Nov 15, 2018 •

edited

maxence-charriere left a comment •

edited

maxence-charriere Nov 16, 2018 •

edited

maxence-charriere Nov 16, 2018 •

edited

jamesalbert commented Nov 17, 2018 •

edited

maxence-charriere Nov 17, 2018 •

edited

maxence-charriere Nov 17, 2018 •

edited