GaryStanton / phaser3-merged-input

A Phaser 3 plugin to map input from keyboard & gamepad to player actions
MIT License
34 stars 5 forks source link

Detect single press on gamepad #17

Closed zewa666 closed 2 years ago

zewa666 commented 2 years ago

Hey there @GaryStanton.

I want to handle a single key up (eq to JustUp) both on Keyboard and Gamepad. That means just a single click on lets say either the key E or Xbox Controller X button. defineKey(0, "B2", "E")

Since the update method gets called quite often and I don't want my action (aka interaction with object or npc) to start multiple times, my code looks somewhat like this:

    if (this.playerCtrl.playerControls.buttons.B2     // check for proper button
      && this.playerCtrl.playerControls.interaction.pressed === "B2"  // <---- check for the single iteration where the button got released
      ) {
      ...
    }

where the interaction.pressed looked like a great indicator. The trouble though is that this only works with the keyboard but not the Gamepad. Do you have any idea how I could make it work with that one as well?

GaryStanton commented 2 years ago

Hi @zewa666 ,

I'm using interation.pressed for exactly this usecase and it does indeed work with the gamepad too. Are you using the latest version? I think the gamepad events weren't in some of the earlier releases.

You don't need to check buttons.B2 as well as interaction.pressed, just the interaction check should work.

Here's some code I use in one of my games where I only want to detect a single press of the button:

if (['B8', 'B9', 'B0'].includes(this.input.interaction.pressed)) {
  this.handleInput({ action: 'START'})
}

If you're having problems, are you able to set up a demo in a codepen?

zewa666 commented 2 years ago

I can reproduce the behavior with your Demo.

Inside the update method I've added

if (this.mergedInput.getPlayer(0).interaction.pressed) {
  console.log("yipeee")
}

and this condition is always hit with the keyboard, but never with Mouse or Gamepad. I've got a classic old Xbox 360 wired controller if that's important for the case.

GaryStanton commented 2 years ago

Thanks for the update. You're quite right, I'm seeing the same problem in the demo - though not in my games! Leave it with me and I'll dig into it later today.

GaryStanton commented 2 years ago

So, it looks like this is some kind of a race condition issue. You won't actually receive all of the events if your input controller is on the same scene as the code that handles the input. The demo is all on one scene, so we run into problems there. In my games, I have a separate inputController scene that is launched before the game scenes and run in the background. This is good practice, as you want to be able to handle input across multiple scenes without having to re-set it all up on each scene.

I've been trying to adapt the demo to show this, but I've not quite got it working and very much need to sleep! I'll see if I can get it updated tomorrow and I'll push a patch release.

zewa666 commented 2 years ago

alright thanks for looking into it. I'm not entirely sure I would want an additional parallel active scene just for the inputs tbh as I'm currently merely extracting the movement logic in another class which than is consumed from each scene.

if you could give me pointers at where to look for the race condition I can perhaps help as well. I'm sure there is a way around as the keyboard itself works so its more to like how other inputs are wired most likely

GaryStanton commented 2 years ago

Sure... so, the keyboard 'pressed' flags are detected and set in plugin's update function through calls to checkKeyboardInput(). It uses the key's justDown() function, which only returns true once for each keypress.

However, the gamepad doesn't have a similar function (or didn't at the time of writing)... so the 'pressed' flags occur when a gamepad's 'down' event is triggered. this.systems.input.gamepad.on('down', this.gamepadButtonDown, this);

My guess is that the order of execution is such that the plugin's update function clears the pressed flag before your scene's update function gets to use it. Indeed, if you put a log in the plugin's update function, you can see the gamepad's 'pressed' flag there, though it's missing from your scene.

I haven't dug too far into Phaser to see exactly what's happening here. I've just found it easier to not put anything into the update function of the scene that houses the plugin... i.e. I have my inputController scene which runs along side my game scenes. The logic that actually interprits the input exists in various game scenes and interrogates the inputController scene. This works for me, as button presses do different things in different scenes. In your case I understand you've already done some abstraction and don't want to split that up further. I'm not sure how to solve your problem without doing that though! By all means, if you can dig about and come up with a workaround - presumably by having a better understanding of the order of execution than me - then please let me know!!

zewa666 commented 2 years ago

alright thanks for the hints. yep thats exactly what I've seen logged internally including pressed just not propagated.

I'll try to figure it out and worst case I can always switch to the approach mentioned by you.

GaryStanton commented 2 years ago

Let me know how it goes!

zewa666 commented 2 years ago

@GaryStanton as you mentioned it's about the call order and your update method fires too fast not letting the interactions pass to the consumer. So if we delay the reset for one iteration everything works smoothly.

This is a demo workaround I tried to verify my theory, obviously far from prod-ready:

diff --git a/src/main.js b/src/main.js
index d42f057..3a1f496 100644
--- a/src/main.js
+++ b/src/main.js
@@ -124,10 +124,13 @@ export default class MergedInput extends Phaser.Plugins.ScenePlugin {
        }, this);
    }

+   wasShown = [];
+
    update() {
        // Loop through players and manage buffered input
        for (let thisPlayer of this.players) {
-           if (thisPlayer.interaction.pressed != '') {
+           if (this.wasShown && thisPlayer.interaction.pressed != '') {
+               this.wasShown = false;
                thisPlayer.interaction.buffer = '';
            }
            if (thisPlayer.interaction.buffer == '') {
@@ -153,6 +156,10 @@ export default class MergedInput extends Phaser.Plugins.ScenePlugin {
        this.checkKeyboardInput();
        this.checkGamepadInput();
        this.checkPointerInput();
+
+       if (this.players[0]?.interaction.pressed) {
+           this.wasShown = true;
+       }
    }

    /**

So with this we're down to:

  1. Are there any larger side-effects using this behavior?
  2. Only enqueue it to next update if input !== keyboard?
  3. Is there a Phaser API for doing that instead my hacky workaround? I've found https://newdocs.phaser.io/docs/3.52.0/Phaser.Structs.ProcessQueue#add but don't really understand where to access the scene's processQueue instance
zewa666 commented 2 years ago

@GaryStanton one more thing. the up handler https://github.com/GaryStanton/phaser3-merged-input/blob/8cb8fcafaeb13a02006a8529d15af552ffa1a589/src/main.js#L457 of the gamepad is not defining the pressed interaction as the down one does. is that on purpose?

GaryStanton commented 2 years ago

@zewa666 Thanks for your investigation into this, it's been really helpful. What I've found is that the interactions caused by events, appear to occurr before the update() function is run - so when the code clears the values, that happens before we get a chance to interrogate the player object. Your approach works well... I updated it to work for each player object, and to take into account the device type. Specifically I did this by moving the buffer clearing logic into a separate function, and adding a flag to the interaction struct to indicate that the clearBuffer function should not be called until the next update.

This all worked perfectly, however when I pulled the changes into the game I'm currently working on, it resulted in the 'pressed' flag sticking around for two ticks of the scene's loop! I'm not sure why that would be - though it explains why I wasn't having the problem in my games even though it occurrs in the demo.

I've reverted the changes for now - I'm going to need to get to the bottom of why the parallel scene in my game is seeing the pressed flag twice before I can release the update.... and it's gone 2am now. ;)

In regard to the Phaser API... I had a look at processQueue, but I'm not sure how to use it or indeed if it's appropriate. Once I realised what was going on with the order of execution it seemed easy enough to resolve there.

As for the up handler, that was by design. I want to know specifically when a button has been pressed, i.e. to give the gamepads a justDown function similar to the keyboard... however, I can how you might want to use a justUp function too, so I've added a new 'released' flag to handle this. It'll be in the finished release once I've got to the bottom of this issue.

zewa666 commented 2 years ago

with regards to processQueue, scratch that, meanwhile I learned it is this.time.delayedCall(cycles) what I was looking for.

yes I think it would be great to have a simulated justUp as well and the released flag should definitely help there.

thanks for looking into this

GaryStanton commented 2 years ago

@zewa666

I think I've managed to get this to where we need it to be now... What I've ended up doing is migrate the keyboard interactions from the key's functions (justDown & justUp) to instead use the keyboard's keyDown & keyUp events. Since the Gamepad interactions are handled with events, and since those seem to fire before the update call is made, it makes sense to keep things consistent.

I also moved the existing logic in the update function to pre and post updates accordingly. This actually worked on its own, but in my game's setup, whereby I have my input controller on a separate scene; I found that the whole pre>update>post in the plugin completes before scene's update call... so migrating to events resolves this in both cases.

Finally, I added the released key in the interaction object to indicate that a button has been released, and replaced the last key with lastPressed and lastReleased. Hope that comes in handy for you.

Could I ask you to take a look at the dev branch and see if this is now working as you'd expect before I publish a new release? https://github.com/GaryStanton/phaser3-merged-input/tree/dev You can simply copy the contents of main.js over your local version if that's easier.

Once again, thanks for all your time and help with this - I think this is a real improvement to the inner workings of the plugin!

zewa666 commented 2 years ago

alright @GaryStanton. Wonderful work mate, everything works as expected. Thanks a lot for your efforts. This is really a great addition and makes using universal controls such a breeze

I've created a PR (https://github.com/GaryStanton/phaser3-merged-input/pull/21) targeting your dev branch, to update the typings with the new fields/methods, so if you could merge that before merging dev back to master that'd keep us TS user up-to-date as well ;)

GaryStanton commented 2 years ago

Thanks for the updated typings... I have to admit I still don't understand TypeScript, why those are needed, or why it's a good thing... but glad to be able to help more people use the plugin.

The v1.3.0 release is now live.

zewa666 commented 2 years ago

I guess something went south during merging your branch as the game now fails with this.clearBuffer is not a function and yes the function itself is missing. So I guess that was due to a merge conflict.

Why typings? Well exactly this kind of issue wouldn't have happened with TypeScript as it would have warned you in the editor or latest at compile time that there is no function clearBuffer ;)

GaryStanton commented 2 years ago

@zewa666 Thanks for the heads-up!! I've readded the code for another patch release... Alas, I still find Git difficult to work with, and this was actually my first time merging a branch. I'm more used to Mercurial - which is... better. There. I said it. ;)

GaryStanton commented 2 years ago

(and clearly I need to embrace TypeScript!)

zewa666 commented 2 years ago

Sure thing @GaryStanton.

Tested out the 1.3.1 release and it now works as expected.

Well with Git vs others it's pretty much like English and other spoken languages. There are so much more beautiful, rich and whatsoever languages, yet heck the whole world speaks English so better stick with it :)

If you want to get your first baby steps with type safety and work with VSCode, try adding // @ts-check at the top of a JS file. This will already point out a few gotchas and give you a better idea what's doable with stronger intellisense

GaryStanton commented 2 years ago

Yup, that's about the size of it... Github and the infrastructure surrounding it is second to none, but it means we have to work with Git. I know there are a lot of people who would say Git is the best DVCS, and I'm sure my bias is down to the years of experience I have with Mercurial... but I'd say Git's a lot harder to work with and easier to mess things up. But... I need Github and NPM, so... 🤷

Thanks for the tip, I'll give // @ts-check a go and see how I get on.