HaxeFlixel / flixel

Free, cross-platform 2D game engine powered by Haxe and OpenFL
https://haxeflixel.com/
MIT License
1.98k stars 438 forks source link

FlxTouch and FlxButton pressed being missed #1370

Open JoeCreates opened 9 years ago

JoeCreates commented 9 years ago

This seems to be most noticeable when using touch, for example, when touching FlxButtons. Button pressed are often missed, making the game feel unresponsive. How long you touch a button for makes a difference. It is only quick presses that get missed.

I have tried adding the following traces to FlxButton#onUpEventListener:

#if !FLX_NO_MOUSE
    private function onUpEventListener(_):Void
    {
        trace("ONUPLISTENER");
        if (visible && exists && active && status == FlxButton.PRESSED)
        {
            trace("handled");
            onUpHandler();
        }
    }
#end

With the above, a quick press on a button causes "ONUPLISTENER" to be traces, but not "handled". The reason is that the status is not yet PRESSED. If the button has not been updated before the touch has been released, it will likely be missed as the state is incorrect.

It looks like this could be due to the fact that Inputs can be pressed and released in a single update, and yet the input cannot simultaneously be in the states JUST_PRESSED and JUST_RELEASED. The rapid release is meaning that the JUST_PRESSED state is being missed.

Also, is it possible that an input is going to be updated before the button? If so the input could move from JUST_RELEASED to RELEASED before the button has had time to check if the input was just released, and thus the onUp callback will be skipped.

Gama11 commented 8 years ago

The fix for #1686 might have helped with this (2c9a545168). Not really sure how to test it.

JoeCreates commented 8 years ago

I don't think so. That seems to be an unrelated issue. @Tw1ddle and I fixed this on a private branch of flixel, but completely changes the states inputs can have, and so it is incompatible with Flixel's input recording.

It has to be possible for a button to be both justPressed and justReleased on the same frame.

This is a major issue for mobile devices (ours and other's mobile games have been rejected from app store for having unresponsive buttons, although submitting multiple times eventually gets it through).

I would suggest that the existing system is simply broken, but it is unfortunate that the recording is dependent on it. Perhaps we could merge our fixed version of FlxInput in and make it optional to use the old system for flxrecord?

Gama11 commented 8 years ago

I'm not sure it's only recordings that depend on this..

JoeCreates commented 8 years ago

@Gama11 Here's how it looks in our new version: https://gist.github.com/JoeCreates/aba33862ddfb75d3071b9e5d219878df

justPressedQueueFalse and justReleasedQueueFalse ensure the justPressed and justReleased states last for a frame.

We kept the public interface as close as possible to the existing interface. For the most part, existing classes extending FlxInput or implementing IFlxInput will work the same. The reason FlxRecord breaks is that it assumes an input only has one state in any given frame, and modifying this will break existing recordings.

sruloart commented 8 years ago

The VCR isn't even relevant for mobile (touches aren't being logged AFAIK, right?), so, this solution can be blocked within an #if FLX_TOUCH (thus not being a breaking change, simply a fix for a serious usability issue).

P.S And the VCR is just a glorified keylogger for the mouse and keyboard, I have no idea why it's even on the Flixel's core (probably for (pre)historic reasons). If something breaks that let it break I say :D

Gama11 commented 8 years ago

Mouse input works just fine on mobile.

sruloart commented 8 years ago

Not if we break it :D

And more specifically, does the recording system work on mobile?

and also, this: https://github.com/HaxeFlixel/flixel/issues/1739

JoeCreates commented 8 years ago

The issues arent actually mobile specific, its just more likely to manifest on mobile due to low frame rates on old devices and the fact that touches tend to happen faster than mouse clicks.

The primary way we tested this was on desktop, and we did it by setting the game to update at a very low frame rate.

You can, if you really try on a game with a lower frame rate, have the issue appear when using the mouse. You just have to click extra quick, e.g. by tapping the mouse button.

If it is provided as an option, I think it would make sense to have the fixed version be the standard and default. A new recording system could be written, but to support older games with old recordings there would still need to be the option to use the old version. On 4 Apr 2016 22:49, "Sruloart" notifications@github.com wrote:

Not if we break it :D

And more specifically, does the recording system work on mobile?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/HaxeFlixel/flixel/issues/1370#issuecomment-205512209

IBwWG commented 8 years ago

The old recording system is broken anyway, pending my PR.

Wouldn't it be most sensible to have pressed be a boolean all by itself, indicating the plain current state (which, in the case of a quick tap, would be false), then justPressed and justReleased would be two more independent booleans. This allows all possible combinations of reality, and a few non-sense that are not possible to get around:

p   jP  jR
t   t   t   doesn't happen
t   t   f   started pressing this frame
t   f   t   doesn't happen
t   f   f   started pressing before this frame
f   t   t   quick tap this frame
f   t   f   doesn't happen
f   f   t   stopped pressing this frame
f   f   f   stopped pressing before this frame or was never pressing

The "doesn't happen" cases should be handled accordingly (trace a warning indicating a bug?)

The other cases allow us to solve the current issue.

The lack of a released boolean removes even more impossible cases that would be possible with the gist (and maybe are currently), where something can be both pressed and released simultaneously. So we can implement it for backwards compatibility as a getter function returning !pressed.

As for the currently-broken-has-been-for-a-while-anyway recorder, it could be made to be backwards-compatible if we just use higher-numbered enums for each of the above combinations (except f-f-f, which can be 0) going forward, and map the old "2" to t-t-f in the table above.

IBwWG commented 8 years ago

In fact, maybe it would make sense to have an enum like this (for the five above combos that do happen):

justPressed
stillPressed
justPressedAndReleased
justReleased
notPressed

This could hold the actual state and be backwards-compatible with the current recorder and most current things via getter functions.

So depending on the incoming events for a given frame:

notPressed either becomes justPressed or justPressedAndReleased, or stays the same justPressed becomes either stillPressed or justReleased stillPressed either becomes justReleased or stays the same justPressedAndReleased and justReleased both have the same options as notPressed (because you could have multiple single-frame taps in a row; when done tapping you revert to notPressed)

Then the backwards-compatible getter functions would be:

justPressed: justPressed || justPressedAndReleased pressed: justPressed || stillPressed justReleased: justReleased || justPressedAndReleased released: notPressed || justReleased || justPressedAndReleased

JoeCreates commented 8 years ago

Its not quite as simple as that. My implementation does have a single flag for each, but the cases you present are not all the possible ones. Consider, for example, in a single frame an input is pressed, released and pressed again. The final state should have all three true and it is valid. Unlikely, sure, but still possible especially if there is a spike in fps. I thought I had already put a gist up but if not I'll post one when I'm back at my pc.

IBwWG commented 8 years ago

OK, fair enough. You did put up a gist, I referred to it (without a link)...or do you mean something else?

I guess then I would still propose 3 bools instead of 4 as an improvement, so that at least pressed and released are mutually exclusive.

Although, on second thought--the situation you presented is a good one to consider, and what about beyond that? What if the user taps and releases twice or three times within a frame? Especially in a gaming context this might be desirable to capture properly. In that case, I would go back to the 5-enum, and have an additional var completeTapsThisFrame:Int that keeps track of how many complete taps there were. So in the press-release-press example you gave, the state would be justPressed but completeTapsThisFrame would be 1. But applications would need to be pretty aware of this...backwards compatibility might not give you what you expect if you just compile a working desktop game for phones and encounter this for the first time. Isn't there already a multiple-gesture API for touch in flixel? Oh, that makes it even more complicated...how do you know what was two separate taps completed within a frame, and what was a multitouch gesture?

Maybe flixel should just forward down/up events on directly, playing them in order?

That, or it's just a known byproduct of having a low-FPS app...there's only so much you can/should try to do within a frame if your frames are already lagging. (I mean, it entirely depends on the application. If processing additional taps doesn't add too much more lag, then it's not like you're making a bad situation worse; but in some games you very well could be.)

IBwWG commented 8 years ago

Regarding your initial issue, though, I wonder if it's something with FlxButton in particular. To me that class always seemed a bit strange, since it uses OpenFL event handling but only for mouseups, and then also messes with FlxInput updating. (I had to do something similar in the VCR code mostly because of this messing--otherwise played back mouseclicks do not function at all on FlxButtons.) There is also the FlxMouseEventManager class...have you tried it out in context? If so, and if it's any better than FlxButton, maybe we could rewrite FlxButton to use FlxMouseEventManager instead of the way it currently does things.

Tw1ddle commented 8 years ago

@IBwWG You could queue events up and forward them on, or handle them immediately. That way you'd be able to process every event and this kind of problem wouldn't occur. However this boolean flags fix is a relatively simple change and fixes the most common problems.

About mouse ups on FlxButton, it was necessary to defer calls to onUpHandler until the updateStatus(input:IFlxInput) method to get it working how I wanted (I don't recall the exact problem I had though... :smile: ).

JoeCreates commented 8 years ago

@Tw1ddle I just saw the "hack" you added. Was this actually still necessary after the changes to FlxInput?

Tw1ddle commented 8 years ago

@JoeCreates I couldn't remember, so I made this demo... Try switching between the flixel dev submodule and the other branch in project.xml: https://github.com/Tw1ddle/FlixelInputBug

On current dev flixel, you reproduce the basic problem by spam clicking the button and noting any difference between the number of OpenFL mouseup events and the number times the button is triggered.

On the branch with the added *queue flags in FlxInput and the button mouseup deferral check, any single click over the course of a frame is guaranteed to be handled. Spam clicking still means events are dropped, but that's just how it works, since this implementation doesn't queue events up.

And if you remove the hacky extra fix I put in FlxButton, fast clicks on the button made within a single frame seem to get missed: https://github.com/Tw1ddle/flixel/commit/53e700250de9e4808ccd9589ac82ceacee667f0f

So yeah. This problem will crop up when slow devices will run games at a poor framerate, so mobile mainly.

JoeCreates commented 8 years ago

Just spoke to @Tw1ddle on skype. I have some information as to why this issue exists in FlxButton and also a suggestion for how to fix it.

The reason FlxButton is a special case is that apparently on flash certain operations can only be performed if the user initiated it (such as opening a new window https://github.com/Tw1ddle/flixel/blob/62bf66104f29ca1c3d514306015cb04ab97c322f/flixel/ui/FlxButton.hx#L518). To allow FlxButtons to do this extra code was added to make FlxButton use an onUpEventListener and perform the buttons actions after that. The problem with this is that it still checked flixel's PRESSED state on the button to ensure that the button had been clicked before the up event happened. Flixel may skip the PRESSED state if the input happens quickly at a slow framerate, so the previous condition in onUpEventListener would fail.

I don't know if this security issue mentioned is still a thing. If it is not, then this special button case should be removes. If it is still an issue, however, then a anDownEventListener could be added which sets a new "flash state" (as opposed to the flixel state) of the button to being down. This would be checked by onUpEventListener instead of flixel's PRESSED.

Gama11 commented 8 years ago

Flash security restrictions definitely haven't changed.

JoeCreates commented 8 years ago

@Gama11 Yeah, Sam confirmed this.

mastef commented 7 years ago

I think we're running into this on Androids and iPads on buttons that extend FlxButton. Quick presses on the button get ignored at a 30-40 fps framerate ( or when there's frame drops ).

What's the recommended solution currently? Any plans to fix it?

mastef commented 7 years ago

Spoke with @Tw1ddle in Slack about this, for anyone else looking for solutions these 2 patches should be of help :

FlxInput patch : https://github.com/Tw1ddle/flixel/commit/87d00c6fe7f957d0e832ac8562db9490624c661b FlxButton hack : https://github.com/Tw1ddle/flixel/commit/53e700250de9e4808ccd9589ac82ceacee667f0f#diff-75008da6c068aef2396448261140bc16