Skip to content
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

DeviceSourceManager #8031

Merged
merged 19 commits into from
May 11, 2020
Merged

DeviceSourceManager #8031

merged 19 commits into from
May 11, 2020

Conversation

PolygonalSun
Copy link
Contributor

@PolygonalSun PolygonalSun commented Apr 16, 2020

This PR has the initial code for the DeviceSourceManager, a manager that keeps track of specific devices and provides the user with easier to understand context when getting input.

deviceEnum.ts:

  • Added enums for device types and inputs for known gamepads (xbox, ps4, and switch)

deviceInputSystem.ts:

  • Increased _MAX_KEYCODES to 255 to account for international keyboard layouts
  • Added MOUSE_DEVICE and mouse specific ternary operator to account for chance that a mouse's pointerId is not 1 (spec no longer guarantees this).
  • Fixed an issue where touch pointers were no longer being registered after initial one
  • Removed a throw from pollInput to allow for new change to input data access
  • Added observeInput callback to enable use of event-driven updates for Keyboard, Mouse, and Touch

deviceSourceManager.ts:

  • Users can now get input from the input system via the getInput function. This function is not pollInput as I felt that getInput was less simpler terminology.
  • There are 4 different observables (connect-before/after, disconnect-before/after) to allow for better customization of the connection process. While they have no major use in the DSM, they will provide higher levels with some event control.
  • All gamepad detection methods are based off of the device names given by the Gamepad API. Considerations will need to be made for other platforms.
  • With the getInput command, you can call it with the device type and input enum and get the first connected input value or you can specify a slot (only applicable to gamepads) and get a specific devices value.

To use this code, the minimum code that you need is to create an instance of the DeviceSourceManager and have a getInput call somewhere in your game/render loop.

Example:

var dsm = new BABYLON.DeviceSourceManager(scene.getEngine());
...
scene.onBeforeRenderObservable.add(() => {
     if (dsm.getInput(BABYLON.DeviceType.Xbox, BABYLON.XboxInputs.A) == 1) {
          // Do something while A is pressed
     }
});

@sebavan sebavan requested a review from bghgary April 16, 2020 22:42
Copy link
Member

@ryantrem ryantrem left a comment

Choose a reason for hiding this comment

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

My understanding is that this API is entirely polling based, specifically to get current input state. It seems like for many situations this may be hard to use. For example:

  1. When a key is pressed, I want to add an object to a scene - do I need to manually track key state and look for a change across two frames and then react to that?
  2. As a pointer is moved across the screen, I want to rotate an object - do I need to track pointer position deltas across frames and apply that delta to a rotation?

I would have though we'd want to include a more event driven model, or at the very least for polling have a way of querying deltas across frames without the consumer having to manage this themselves. Am I missing anything, or are these scenarios not well covered with the current proposal?

Edit: It's possible I'm not understanding how this is supposed to work. In your example above, you check whether the Xbox input A's state is 1 and your comment says "Do something when A is hit". Would that example be doing something only when A transitions from a non-pressed to pressed state, or would it do something every frame as long as A is in a pressed state? I guess if it is the latter, then how would you expect to do the former (perform an action when A transitions from non-pressed to pressed)?

src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
@PolygonalSun
Copy link
Contributor Author

PolygonalSun commented Apr 17, 2020

My understanding is that this API is entirely polling based, specifically to get current input state. It seems like for many situations this may be hard to use. For example:

  1. When a key is pressed, I want to add an object to a scene - do I need to manually track key state and look for a change across two frames and then react to that?
  2. As a pointer is moved across the screen, I want to rotate an object - do I need to track pointer position deltas across frames and apply that delta to a rotation?

I would have though we'd want to include a more event driven model, or at the very least for polling have a way of querying deltas across frames without the consumer having to manage this themselves. Am I missing anything, or are these scenarios not well covered with the current proposal?

Edit: It's possible I'm not understanding how this is supposed to work. In your example above, you check whether the Xbox input A's state is 1 and your comment says "Do something when A is hit". Would that example be doing something only when A transitions from a non-pressed to pressed state, or would it do something every frame as long as A is in a pressed state? I guess if it is the latter, then how would you expect to do the former (perform an action when A transitions from non-pressed to pressed)?

This is a polling based system. For #1, I'm not 100% sure that I understand the scenario. With the code as is, you should be able to just listen for input using onBeforeRenderObservable and add your object there. This may not be what you're asking so I'd like to confirm things with you offline. For #2, the current system just keeps track of absolute position and click status. For this scenario, you'd need to calculate and track deltas manually. If you want, I could add additional support for things like movement deltas without major issue.

For the XboxInputs.A example, the comment may be a bit misleading. It should perform whatever action is in the if statement block, each frame, as long as the A button remains pressed. For the alternative, the user would have to add additional code to their query to handle single button presses (eg. add a bool to check that a button was pressed and the execute an action if the button set the bool to true and is currently 0).

Edit: I also changed the comment to properly state what is happening.

@ryantrem
Copy link
Member

This is a polling based system. For #1, I'm not 100% sure that I understand the scenario. With the code as is, you should be able to just listen for input using onBeforeRenderObservable and add your object there. This may not be what you're asking so I'd like to confirm things with you offline. For #2, the current system just keeps track of absolute position and click status. For this scenario, you'd need to calculate and track deltas manually. If you want, I could add additional support for things like movement deltas without major issue.

For the XboxInputs.A example, the comment may be a bit misleading. It should perform whatever action is in the if statement block, each frame, as long as the A button remains pressed. For the alternative, the user would have to add additional code to their query to handle single button presses (eg. add a bool to check that a button was pressed and the execute an action if the button set the bool to true and is currently 0).

For my first example above, I just meant that when the A keyboard key (for example) goes from up state to down state, I want to add an object to the scene. I think ultimately all my examples are examples of wanting frame-to-frame deltas. For things like an axis (e.g. pointer position), the delta is really obvious (it moved x pixels). I think we can apply the same thing to buttons though (the delta would be 0 if the button has the same state, either down or up, between frames; the delta would be 1 if it went from not pressed to pressed; the delta would be -1 if it went from pressed to not pressed). I think to make the programming model easy, we should do one of two things (or both):

  1. In addition to being able to query the current input state, also be able to query the input state delta since the last frame (for any device type + input index), and be able to get a list of input indexes (per device type) that have changed since the last frame (this would make it easy to see that on a device with many input indices, like a keyboard, that specifically the A key has changed since the last frame).
  2. Expose events for when an input index value changes.

cc @bghgary

@PolygonalSun
Copy link
Contributor Author

@ryantrem @bghgary I believe all of the requested changes should be included now.

@PolygonalSun
Copy link
Contributor Author

Here's a sample script that should demonstrate some of the new features:

var createScene = function () {

    // This creates a basic Babylon Scene object (non-mesh)
    var scene = new BABYLON.Scene(engine);

    // This creates and positions a free camera (non-mesh)
    var camera = new BABYLON.FreeCamera("camera1", new BABYLON.Vector3(0, 5, -10), scene);
    // Set to false to disable events (false is the default)
    var test = new BABYLON.DeviceSourceManager(scene.getEngine(), true);

    test.onBeforeDeviceConnectedObservable.add((device) => {
        const type = device["deviceType"];
        const slot = device["deviceSlot"];
        switch(type)
        {
            case BABYLON.DeviceType.Keyboard:
                console.log("Keyboard connected.");
                break;
            case BABYLON.DeviceType.Mouse:
                console.log("Mouse connected.");
                break;
            case BABYLON.DeviceType.Touch:
                console.log("Touch detected with id of " + slot);
                break;
            case BABYLON.DeviceType.Xbox:
                console.log("Xbox controller connected in Slot " + slot);
                break;
            case BABYLON.DeviceType.DualShock:
                console.log("Dualshock controller connected in Slot " + slot);
                break;
            case BABYLON.DeviceType.Switch:
                console.log("Switch controller connected in Slot " + slot);
                break;
            case BABYLON.DeviceType.Generic:
            default:
                console.log("Unknown device connected in Slot " + slot);
                break;
        }
    });

    // Test for getDeviceSource
    test.onAfterDeviceConnectedObservable.add((device) => {
        if (device["deviceType"] == BABYLON.DeviceType.Keyboard) {
            test.getDeviceSource(BABYLON.DeviceType.Keyboard).onInputChangedObservable.add((source) => {
                console.log("Input KeyCode: " + source["inputIndex"]);
            });
        }
    });

    scene.onBeforeRenderObservable.add(() => {
        if (test.getInput(BABYLON.DeviceType.Xbox, BABYLON.XboxInput.A, 0) == 1)
        {
            console.log("Player 1: A");
        }
        else if (test.getInput(BABYLON.DeviceType.Xbox, BABYLON.XboxInput.A) == 1)
        {
            console.log("First available: A");
        }
    });
    return scene;
};

@PolygonalSun PolygonalSun marked this pull request as ready for review May 5, 2020 02:52
@RaananW
Copy link
Member

RaananW commented May 5, 2020

Would it be possible to add an observable when input actually changed and have the user listen to this, instead of running it on the beforeRender loop?
If I understand @ryantrem correctly, this is what he expected the API to look like (in the first comment) - more event driven.
I know it is much simplified, but the WebXR motion controller API is also polling based. I added a few ways for the user to monitor changes, which you might be able to use here. And again, I understand this is a much broader API.

And just for me to understand - I noticed you removed observables and switched to callback-based API. Is there a specific reason for that?

src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceEnums.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
Copy link
Member

@ryantrem ryantrem left a comment

Choose a reason for hiding this comment

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

The biggest piece of feedback I have for this (mentioned in one of the comments) is that I would make DeviceSource be a nested class inside DeviceSourceManager whose purpose is to provide a type safe interface to dealing with input for a specific DeviceType (and slot), and have all the state managed inside DeviceSourceManager.

src/DeviceInput/InputDevices/deviceEnums.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceEnums.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/InputDevices/deviceSourceManager.ts Outdated Show resolved Hide resolved
src/DeviceInput/deviceInputSystem.ts Outdated Show resolved Hide resolved
src/DeviceInput/index.ts Show resolved Hide resolved
@PolygonalSun
Copy link
Contributor Author

Would it be possible to add an observable when input actually changed and have the user listen to this, instead of running it on the beforeRender loop?
If I understand @ryantrem correctly, this is what he expected the API to look like (in the first comment) - more event driven.
I know it is much simplified, but the WebXR motion controller API is also polling based. I added a few ways for the user to monitor changes, which you might be able to use here. And again, I understand this is a much broader API.

And just for me to understand - I noticed you removed observables and switched to callback-based API. Is there a specific reason for that?

To answer your questions, I've added a callback and observables to allow the user to receive and handle events that will fire off when input sources are updated. The only caveat is that these will not be able to be used with Gamepads because they are not event based. I may revisit this later on. For the change back to callback from Observables, this was done to make the implementation of the Native version of the DeviceInputSystem easier.

Comment on lines -89 to -91
this.onDeviceConnectedObservable.clear();
this.onDeviceDisconnectedObservable.clear();

Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this? If intended, perhaps update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted those observables back to callbacks. I'll update the comment.

@deltakosh deltakosh merged commit c825335 into BabylonJS:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants