-
Notifications
You must be signed in to change notification settings - Fork 7
Basic implementation of activators and behaviours #19
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
Conversation
2d79179 to
f87969c
Compare
examples/basic/index.html
Outdated
| gripdown: 'changeTask', | ||
| trackpaddown: 'logdefault' | ||
| trackpaddown: 'logdefault', | ||
| 'trackpad.doubletouch': 'doubletouch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the dot for consistency with other event names? trackpaddoubletouch, 'trackpaddoublepress', 'trackpadlongpress'
f6ea734 to
e526bbf
Compare
|
Hmm, not sure how I would implement a dpad as a modifier in this way... You need both a To give a specific example we can iterate on: 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). |
|
@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. |
|
Updated gif too |
|
The map between the input sources and modes, is what I'm defining in |
|
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);
class DoublePress {
constructor(el, button, targetEventname) {
this.lastTime = 0;
this.timeOut = 250;
this.eventName = button + 'down';
this.onButtonDown.bind(this);
el.addEventListener(this.eventName, this.onButtonDown);
},
onButtonDown(event) {
var time = performance.now();
if (time - this.lastTime < this.timeOut) {
activate(event.detail);
} else {
this.lastTime = time;
}
},
destroy() {
el.removeEventListener(this.eventName, this.onButtonDown);
}
}
AFRAME.registerInputActivator('doublepress', DoublePress); or just become normal functions that return a cleanup function like: 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. |
|
@netpro2k thanks for the feedback! I've just updated the PR
I agree!, done in 4755f58
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
Yep, done in 41bc259
I believe that it's better to keep the naming consistency as @dmarcos suggested and remove the dot, so the events should be Changed it on: d8a4d23
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? |
fd76207 to
d8a4d23
Compare
If input mapping provides a polling api instead of an eventing api, activators and behaviors can poll their digital |
|
I'm not sure if this is the best place for this comment, but I wanted to lend some thoughts. 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 The application registers its action sets, The user provides a configuration: The configuration depends on "behaviors" being registered: Activators allow us to further configure input such as differentiating 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 |
|
Hmm, not sure I like removing 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): |
|
@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. 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 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. 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. 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. |
af1b095 to
1e21850
Compare
1e21850 to
3da9555
Compare
| 'trackpad.doubletouch': 'doubletouch', | ||
| 'trackpad.doublepress': 'doublepress', | ||
| //'trackpad.longpress': 'longpress', | ||
| 'trackpaddpadleftdown': 'dpadleft', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use the "down" activator here, but might be nice to have an example showing its optional. In that case though maybe add a comment explaining this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good point, fixed
| task1: { | ||
| changeTask: { label: 'Change task' }, | ||
| logdefault: { label: 'Test Log' }, | ||
| logtask1: { label: 'Test Log Task 1' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor point since this is just a test page, but might be nice to use action that less directly tie to what their bindings are to get people thinking about actions in the right way. Weather this is worth fixing mostly depends on whether this file servers more as an integration test or a usage example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep the current example is just used to test, so I'll create a more polished example with some text showing instructions and some kind of interactions like press X to change color of this cube or whatever
src/activators/doublepress.js
Outdated
| } | ||
| }, | ||
|
|
||
| destroy () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removeListeners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
longpress,doublepressanddoubletouch. Registered byAFRAME.registerInputActivator.trackpad=>dpad. Registered byAFRAME.registerInputBehaviour.