feat: Add isFirstFrame and onStart event to SpriteAnimation

[munsterlander]

Description

This adds isFirstFrame and onStart event callback to sprite_animation.dart. May not be useful for everyone but I needed to trigger another event whenever an animation was called as multiple factors could trigger that animation.

Checklist

• The title of my PR starts with a [Conventional Commit] prefix (fix:, feat:, docs: etc).
• I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
• I have updated/added tests for ALL new/updated/fixed functionality.
• I have updated/added relevant documentation in docs and added dartdoc comments with ///.
• I have updated/added relevant examples in examples.

Breaking Change

• Yes, this is a breaking change.
• No, this is not a breaking change.

[munsterlander]

Not certain what format is off....

[spydon]

Not certain what format is off....

Just run flutter format . and it will will it for you.
(or melos run format if you have installed melos)

[munsterlander]

Not certain what format is off....

Just run flutter format . and it will will it for you. (or melos run format if you have installed melos)

Ran and updated. I am so used to that being ran in VS Code that I forgot to do it when I did all of this in Android Studio.

[alestiago]

@munsterlander would an API as #1552 solve your issue?

For example, would adding a listener to the controller to check when it starts suffice?

[munsterlander]

I'm not certain but at first glance, I don't think it would, at least not without additional code to determine where the animation is at.

[alestiago]

@munsterlander I believe you can listen to the value an determine where the animation is at.

I'm on the phone right now but perhaps in the upcoming week I can provide some code to show that functionality.

[munsterlander]

@munsterlander I believe you can listen to the value an determine where the animation is at.

I'm on the phone right now but perhaps in the upcoming week I can provide some code to show that functionality.

That's what I meant. I would need one listener with a switch statement and now would be math, i.e. animation.length-currentFrame==0 then it's the end, if it's 0 then the beginning and every where else in between.

Currently, it's just clean defined callbacks. I mean the api route will work - not saying it won't - but it's more verbose and less intuitive.

Ultimately, I will go which ever way you all want to go though.

[alestiago]

I will look into this, but wouldn't controller.value == 0 be enough? You could also check against lowerBound or upperBound.

I'll soon make a sample code to actually see how the implementation would be. Then, if you don't mind, I'll ask for your feedback, since it's very helpful.

Thanks a lot!

[munsterlander]

I will look into this, but wouldn't controller.value == 0 be enough? You could also check against lowerBound or upperBound.

I'll soon make a sample code to actually see how the implementation would be. Then, if you don't mind, I'll ask for your feedback, since it's very helpful.

Thanks a lot!

For sure and I will gladly test. I can go either way, but I liked the callbacks because it was set and forget. With a listener, I need to have the switch statement for the current frame as compared to other values and this means my onStart and onComplete code is now comingled. That is where my fear of making things more complex vs what do we gain type issue. I really like single scope functions and not multi function but multifunction can be more versatile, so...

[alestiago]

@munsterlander what do you think of:

 _controller.addStatusListener((status) {
  if (status == AnimationStatus.forward) {
    // Started
  } else if (status == AnimationStatus.completed) {
    // Completed
  }
});

I think you can also do:

_controller.forward().whenComplete(...)

Would the above work for your usecase?

I think this suggested approach is more flexible and as you stated more versatile. For example, you can define the callbacks after the initialisation. A big advantage is that just by googling you can find good solutions since this is just how AnimationController in Flutter works.

We could add onComplete and onStarted callbacks to SpriteAnimationController. Although it would be more convenient, it will deviate us from the "Flutter" side and add more code to maintain. I'm curious to know why Flutter decided not to add those function callbacks altogether. Do you know of any Flutter ticket (GitHub issue) that addresses this?

Wdyt?

[st-pasha]

I like the idea of having onFrame(index) callback. That way the user will have flexibility of reacting to specific moments within the animation cycle, and even have multiple actions firing at different frames. The onStart() callback then becomes unnecessary.

so how would you check in the update method if (frameIndex == currentIndex)?

Simply call the onFrame() whenever the currentIndex variable is updated.

[spydon]

Edit: Also, I am not 100% the test for event lifecycle will work for all devices - specifically, line 123. If I call that expect after the update, I am curious if its synchronous in nature or if its working because my device is slow and the onComplete executes faster than the update. Anyway, I open to suggestions.

There are no asynchronous operations involved here so you don't have to worry about the order.

For test number 3 you can run small increments in update to see that the first increment that overlaps the frame triggers the callback (and not the last). It looks like it actually triggers in the end of the frame now, so you'd have to call the callback for the next index (wrapped with modulo if the animation should repeat) in the while-loop, and the callback for the first frame has to be done together with the onStart callback.

[st-pasha]

For test number 3 you can run small increments in update to see that the first increment that overlaps the frame triggers the callback

Good idea!

so you'd have to call the callback for the next index (wrapped with modulo if the animation should repeat) in the while-loop

I think a more reliable approach is to call onFrame() immediately after the currentIndex++ statement (and also currentIndex = 0). That way you won't have an extra call when the animation is about to complete.

[munsterlander]

    test('verify call is being made at the first of the frame for multi-frame animation', () {
      double step = 0;
      final sprite = MockSprite();
      final animation =
      SpriteAnimation.spriteList([sprite, sprite, sprite], stepTime: 1, loop: false);
      animation.onFrame = (index) {
        if(step % 1 == 0){
          expect(index,step);
        }
      };
      while(step <= 3){
        step += 0.5;
      }
    });

I moved the onFrame call as suggested and this is passing. I just want to check and make sure this is what you were thinking? I have a merge conflict that for whatever reason won't fetch on my machine, so I will need to figure it out.

Edit: @spydon and @st-pasha I just want to confirm with you that onFrames expected behavior (by doing the changes suggested) means that it would not be called on the last frame of a non-looping animation and the only way for that to succeed would be by calling onComplete. By doing the calls after currentIndex is incremented, this is a by-product UNLESS I pull it out of the if statement and just put it after the if statement, in which case onFrame will get called regardless of the type of animation but at the beginning of every frame. Does that make sense?

[spydon]

I moved the onFrame call as suggested and this is passing. I just want to check and make sure this is what you were thinking? I have a merge conflict that for whatever reason won't fetch on my machine, so I will need to figure it out.

Almost, you also have to pass the step to update, and you can't have % 1 when you have a step of 0.5 since both the beginning and end of a frame will result in 0 then.
So it would be something like this instead:

    test('verify call is being made at the first of the frame for multi-frame animation', () {
      var timePassed = 0.0;
      var dt = 0.5;
      var timesCalled = 0;
      final sprite = MockSprite();
      final spriteList = [sprite, sprite, sprite];
      final animation = SpriteAnimation.spriteList(spriteList, stepTime: 1, loop: false);
      animation.onFrame = (index) {
          expect(index, timePassed);
          timesCalled++
      };
      while(step <= 3){
        timePassed += dt;
        animation.update(dt);
      }
      expect(timeCalled, spriteList.length);
    });

Edit: @spydon and @st-pasha I just want to confirm with you that onFrames expected behavior (by doing the changes suggested) means that it would not be called on the last frame of a non-looping animation and the only way for that to succeed would be by calling onComplete. By doing the calls after currentIndex is incremented, this is a by-product UNLESS I pull it out of the if statement and just put it after the if statement, in which case onFrame will get called regardless of the type of animation but at the beginning of every frame. Does that make sense?

Yes, onFrame should be called for the last frame too, and it should not be called when onComplete is called since that should be called once the last frame ends, not when it begins. It should be fine to have it within the if-statement since the if statement checks if the current frame is the last frame before currentIndex is incremented.

I have a merge conflict that for whatever reason won't fetch on my machine, so I will need to figure it out.

This is due to the change that was merged in #1564, do you need help sorting out the conflict?

[munsterlander]

@spydon Here is what ended up passing:

    test('verify call is being made at the first of the frame for multi-frame animation', () {
      var timePassed = 0.0;
      const dt = 0.5;
      var timesCalled = 0;
      final sprite = MockSprite();
      final spriteList = [sprite, sprite, sprite];
      final animation = SpriteAnimation.spriteList(spriteList, stepTime: 1, loop: false);
      animation.onFrame = (index) {
        expect(index, timePassed);
        timesCalled++;
      };
      while(timePassed <= 3){
        timePassed += dt;
        animation.update(dt);
      }
      expect(timesCalled, spriteList.length-1);
    });

Other tests were corrected. I will now work on resolving the conflict. Its a simple conflict, but git isn't fetching the change, so I just need to see why.

[spydon]

  expect(timesCalled, spriteList.length-1);
});

Why do you have -1 here? It should be called for each frame?

[munsterlander]

  expect(timesCalled, spriteList.length-1);
});

Why do you have -1 here? It should be called for each frame?

Thats what we are discussing here: #1492 (comment)

[spydon]

You can run flutter format . and then commit the result to make the formatting check pass.

[munsterlander]

This thing is annoying. I ran it, committed and pushed, then melos says lines over 80, so I go correct those, re run it, commit, but something else pops up after push. I have rerun it, commited and pushed again, lol. Quick question on that though, do you manually run that command or do you let Android Studio (if you use that) do its reformat code before commit?

[spydon]

This thing is annoying. I ran it, committed and pushed, then melos says lines over 80, so I go correct those, re run it, commit, but something else pops up after push. I have rerun it, commited and pushed again, lol. Quick question on that though, do you manually run that command or do you let Android Studio (if you use that) do its reformat code before commit?

I let android studio auto format. Maybe you don't have the newest version of flutter? Then the formatting can differ, run flutter upgrade first.

[munsterlander]

I do. Not certain, but I found this and will check it shortly. https://stackoverflow.com/questions/27092772/auto-code-formatting-in-android-studio

Edit: Format code on save was not checked. I have checked that now and will see how it goes.

[spydon]

Good job! You put a lot of time into this feature, very appreciated. :)