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

FlxButton mouseups don't occur during VCR recording/playback #1729

Closed
IBwWG opened this issue Feb 19, 2016 · 7 comments
Closed

FlxButton mouseups don't occur during VCR recording/playback #1729

IBwWG opened this issue Feb 19, 2016 · 7 comments

Comments

@IBwWG
Copy link
Contributor

IBwWG commented Feb 19, 2016

  • Flixel version: dev
  • OpenFL version: 3.6.0
  • Lime version: 2.9.0
  • Affected targets: flash, probably all?

Code snippet reproducing the issue:

// in update():
if (FlxG.mouse.justPressed) {
    trace( 'mouse down at ', FlxG.mouse.getWorldPosition() );
}
if (FlxG.mouse.justReleased) {
    trace( 'mouse up at ', FlxG.mouse.getWorldPosition() );
}
// and also the typical recording/playback code, e.g. from the Replay demo

Extra steps to reproduce:
When you record, make sure to click without moving the cursor position at all.


Observed behavior: Your .fgr will contain lines like this:

84km478,95,2,0
85km478,95,1,0
86km478,95,1,0
87km
88km
89km
90km

Thus, on playback, the above code snippet does not trace any mouseups, even though during recording it does.

Expected behavior: The .fgr should contain one more update to trigger a mouseup, I think:

84km478,95,2,0
85km478,95,1,0
86km478,95,1,0
87km478,95,-1,0
88km
89km
90km

Alternatively, the playback function should somehow set FlxG.mouse.justReleased at 87km when it sees that the mouse is no longer pressed. I think it's "more correct" to record the mouseup, but maybe it's easier to fix playback than it is to fix recording.

Upon further investigation, it seems that this behaviour is quite intentional. FlxMouse.hx has:

        if ((_lastX == _globalScreenX) && (_lastY == _globalScreenY) 
            && (_leftButton.released) && (_lastWheel == wheel))
        {
            return null;
        }

So far I've traced that intention back at least three years (well, almost--as far back as the repo seems to go, Aug 2013), but I don't understand the rationale behind it.

In fact, it breaks FlxButton behaviour, which depends on mouseups to function properly. I just tried this case (clicking a FlxButton without moving the cursor between mousedown and mouseup) and it does indeed seem broken: during recording the button records a press, but during playback it doesn't.

So, assuming there is a good reason for having included that snippet in FlxMouse in the first place, and assuming it's still valid, there's a bit of a conflict with how FlxButtons work (not to mention my own update code, which does similar things to FlxButtons.) Depending on what that reason is, maybe we could add a flag somewhere to allow disabling this behaviour?

@IBwWG IBwWG changed the title VCR doesn't record mouseups unless cursor moved (By design) VCR doesn't record mouseups unless cursor moved...why? Feb 19, 2016
@IBwWG IBwWG changed the title (By design) VCR doesn't record mouseups unless cursor moved...why? (By design) VCR doesn't record mouseups unless cursor moved, causing flakey FlxButton playback...why? Feb 19, 2016
@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 19, 2016

After seeing the effect of disabling that FlxMouse code, I'm guessing that the intention was to reduce the file size. (If that's true, there's maybe a better way we could implement, which would be to not record a line at all if it's exactly the same as the previous line other than the timestamp. In fact, I have a patch ready, but it also still exhibits the behaviour described next.)

But now I'm seeing something new, that sometimes a mouseup is actually recorded, with a -1, and sometimes not. So far the only pattern I see is that clicks on FlxButtons suffer from this, but other sprites don't.

Meanwhile I also found this further code to suppress mouseups, in FlxMouseButton.hx:

    public function onUp(?_):Void
    {
        #if !FLX_NO_DEBUG
        if ((FlxG.debugger.visible && FlxG.game.debugger.hasMouse) 
            #if (FLX_RECORD) || FlxG.game.replaying #end)
        {
            return;
        }
        #end

        release();
    }

After some testing with trace functions in onUp() I realize this function isn't actually called when the mouse button is released, but when it moves outside the window. It does seem from the event listener add that it's actually intended for mouseup events rather than mouseout events, so...is this broken?

@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 19, 2016

It turns out it's this line that causes this behaviour, by design.

I think I have a workaround that we can use. For inputs that swallow the JUST_RELEASED state by calling release(), we can just compare the last value with the current, and have it be recorded as JUST_RELEASED even if it got changed to RELEASED.

The workaround and the shorter-recording patch work in the sense that they record the -1 now correctly. A second workaround is also needed, though, because an actual MouseEvent.MOUSE_UP event is not fired, which FlxButton and presumably others depend on.

I have a working patch. Will submit soon.

IBwWG pushed a commit to codingthat/flixel that referenced this issue Feb 19, 2016
…ient while leaving loader backward-compatible.
IBwWG pushed a commit to codingthat/flixel that referenced this issue Feb 19, 2016
…ient while leaving loader backward-compatible.
@IBwWG IBwWG changed the title (By design) VCR doesn't record mouseups unless cursor moved, causing flakey FlxButton playback...why? FlxButton mouseups don't occur during VCR recording/playback Feb 20, 2016
@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 23, 2016

Although, this workaround, and some recent experiments with keyboard-mashing, have me wondering if the VCR would work better overall if it processed events at the openfl level for recording and playback. But that would probably be a breaking change...

IBwWG pushed a commit to codingthat/flixel that referenced this issue Apr 22, 2016
…ient while leaving loader backward-compatible.
@Gama11 Gama11 added the Bug label Apr 22, 2016
@Gama11
Copy link
Member

Gama11 commented Sep 7, 2016

Hm.. this is interesting: I went back to AS3 Flixel and checked how this works there. Well, it doesn't actually...

@Gama11 Gama11 closed this as completed in 51963cf Sep 7, 2016
@Gama11
Copy link
Member

Gama11 commented Sep 7, 2016

I merged your fix manually. Also added a unit test. :)

@mastef
Copy link

mastef commented Sep 7, 2016

\o/ 💯 Did some testing here and it fixed the buttons issue, and then @Gama11 was kind enough to do the merging :-)

@IBwWG
Copy link
Contributor Author

IBwWG commented Sep 7, 2016

Thanks folks, I was focusing on getting our game released (hopefully tomorrow 1.0 goes live) so I didn't have a chance to catch up on flixel-related stuff just yet.

Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this issue Apr 18, 2018
Partial / manual merge of @IBwWG's PR (HaxeFlixel#1733). Also added a unit test.
closes HaxeFlixel#1729
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

No branches or pull requests

3 participants