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

Fix VCR to record simultaneous keys+mouse #1825

Merged
merged 4 commits into from
Apr 22, 2016

Conversation

IBwWG
Copy link
Contributor

@IBwWG IBwWG commented Apr 22, 2016

C.f. #1740, this is a fresh branch redo. Tests pass locally on cpp.

var key:Int;
var x:Int;
var y:Int;
var possibleKeys = [8, 9, 13, 16, 17, 18, 20, 27, 32, 33, 34, 35, 36, 37, 38, 39, 40, 45, 46, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 109, 110, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 186, 187, 188, 189, 190, 191, 219, 221, 222, 301];
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit silly... You could probably use FlxKey.toStringMap.keys() (as long as -1 and -2 are excluded).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also backslash and graveaccent, so you don't bring up the debugger and throw off other tests? This test ended up throwing off the flxbutton test because it moved the mouse, so I guess they're not set up to be independent, exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Really? That's weird.. There should be a FlxG.resetGame() call after each test class at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I went to do it as you said, but that returns an iterator. I then learned a new trick, thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

An iterator can be converted to an array fairly easily.

Copy link
Contributor Author

@IBwWG IBwWG Apr 22, 2016

Choose a reason for hiding this comment

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

I know that now ("I then learned..."), see my latest commit. :)

@Gama11 Gama11 added the Bug label Apr 22, 2016
@Gama11 Gama11 added this to the 4.1.0 milestone Apr 22, 2016
@@ -14,6 +17,7 @@ class FlxButtonTest extends FlxTest
{
button = new FlxButton();
destroyable = button;
FlxG.mouse.playback(new MouseRecord(0, 0, FlxInputState.RELEASED, 0)); // put the mouse in the upper-left corner to prepare for highlight testing
Copy link
Member

Choose a reason for hiding this comment

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

Why is this suddenly necessary?

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 would not have thought it so, except that after starting from the current dev branch, this test was failing. After resetting the cursor, it was succeeding.

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 guess resetGame() either doesn't run between the tests, or it does, but doesn't touch the cursor...?

Copy link
Member

@Gama11 Gama11 Apr 22, 2016

Choose a reason for hiding this comment

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

Hm, yeah.. I thought resetGame would completely recreate all instances in FlxG, guess it's actually far from that. All this global stuff is pretty annoying for unit testing... :/

We should at least handle the mouse reset in FlxTest#resetGame(), so it's not just a random hack, but happens consistently. Perhaps we can even manually call FlxG.init(), so things really get reset (new FlxMouse instance for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting. FlxTest#resetGame() calls FlxG.resetGame(), which eventually should mean hitting FlxG.reset(), which calls input.reset(), which ought to, ya know, reset the inputs, I guess. But actually FlxMouse's reset() just resets the mouse button states. I guess that makes sense, otherwise you might have a weird cursor jump for the user, but I'm not sure how resetGame() gets used in the wild anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Could you be a bit more specific than "causing trouble"?

Copy link
Contributor Author

@IBwWG IBwWG Apr 22, 2016

Choose a reason for hiding this comment

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

Sorry, see edit. It was the trouble I'd already described (crashing, other tests failing) but there's more detail now. Also re: hang vs crash, sometimes there's a window that comes up, with a Windows crash, or the first time I saw it, maybe I just didn't see the window, but from the CLI, the test runner was just sitting there forever (same output minus the Error:...)

Copy link
Contributor Author

@IBwWG IBwWG Apr 22, 2016

Choose a reason for hiding this comment

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

I just tried a new approach:

        Lib.current.stage.removeChild(FlxG.game);
        FlxG.game = null;
        Lib.current.stage.addChild(new FlxGame(640, 480, FlxState, 1, 60, 60, true));

Which works better: the FlxButtonTest and the others that broke now pass. However, it still makes the FlxReplayTest fail with accumulated keys. I find this strange, but we're getting closer...

I guess there's no way to clean up FlxG's static stuff, or clean up at all, since it's meant to just evaporate when the game quits, right? As TestMain.hx says,

// Flixel was not designed for unit testing so we can only have one instance for now.

So we have to figure out how to clean up that one instance...

Copy link
Member

Choose a reason for hiding this comment

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

That seems to have a pretty big impact on the runtime of the tests. Perhaps it's best to keep it as simple as we need it for now - we could just create a new FlxMouse instance manually. There's even already a setter that handles all the details of that since we needed that in flixel-ui at some point.

Copy link
Contributor Author

@IBwWG IBwWG Apr 22, 2016

Choose a reason for hiding this comment

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

Or, I could just make the final playback frame in the flxrecord test put things back where they were. It's easier and amounts to the same thing in the end, resetting it to 0,0 and with no buttons down.

@IBwWG
Copy link
Contributor Author

IBwWG commented Apr 22, 2016

Tests pass locally.

@Gama11
Copy link
Member

Gama11 commented Apr 22, 2016

Thanks for your patience!

@Gama11 Gama11 merged commit 71cdf01 into HaxeFlixel:dev Apr 22, 2016
@IBwWG IBwWG deleted the new-vcr-skm-fix branch April 22, 2016 20:14
IBwWG added a commit to codingthat/flixel that referenced this pull request Apr 22, 2016
Fix VCR to record simultaneous keys+mouse, closes HaxeFlixel#1739 (HaxeFlixel#1825)
Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018
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.

2 participants