-
Notifications
You must be signed in to change notification settings - Fork 458
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
FlxTimerManager: fix array manipulation in onComplete #1954
Conversation
I've made mistake in my previous PR (inserted while loop in wrong place). This PR request also changes `completeAll()` method. Previous version of this method doesn't take into account the value of `active` property of timer. This could lead to infinite loop (maybe i'm wrong about this), because without check of `active` value it adds inactive timers to `timersToFinish` array, which won't finish (there is check in timer's update method). And thus i have a question: Do we need to activate paused timers and force them to finish or should we complete only active timers. I could add some parameter to `completeAll()` which will be responsible for this behavior, like `includeInactive:Bool = false`
My first instinct is "no", that would seem a little counter-intuitive. Does |
@Gama11 ok, let's leave this behavior for timer manager. About
|
Should we add check for tween's |
i don't understand why CI checks are failing |
|
i'll try tomorrow |
stupid question: how to run those unit tests? |
There's a Readme. ;) https://github.com/HaxeFlixel/flixel/blob/dev/tests/unit/README.md |
@Gama11 thanks! |
Fixed by changing timer's `update()` method. But timer `onComplete` callback is called after timer's property `finished` is set to true (which seems logical to me, this is similar to tween).
this patch adds check for tween's `active` value in `completeAll()` method od `FlxTweenManager` (just like in #1954) I think that we shouldn't finish tween if it's inactive. Should we add this check in tween's `update()` method as well (like in `FlxTimer` class)?
All checks are passing now |
@@ -281,12 +309,35 @@ class FlxTimerManager extends FlxBasic | |||
public function completeAll():Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... Does completeAll()
really need to be this complex? For tweens it's only 3 lines, this seems a little much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this doesn't really seem related to #679? Might be better to handle changes to completeAll()
in a separate pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more complex because docs about these methods says different things:
- in flxtween it says that onComplete callback is called only once
- in flxtimer - onComplete callback will be called
loopsLeft
times.
With your change there is should be onLoopFinished()
call, because timer's update()
method had been changed and doesn't contain this logic anymore
I've made mistake in my previous PR (inserted while loop in wrong place).
This PR request also changes
completeAll()
method. Previous version of this method doesn't take into account the value ofactive
property of timer. This could lead to infinite loop (maybe i'm wrong about this), because without check ofactive
value it adds inactive timers totimersToFinish
array, which won't finish (there is check in timer's update method).And thus i have a question: Do we need to activate paused timers and force them to finish or should we complete only active timers. I could add some parameter to
completeAll()
which will be responsible for this behavior, likeincludeInactive:Bool = false