HaxeFlixel / flixel

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

FlxTimer cannot be restarted from its callback (regression in 4.2.0) #2002

Closed ttencate closed 7 years ago

ttencate commented 7 years ago

Code snippet reproducing the issue:

class PlayState extends FlxState
{
  private var myTimer: FlxTimer;

  override public function create() {
    super.create();
    myTimer = new FlxTimer();
    timerElapsed(myTimer);
  }

  private function timerElapsed(timer: FlxTimer) {
    doSomething();
    timer.start(FlxG.random.float(5, 10), timerElapsed);
  }
}

Expected behavior: The timer fires once every 5-10 seconds. Observed behavior: The timer never fires.

I think this might be a recently introduced bug, because this code used to work fine in 4.1.0 and now in 4.2.0 it doesn't. I suspect https://github.com/HaxeFlixel/flixel/commit/b8b93316cbed1dd1910ea3e7ad1266ca6bd9dff8, which calls cancel() right after invoking the onComplete() if the timer is finished.

Beeblerox commented 7 years ago

@ttencate it should be fixed on the dev branch. Can you check it?

ttencate commented 7 years ago

Yes, fixed, thank you!

I initially had a hard time verifying this because it turned out (unlike in the above repro case) I was also creating FlxTimer objects during FlxState construction, rather than from the create() method. I wish that would either work, or throw an exception, rather than silently creating broken objects (FlxTimer is not the only class suffering from this). Oh well.

Gama11 commented 7 years ago

I was trying to write a unit test for this, but I couldn't even reproduce the original issue when going back to 4.2.0. Am I missing something?

package;

import flixel.FlxState;
import flixel.util.FlxTimer;

class PlayState extends FlxState
{
   private var myTimer: FlxTimer;

  override public function create() {
    super.create();
    myTimer = new FlxTimer();
    timerElapsed(myTimer);
  }

  private function timerElapsed(timer: FlxTimer) {
    trace("timer complete");
    timer.start(1, timerElapsed);
  }
}
PlayState.hx:17: timer complete
PlayState.hx:17: timer complete
PlayState.hx:17: timer complete
PlayState.hx:17: timer complete
PlayState.hx:17: timer complete
...
Beeblerox commented 7 years ago

@Gama11 after your comment i've checked FlxTimer one more time and you're right - it works with 4.2.0 (the logic of timer allows to restart timer in onComplete handler without my change, but my commit shouldn't break anything, at least i think so). It seems that @ttencate had this issue because he created timer in state's constructor, not in the create() method. so i think there wasn't issue with FlxTimer itself

ttencate commented 7 years ago

Could be right. My apologies!

On Dec 4, 2016 13:48, "Zaphod" notifications@github.com wrote:

@Gama11 https://github.com/Gama11 after your comment i've checked FlxTimer one more time and you're right - it works with 4.2.0 (the logic of timer allows to restart timer in onComplete handler without my change, but my commit shouldn't break anything, at least i think so). It seems that @ttencate https://github.com/ttencate had this issue because he created timer in state's constructor, not in the create() method. so i think there wasn't issue with FlxTimer itself

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/HaxeFlixel/flixel/issues/2002#issuecomment-264702045, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFjMmIONyByaSbn2vb9IOuG5mTBnuKpks5rEraRgaJpZM4K7ozB .