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

VCR-FlxButton MOUSE_UP fix (closes #1729), VCR format improvement #1733

Closed
wants to merge 15 commits into from

Conversation

IBwWG
Copy link
Contributor

@IBwWG IBwWG commented Feb 19, 2016

This is working in my project correctly now. Hopefully in yours too :)

@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 19, 2016

Sorry, still getting used to git workflow, I forgot to rebase this after fixing my own dev branch. Should be good to go now.

@Gama11
Copy link
Member

Gama11 commented Feb 19, 2016

What do you mean by "VCR format improvement"? Just some code cleanup, or is the output different now? We can't really change the output, we need to remain compatible with old recordings.

@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 19, 2016

The output is different (smaller) but the loader remains able to load .fgr files that were made in the old format. So if I understood you correctly this is what you want.

All it does is not bother outputting frames that are identical to the previously outputted frame. To the new loader, the old format just has extra frames to load, but since such frames never caused anything to happen anyway, it makes no difference.

In fact, I'm pretty sure the old loader could load the new format, now that I look at it again. The loader itself I just cleaned up by making the "are we at the last line" check actually be based on the line number, rather than how many characters it has, and the player I only cleaned up extra braces in the end and didn't need to change anything else.

I tested this just now and (aside from mouseups not happening on playback) the old loader was able to play the new format just fine AFAICS.

@IBwWG
Copy link
Contributor Author

IBwWG commented Feb 23, 2016

Huh. Is the build failing because String isn't nullable on neko or something? (I'm not using neko myself, so I'm not super familiar with it.)

output += _frames[i++].save() + "\n";
curLine = _frames[i++].save();
if (curLine != null)
output += curLine + "\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens during playback if we skip save frames where nothing happens? Does the VCR recording properly wait for input during frames where nothing happens?

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 don't understand what you mean--you can't record during playback. Or were your two questions unrelated to each other? In that case your second question still doesn't make sense to me--what input are you waiting for on a frame where nothing happens? (Doesn't "nothing happens" mean no input occurs on that frame?)

IBwWG and others added 4 commits April 21, 2016 20:19
Catch up with upstream.
FlxTilemapTest: add a basic test for loadMapFromGraphic()
Catch up with upstream.
…ient while leaving loader backward-compatible.
@IBwWG IBwWG force-pushed the vcr-flxbutton-mouseup-fix branch from a848263 to b089ca5 Compare April 22, 2016 18:16
@IBwWG
Copy link
Contributor Author

IBwWG commented Apr 22, 2016

Conflicts here too? Maybe it was a bad idea to try separating these two VCR-related PRs.

# Conflicts:
#	flixel/input/mouse/FlxMouse.hx
@IBwWG
Copy link
Contributor Author

IBwWG commented Apr 22, 2016

Ah, OK, it was just the same conflicts as before. Hopefully this is good now?

@Gama11
Copy link
Member

Gama11 commented Apr 22, 2016

Seems that way.

Separating things makes it easier to review a pull request. This already does two separate things, changes the recording format and fixes a bug, adding a third wouldn't help.

Fix VCR to record simultaneous keys+mouse, closes HaxeFlixel#1739 (HaxeFlixel#1825)
@Gama11 Gama11 changed the title VCR-FlxButton MOUSE_UP fix, VCR format improvement VCR-FlxButton MOUSE_UP fix (closes #1729), VCR format improvement Apr 22, 2016
@IBwWG
Copy link
Contributor Author

IBwWG commented Apr 23, 2016

OK, this should be good to go now, too. I've also tested it recently with my project, so it works "in the wild" as well as in the unit test. :)

@Gama11 Gama11 added this to the 4.1.0 milestone May 11, 2016
@Gama11 Gama11 modified the milestones: 4.1.0, 4.2.0 Jul 10, 2016
@Gama11 Gama11 modified the milestones: 4.2.0, 4.1.0 Jul 10, 2016
@IBwWG
Copy link
Contributor Author

IBwWG commented Aug 10, 2016

What's this waiting on, exactly?

IBwWG and others added 2 commits August 10, 2016 08:13
Catch up with upstream.
…r before. Somehow between earlier catchup merges it got cut again.
@IBwWG
Copy link
Contributor Author

IBwWG commented Aug 10, 2016

There, in case it helped to update the base, this is caught up to dev...not sure what's with that import line, but anyway it's there again.

@IBwWG
Copy link
Contributor Author

IBwWG commented Aug 10, 2016

Ups, guess I tempted fate there. I don't understand what's broken though--no test errors, and build completed successfully, yet exit code 1? Also only on a few targets, not all? Reminds me of a popup I once got in Win98..."Error: the operation was a success." ;)

@IBwWG
Copy link
Contributor Author

IBwWG commented Aug 10, 2016

Maybe it's this?

PASSED

Tests: 271  Passed: 271  Failed: 0 Errors: 0 Ignored: 2 Time: 0.31342

X Error of failed request:  BadRequest (invalid request code or no such operation)

  Major opcode of failed request:  147 (GLX)

  Minor opcode of failed request:  168 ()

  Serial number of failed request:  173

  Current serial number in output stream:  173

Building coverage tests...

@IBwWG
Copy link
Contributor Author

IBwWG commented Aug 10, 2016

How can this change cause graphics driver trouble? Is this another one of those situations where the build system is broken and I just happened to push a change at the wrong moment? (A la @Gama11 your change to simply update the haxelib version, which broke the build here https://travis-ci.org/HaxeFlixel/flixel/jobs/150150706 with again no indication of any actual error, but an exit code of 1...although in your case I don't even see something like the "error of failed request" above)

@Gama11
Copy link
Member

Gama11 commented Aug 21, 2016

I don't know what's causing the Travis failures right now, but it's unrelated to your changes.

I'm having a hard time reviewing this, because it mixes a bugfix with formatting changes (which might not intend to break backwards compatibility, but definitely has the potential to, so it has to be tested).

Could you open two separate pull requests for these things?

@Gama11 Gama11 closed this Aug 21, 2016
Gama11 added a commit that referenced this pull request Sep 7, 2016
Partial / manual merge of @IBwWG's PR (#1733). Also added a unit test.
closes #1729
Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request 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

Successfully merging this pull request may close these issues.

3 participants