FlxCollision: fix an animation-related crash with renderBlit #1928

IBwWG · 2016-09-05T06:40:49Z

This needs a unit test, but in case anyone else was having flash crash on them during a collision check, before I get to that...

Catch up with upstream.

FlxTilemapTest: add a basic test for loadMapFromGraphic()

Catch up with upstream.

Fix VCR to record simultaneous keys+mouse, closes HaxeFlixel#1739 (HaxeFlixel#1825)

Catch up with upstream.

Add function to get sound length.

Revert "Add function to get sound length."

…oop, if you play an animation that has a larger frame size than the previously playing one. (The loop assumes that there are more pixels than there are and crashes trying to read past the end of the "file".) Therefore they shouldn't be limited to renderTile, but should also work on flash/blit.

Gama11 · 2016-09-05T07:22:06Z

Definitely need a way of reproducing the issue.

Gama11 · 2016-09-08T07:39:21Z

Any update on this one? In what situation does this fix a crash?

IBwWG · 2016-09-08T08:31:22Z

As the commit says, on flash target, "if you play an animation that has a larger frame size than the previously playing one" and then do a collision check. I'll do up a test case now.

Gama11 · 2016-09-08T08:35:17Z

Oh, right. I missed that commit comment.

I'm a bit worried about this change because this code is fairly performance-critical, and doing two drawFrame() calls each time seems like it'd impact things negatively. Might want to check if this has an impact on FPS in the PixelPerfectCollision demo.

IBwWG · 2016-09-08T08:37:32Z

I suppose we could first check whether the frame size changed, and only run it then.

IBwWG · 2016-09-08T08:55:54Z

Hmm, having trouble reproducing it. Very strange, I definitely needed this fix for my project.

IBwWG · 2016-09-08T09:08:32Z

Ah, OK it's even a bit more specific, it seems. Got it breaking now.

IBwWG · 2016-09-08T09:16:02Z

Here's a repro, I'm just turning it into a test case:

package;

import flash.display.BitmapData;
import flash.geom.Matrix;
import flash.geom.Point;
import flash.geom.Rectangle;
import flixel.FlxSprite;
import flixel.FlxState;
import flixel.graphics.frames.FlxAtlasFrames;
import flixel.util.FlxCollision;
import flixel.util.FlxColor;

class PlayState extends FlxState{
    override public function create():Void {
        var sprite:FlxSprite = cast add(new FlxSprite(30, 0));
        sprite.makeGraphic(100, 100, FlxColor.WHITE, true);
        var sourceSprite = new FlxSprite();
        sourceSprite.makeGraphic(100, 100, FlxColor.RED, true);
        var animatedSprite:FlxSprite = cast add(new FlxSprite());
        // load animations with varying frame sizes
        animatedSprite.frames = FlxAtlasFrames.fromLibGdx(sourceSprite.pixels, '
test.png
size: 100,100
format: RGBA8888
filter: Nearest,Nearest
repeat: none
a/0
  rotate: false
  xy: 0, 0
  size: 10, 10
  orig: 10, 10
  offset: 0, 0
  index: -1
b/0
  rotate: false
  xy: 90, 90
  size: 20, 20
  orig: 50, 50
  offset: 40, 0
  index: -1
');
        // add animations
        for (name in ["a","b"])
            animatedSprite.animation.addByPrefix(name, name);
        // play smaller-framed anim
        animatedSprite.animation.play("a");
        trace(FlxCollision.pixelPerfectCheck(sprite, animatedSprite));
        // play larger-framed anim
        animatedSprite.animation.play("b");
        // run PPC
        trace(FlxCollision.pixelPerfectCheck(sprite, animatedSprite));
    }



}

IBwWG · 2016-09-08T09:23:57Z

Gee, it'd help if the test cases ran before my tests. How'd this get committed?

C:\HaxeToolkit\haxe\lib\flixel\git\tests\unit>lime test flash
src/TestSuite.hx:32: characters 7-50 : Type not found : flixel.system.frontEnds.
DebuggerFontEndTest
src/TestMain.hx:15: lines 15-64 : Defined in this class

font should be front in both places...

But even with that for some reason it's not working:

C:\HaxeToolkit\haxe\lib\flixel\git\tests\unit>lime test flash
src/TestSuite.hx:96: characters 6-12 : Unknown identifier : flixel

C:\HaxeToolkit\haxe\lib\flixel\git\tests\unit>lime test flash
massive.munit.MUnitException: ExternalInterface not available at massive.munit.c
lient.ExternalPrintClientJS#new (320)
        at massive.munit.client::ExternalPrintClientJS()
        at massive.munit.client::RichPrintClient/init()
        at massive.munit.client::AbstractTestResultClient()
        at massive.munit.client::PrintClientBase()
        at massive.munit.client::RichPrintClient()
        at TestMain()
        at TestMain$/main()
        at Function/http://adobe.com/AS3/2006/builtin::apply()
        at Function/<anonymous>()
        at ApplicationMain$/start()
        at ApplicationMain$/init()
        at lime.app::Event_Void_Void/dispatch()
        at lime.app::Preloader/start()
        at openfl.display::Preloader/display_onComplete()
        at flash.events::EventDispatcher/dispatchEventFunction()
        at flash.events::EventDispatcher/dispatchEvent()
        at NMEPreloader/onLoaded()
        at openfl.display::Preloader/start()
        at lime.app::Preloader/current_onEnter()

Am I running the wrong command?

…is check...

IBwWG · 2016-09-08T09:27:38Z

Er, OK, I try lime test windows and instead it complains about front and likes font instead. I'm not sure how the test suite gets out of sync with the rest of the codebase when I pull updates?

Gama11 · 2016-09-08T09:31:20Z

Try using these files: https://github.com/HaxeFlixel/flixel/tree/dev/tests/unit/targets

They run munit gen, which regenerates the TestSuite.hx file.

IBwWG · 2016-09-08T09:34:30Z

Sorry, what command should I use?

I got it completed with lime test windows so at least the test as-is works. Travis should show it passing. The test is then, remove my changes to FlxCollision and then the test should fail.

Gama11 · 2016-09-08T09:41:14Z

https://github.com/HaxeFlixel/flixel/blob/dev/tests/unit/README.md

IBwWG · 2016-09-08T09:41:51Z

Missed that layer. Thanks!

I guess also I was wrong earlier about confirming the passing, if it didn't regen the suite, it won't include my new test.

IBwWG · 2016-09-08T09:44:05Z

Yeah, that'll surely fail. Forgot to finish the before section entirely.

Gama11 · 2016-09-08T09:45:23Z

tests/unit/src/flixel/util/FlxCollisionTest.hx

+		var animatedSprite:FlxSprite = cast add(new FlxSprite());
+		// play smaller-framed anim
+		animatedSprite.animation.play("a");
+		trace(FlxCollision.pixelPerfectCheck(sprite, animatedSprite));


This doesn't look like it should even compile...

Hence 'that'll surely fail' :)

Nice having a working test suite so I can actually attempt to compile locally. I had looked for the docs on the haxeflixel.com site and seen nothing there, so I thought it was just assumed you know how to do it, and I had forgotten how. I forgot you can have nested readme.md's within a project and to look for that.

… case, but to avoid unnecessary paint() calls otherwise.

Gama11 · 2016-09-08T09:50:12Z

flixel/util/FlxCollision.hx

@@ -18,6 +18,7 @@ import flixel.tile.FlxTileblock;
 * @link http://www.photonstorm.com
 * @author Richard Davey / Photon Storm
 */
+@:access(flixel.FlxSprite)


Hm, not a fan of this... Have you checked whether it makes a noticable performance difference to call drawFrame()? Thinking about it some more, it might actually not matter much, since it's a non-op if the sprite isn't dirty.

I haven't done a performance test. Oh, I see...that's interesting, I had misread drawFrame()'s if statement. I guess in this case it's properly marked dirty when it needs to be, otherwise this wouldn't be fixing anything.

Gama11 · 2016-09-08T09:56:01Z

Hm, how is it that your test case looks different on Flash than on Neko? (even with the fix)

IBwWG · 2016-09-08T09:58:38Z

Feel free to file a new issue :) I almost never use neko, it seems to just require even more adjustments to all my function calls than flash does, and not provide much benefit to me over flash.

Gama11 · 2016-09-08T10:01:57Z

Well, it's not exactly Neko-specific, looks the same on cpp...

Gama11 · 2016-09-08T10:03:17Z

tests/unit/src/flixel/util/FlxCollisionTest.hx

+	}
+
+	@Test
+	function pPCOnAnim()


pPCO? What's that supposed to mean?

IBwWG · 2016-09-08T10:04:08Z

Ah, to be fair, it's bad input, I just noticed. I should probably change that... (the offset, size, and orig don't make sense together in b/0.)

IBwWG · 2016-09-08T10:07:43Z

Although, even with that fixed, it still looks wrong. Try setting the offset to 30,0 and you'll see the resulting size is still 10x10 instead of 20x20.

Gama11 · 2016-09-08T10:08:21Z

Different bug? :)

IBwWG · 2016-09-08T10:10:41Z

I think it's definitely a different bug. Maybe related to the cause of #1936, not sure.

Added #1937 for this.

IBwWG · 2016-09-08T10:11:15Z

tests/unit/src/flixel/util/FlxCollisionTest.hx

@@ -36,7 +37,7 @@ b/0
  xy: 90, 90
  size: 20, 20
  orig: 50, 50
-  offset: 40, 0
+  offset: 30, 0


Forgot to mention this in commit msg. Adjust so this is legal input.

If you comment out these lines, and just run drawFrame() no matter what, on my system it's about 9% slower.

IBwWG · 2016-09-08T10:22:07Z

It's quite odd I didn't notice that bug with my own project, you'd think that would've broken something...

Anyway, hopefully Travis passes and this is all good now.

Gama11 · 2016-09-08T11:13:40Z

I'm gonna double-check the performance implications of always running drawFrame().

The current workaround is too hacky for my taste... (target-specific conditionals, but especially access to private fields...). If it's indeed necessary not to always run drawFrame(), there might be a better / less hacky workaround of doing that.

IBwWG · 2016-09-08T11:21:37Z

Maybe it's not target-specific but renderBlit-specific, like you mentioned in the other issue. I haven't tried this on html5. I suppose you could have it always run drawFrame() at this point in the code, and then make sure that animation.play() or whatever changes the current animation checks the frame size and marks the sprite dirty if the size changed.

Gama11 · 2016-09-10T19:19:55Z

@Beeblerox Do you have an idea how to fix this in a less hacky way?

Beeblerox · 2016-09-11T16:36:06Z

flixel/util/FlxCollision.hx

 		if (FlxG.renderTile)
+	#end


we don't need both checks:

#if flash if (Contact._frame.sourceSize.x != Contact.framePixels.width || Contact._frame.sourceSize.y != Contact.framePixels.height || Target._frame.sourceSize.x != Target.framePixels.width || Target._frame.sourceSize.y != Target.framePixels.height) #else if (FlxG.renderTile) #end

so we just call Contact.drawFrame(); and Target.drawFrame();.

drawFrame() (and following methods which are called by it) have checks for value of dirty flag, so if sprite doesn't need to update its frame graphic then it will skip this step.
The only perfomance problem is with animated sprites when animation could have series of the same frames: when animation changes the frame (even if it has the same index) then it automatically resets sprite's frame, whih set dirty to true

Seems you're right, I couldn't notice any FPS decrease in the PixelPerfectCollision demo when just always calling drawFrame.

closes HaxeFlixel#1928

IBwWG and others added 12 commits April 21, 2016 20:19

Merge pull request #16 from HaxeFlixel/dev

a197f5c

Catch up with upstream.

Merge pull request #22 from HaxeFlixel/dev

262eabb

FlxTilemapTest: add a basic test for loadMapFromGraphic()

Merge pull request #24 from HaxeFlixel/dev

2966605

Catch up with upstream.

Merge pull request #27 from HaxeFlixel/dev

dedca7c

Fix VCR to record simultaneous keys+mouse, closes HaxeFlixel#1739 (HaxeFlixel#1825)

Merge pull request #29 from HaxeFlixel/dev

072baa7

Catch up with upstream.

Merge pull request #31 from HaxeFlixel/dev

119aa76

Catch up with upstream.

Merge pull request #33 from HaxeFlixel/dev

cbf0dc6

Catch up with upstream.

Add function to get sound length.

1015fed

Merge pull request #38 from IBwWG/gettable-sound-length

f372e05

Add function to get sound length.

Revert "Add function to get sound length."

6a8abc0

Merge pull request #39 from IBwWG/revert-38-gettable-sound-length

675985a

Revert "Add function to get sound length."

Since I can't get the tests working locally in general, I'll let Trav…

6869e3d

…is check...

Gama11 reviewed Sep 8, 2016
View reviewed changes

Only dedraw on flash if frame size changed, to avoid crashing in that…

ae77d68

… case, but to avoid unnecessary paint() calls otherwise.

Gama11 added the Bug label Sep 8, 2016

Finish test cleanup so that it actually compiles.

f928be3

Gama11 reviewed Sep 8, 2016
View reviewed changes

Appease CodeClimate.

587d8c1

Gama11 reviewed Sep 8, 2016
View reviewed changes

tests/unit/src/flixel/util/FlxCollisionTest.hx

}

@Test

function pPCOnAnim()

Copy link

Member

Gama11 Sep 8, 2016

Choose a reason for hiding this comment

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

pPCO? What's that supposed to mean?

Clarify test names, get rid of teardown.

050b8d1

IBwWG reviewed Sep 8, 2016
View reviewed changes

Fix frame check to check the right parameter.

0d1a9f7

If you comment out these lines, and just run drawFrame() no matter what, on my system it's about 9% slower.

Gama11 added this to the 4.2.0 milestone Sep 9, 2016

Beeblerox reviewed Sep 11, 2016
View reviewed changes

Gama11 changed the title ~~Fix bigger anim flash crash~~ FlxCollision: fix a crash with renderBlit and differently-sized animation frames Sep 11, 2016

Gama11 changed the title ~~FlxCollision: fix a crash with renderBlit and differently-sized animation frames~~ FlxCollision: fix an animation-related crash with renderBlit Sep 11, 2016

Gama11 closed this in 1b2c30d Sep 11, 2016

Aurel300 pushed a commit to larsiusprime/haxeflixel that referenced this pull request Apr 18, 2018

FlxCollision: fix an animation-related crash with renderBlit

a04f1f6

closes HaxeFlixel#1928

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FlxCollision: fix an animation-related crash with renderBlit #1928

FlxCollision: fix an animation-related crash with renderBlit #1928

IBwWG commented Sep 5, 2016

Gama11 commented Sep 5, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016 •

edited

Loading

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 Sep 8, 2016

IBwWG Sep 8, 2016 •

edited

Loading

Gama11 Sep 8, 2016

IBwWG Sep 8, 2016

Gama11 commented Sep 8, 2016 •

edited

Loading

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

Gama11 Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016 •

edited

Loading

IBwWG Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 10, 2016

Beeblerox Sep 11, 2016

Gama11 Sep 11, 2016

FlxCollision: fix an animation-related crash with renderBlit #1928

FlxCollision: fix an animation-related crash with renderBlit #1928

Conversation

IBwWG commented Sep 5, 2016

Gama11 commented Sep 5, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016 • edited Loading

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 Sep 8, 2016

Choose a reason for hiding this comment

IBwWG Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

Gama11 Sep 8, 2016

Choose a reason for hiding this comment

IBwWG Sep 8, 2016

Choose a reason for hiding this comment

Gama11 commented Sep 8, 2016 • edited Loading

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

Gama11 Sep 8, 2016

Choose a reason for hiding this comment

IBwWG commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016 • edited Loading

IBwWG Sep 8, 2016

Choose a reason for hiding this comment

IBwWG commented Sep 8, 2016

Gama11 commented Sep 8, 2016

IBwWG commented Sep 8, 2016

Gama11 commented Sep 10, 2016

Beeblerox Sep 11, 2016

Choose a reason for hiding this comment

Gama11 Sep 11, 2016

Choose a reason for hiding this comment

IBwWG commented Sep 8, 2016 •

edited

Loading

IBwWG Sep 8, 2016 •

edited

Loading

Gama11 commented Sep 8, 2016 •

edited

Loading

IBwWG commented Sep 8, 2016 •

edited

Loading