fernandojsg / aframe-input-mapping-component

https://fernandojsg.github.io/aframe-input-mapping-component
MIT License
24 stars 7 forks source link

Basic implementation of activators and behaviours #19

Closed fernandojsg closed 6 years ago

fernandojsg commented 6 years ago

ezgif-5-ced6b184f4

netpro2k commented 6 years ago

Hmm, not sure how I would implement a dpad as a modifier in this way...

You need both a trackpaddown event as well as an axismove event (or the name of an axis to read directly). To complicate things further, on Oculus touch you would only want an axismove. What event would you put the modifier on? And how do I bind one action to left on the dpad and another action to right?

To give a specific example we can iterate on:

const inputActions = {
  default: {
    ACTION_TURN_LEFT: { label: "Turn left" },
    ACTION_TURN_RIGHT: { label: "Turn right" }
  }
}

const inputMappings = {
  default: {
    'vive-contorls': {
      // map ACTION_TURN_LEFT to pressing the left side of the touchpad on the right controller
      // map ACTION_TURN_RIGHT to pressing the right side of the touchpad on the right controller
    },
    `oculus-touch-controls`: {
      // map ACTION_TURN_LEFT to tilting the thumbstick left on the right controller
      // map ACTION_TURN_RIGHT to tilting the thumbstick right on the right controller
    }
  }
}

Note I included handedness and 2 controller types as it forces us to think about how these all interplay. The solution might be that there are different modifiers for the 2 controller types, or that modifiers can branch on device type.

Steam breaks the physical controllers first into input sources and then configures these input sources with specific modes (such as dpad) https://partner.steamgames.com/doc/features/steam_controller/concepts, but this feels intuitively like an extra layer of abstraction to me, though it does suggest that what we are calling "modifiers" above likely want to have multiple inputs, multiple output actions, and arbitrary configuration options (some of which might be actions, ex outer ring binding on dpad source mode).

fernandojsg commented 6 years ago

@netpro2k sorry I forgot to put it as WIP, I've just modified it quite a lot after discussing with @johnshaughnessy exactly that situation for dpad. DPad will be implemented as a behaviour, converting the trackpad into a dpad and generating all events for it. While the rest of (previously called modifiers) are activators to follow steam's controller API.

fernandojsg commented 6 years ago

Updated gif too

fernandojsg commented 6 years ago

The map between the input sources and modes, is what I'm defining in behaviours (I'm open to suggestions to change its name :D) It's still a WIP but I believe with the current way to go we could cover most of our need without moving to a much complex API. I was discussing with @johnshaughnessy about focusing on the current scope with the needs we had (Mainly handedness and gestures) even if the solution doesn't fit 100% of the cases but still it's working for our current purposes. And once we have this working we could evolve it without the pressure to have something ready to migrate our demos.

netpro2k commented 6 years ago

I know this is still a WIP but its looking good. Some random thoughts after looking through it:

function LongPress (el, button, activate) {
  this.pressTimer = null;
  this.timeOut = 1000;

  el.addEventListener(button + 'down', event => {
    this.pressTimer = window.setTimeout(() => {
      activate(event.detail);
    }, 1000);
  });

  el.addEventListener(button + 'up', event => {
      clearTimeout(this.pressTimer);
  });
}

AFRAME.registerInputActivator('longpress', LongPress); 

AFRAME.registerInputActivator('doublepress', DoublePress);


or just become normal functions that return a cleanup function like:

```javascript
function doublePress (el, button, activate) {
  let lastTime = 0;
  let timeOut = 250;

  const buttonDown = function(event) {
    var time = performance.now();
    if (time - lastTime < timeOut) {
      activate(event.detail);
    } else {
      lastTime = time;
    }
  }

  const eventName = button + 'down';
  el.addEventListener(eventName, buttonDown);

  return function() {
    el.removeEventListener(eventName, buttonDown);
  }
}

AFRAME.registerInputActivator('doublepress', doublePress); 

Don't have a strong preference either way but I suspect the former will be more familiar to people.

function createSimpleActivator(suffix) {
  return function(el, button, activate) {
    el.addeventlistener(button + suffix, activate)
  }
}

AFRAME.registerInputActivator('down', createSimpleActivator("down"));
AFRAME.registerInputActivator('up', createSimpleActivator("up"));
AFRAME.registerInputActivator('touchstart', createSimpleActivator("touchstart"));
AFRAME.registerInputActivator('touchend', createSimpleActivator("touchend"));

Not sure how I feel about these since they are basically a noop, but they might help.

behaviors: {
  default: {
    keyboard: {
      // ??
    }
  }
},
mappings: {
  default: {
    keyboard: {
      "axis_w_s.move": "action_translate_z", // move as a simple activator descibed above
      "axis_a_dmove": "action_translate_x" // move as just a event name suffix like axismove
    }
  }
}

but not sure how I would want to setup the behavior for that.

fernandojsg commented 6 years ago

@netpro2k thanks for the feedback! I've just updated the PR

Activators should probably just get a function they can call to trigger their associated action, rather than having to emit the event themselves. This enforces their intended use a simple gate to activating an action rather than generating new events like a behavior:

I agree!, done in https://github.com/fernandojsg/aframe-input-mapping-component/pull/19/commits/4755f5872d92700913ac915659486b7e90e722a7

The string concatenation contract in activators/behaviors still feels a bit odd to me. For example LongPress expects you to pass it the prefix for events that will have both a down and up event. I don't have any better ideas yet, and I guess this pattern is probably common enough in the aframe community that it makes sense, but it still has a bit of a code smell for me.

Yep, that's a part that I don't really like, but still, that could be something configurable in the future in the activator itself. From now we're based on the current tracked-controls implementation so we know the name of the events that we could expect. I can imagine an advanced version where no need to generate the intermediate events from trackedcontrols to inputmapping so we could handle it in a much prettier way.

We will need a way to clean up activators and behaviors when controllers get switched, or mappings change at runtime. Either this means they will need to have some sort of destroy method:

Yep, done in https://github.com/fernandojsg/aframe-input-mapping-component/pull/19/commits/41bc259d54ce55dec59d10f2fbad15c59a5833e6

As a new developer I would probably be a bit confused that I can create a mapping for touchpad.doublepress but I should create a mapping for touchpaddown instead of touchpad.down. Perhaps we should just make (optional?) simple activators for these just for consistency? For example:

I believe that it's better to keep the naming consistency as @dmarcos suggested and remove the dot, so the events should be touchpaddoublepress as touchpaddown. Again, this is the kind of modifications we could do in a future when both events will be generated by the same component and we could decide a better name if needed, although if we state that applications should use always inputmappings we won't need to expose it either.

Changed it on: https://github.com/fernandojsg/aframe-input-mapping-component/pull/19/commits/d8a4d23f7e7e13e9ec79bd889078f2b8bbbb8938

Lastly, one experiment worth mentally thinking through (probably not implementing for now) is how we would implement using the wasd keys as if they were a pair of axes using behaviors/activators (we are doing this in the character controller for the mr-social-cleint).

I don't think I understand you here. Do you want wasd to behave like a dpad? As you're going to have still a custom mapping for the keyboard why don't you just do the mapping with the keys instead?

johnshaughnessy commented 6 years ago

The string concatenation contract in activators/behaviors still feels a bit odd to me. For example LongPress expects you to pass it the prefix for events that will have both a down and up event. I don't have any better ideas yet, and I guess this pattern is probably common enough in the aframe community that it makes sense, but it still has a bit of a code smell for me.

If input mapping provides a polling api instead of an eventing api, activators and behaviors can poll their digital origin_actions e.g. Input.GetDigitalAction(myDigitalActionIndex); // returns true or false This way, activators like down, up, click, double_click, long_press don't have to listen to named events (+ "down" and + "up") .

johnshaughnessy commented 6 years ago

I'm not sure if this is the best place for this comment, but I wanted to lend some thoughts. The OpenXR proposal uses semantic paths for action origins ("/devices/vive/right/menu") and my pseudocode snippets below do too.

The trackpad-to-dpad component is an example of what might become a behavior that requires multiple input source "origins" as well as configuration settings (8-direction, 4-direction, lerping, requires_press) to output the right events. Below is a similar example that maps 4 digital origins to an analog2d.

The application registers its action sets, gameplay and menu.

Input.registerActionSets({
  gameplay: [
    {
      "name" : "/actions/gameplay/walk_direction",
      "type" : "analog2d"
    },
    {
      "name" : "/actions/gameplay/toggle_mute",
      "type" : "digital"
    },
    {
      "name" : "/actions/gameplay/toggle_audio_settings_widget",
      "type" : "digital"
    },
    {
      "name" : "/actions/gameplay/open_menu",
      "type" : "digital"
    }
  ],
  menu :[
    {
      "name" : "/actions/menu/close_menu",
      "type" : "digital"
    }
  ]
});

The user provides a configuration:

Input.registerMapping({
  gameplay: {
    behaviors: [
      {
        name: "some_four_button_analog2d", // unique name in mapping
        type: "four_button_analog2d",
        bindings : [
          {
            origin : "/devices/keyboard/w",
            action : "north" // "activators" array ommitted because we don't need any kind of filter on this origin
          },
          {
            origin : "/devices/keyboard/a",
            action : "east"
          },
          {
            origin : "/devices/keyboard/s",
            action : "south"
          },
          {
            origin : "/devices/keyboard/d",
            action : "west"
          },
        ],
        config : {
          "lerp" : true
        }
      }
    ],
    bindings: [
      {
        "origin" : "/actions/some_four_button_analog2d/axes",
        "action" : "/actions/gameplay/walk_direction"
      },
      {
        "origin" : "/devices/vive/right/trackpad_button",
        "activators" : [
          {
            type: "click",
            action : "/actions/gameplay/toggle_mute"
          },
          {
            type: "hold",
            duration : 1000, 
            action : "/actions/gameplay/toggle_audio_settings_widget",
          }
        ]
      },
      {
        "origin" : "/devices/keyboard/m",
        "activators" : [
          {
            type: "down",
            action: "/actions/gameplay/open_menu"
          }
        ]
      }
    ]
  },
  menu: {
    bindings: [
      {
        origin : "/devices/keyboard/m"
        "activators" : [
          {
            type: "down",
            action : "/actions/menu/close_menu"
          }
        ]
      }
    ]
  }
)})

The configuration depends on "behaviors" being registered:

Input.registerBehavior({
  type: "four_button_analog2d", //unique name among behavior types
  origin_actions: [
    {
      "name" : "north",
      "type" : "digital"
    },
    {
      "name" : "east",
      "type" : "digital"
    },
    {
      "name" : "south",
      "type" : "digital"
    },
    {
      "name" : "west",
      "type" : "digital"
    },
  ],
  output_actions: [
    {
      "name" : "axes",// <-- this will be made available as "/actions/some_four_button_analog2d/axes" below
      "type" : "analog2d"
    }
  ],

  // I imagine "tick" is some function that is called every frame.
  // I'm not sure where its state is stored, but this contains the behavior's logic.
  tick: function(config){
    // Query digital actions
    const north = Input.GetDigitalAction(northActionIndex); 
    // and south, west, east

    // Use settings in config
    const x = config.lerp ? lerp(prevX, targetX, 0.5) : targetX; 
    // and y

    // Update axes (and fire an event?)
    output.analog2d.axes.set(x, y);
  }
});

Activators allow us to further configure input such as differentiating soft_press from full_press via a soft_press_threshold for analog actions, or specifying click and double_click for digital actions.

I don't know whether you all think this amount of configuration of behaviors / mappings is required for this project. I could imagine this kind of configuration being generated by a UI or being paired with localization strings to provide in-game tooltips.

netpro2k commented 6 years ago

Hmm, not sure I like removing . from activators. Though it makes "down" and "up" look more like "longpress", that seems wrong in this case since they are not being handled in the same way and you are sort of just accidentally opting into an activator in some cases.

It seems like activators should definitely be something you explicitly opt into per binding. Having it just be a suffix seems like you could easily accidentally opt into an activator just because your event happened to be named in some way and since the user might not always be in control of what events are being emitted you could end up in a case where a poorly named activator may actually prevent you from ever binding to some event.

Since you may want to specify some configuration for an activator anyway, maybe it should just become an object (also combining in some of the OpenXR stuff @johnshaughnessy and I talked about in person and he described above):

mappings: {
  "vive-controls":  {
    task1: [
      {
        source: 'trackpaddpadright',
        activators:  [
          {
            type: "longpress",
            timeout: 300,
            action: 'dpadrightlong',
          }
        ]
      }
    ]
  }
}
fernandojsg commented 6 years ago

@johnshaughnessy I totally agree with the proposal you shared, although as I stated previously my initial idea with this PR is to add the basic behaviours / activators functionality in a way that doesn't drastically modify the current API and it could be still easy to use manually without need of a new UI. At the same time I'm starting to work on a UI POC for a more ambitious approach. Basically the one we discussed: an independent API that doesn't depends on tracked-controls or aframe at all, so things like the activators will be completely different as you exposed.

So as in that new API we will do much more advanced configurations the discussions probably will last longer than with a simpler approach as the current input-mapping that's why I wanted to add this basic functionality here to release a v1 in the same way as the current released version and move to the next level with the other API.

So my bet is to move this discussions to the other API as it will totally fit there and the implementation will become more robust without hacking around the current tracked-controls and so. In the same way regarding the comment from @netpro2k I still see value on keeping the events name as is to try to be consistent with tracked-controls, otherwise we should add also activators for down, press, up and so on, and as we already know the finite number of combinations as we can't go beyond the tracked-controls events it won't be possible to have name collisions. (That being said, I have no preference if we want to keep the . or not yet)

I agree with that the activators will need some configuration data, but I would leave that out of the current scope and just provide a basic values for the current activators and even create a new one if needed for the sake of simplicity. Once we have a UI as a POC we could explore a more advance way to config it.

Could you provide a use case of your social demo that doesn't fit at all with the current proposal? or if it's possible to adapt it to the current proposal, follow the way I described by leaving it as a simple/entry level input API and go for the more advanced version.