fix: Step time in SpriteAnimation must be positive (#1387)

packages/flame/lib/src/sprite_animation.dart

@@ -105,118 +105,96 @@ class SpriteAnimationFrame {
 /// Represents a sprite animation, that is, a list of sprites that change with
 /// time.
 class SpriteAnimation {
-  /// The frames that compose this animation.
-  List<SpriteAnimationFrame> frames = [];
-
-  /// Index of the current frame that should be displayed.
-  int currentIndex = 0;
-
-  /// Current clock time (total time) of this animation, in seconds, since last
-  /// frame.
-  ///
-  /// It's ticked by the update method. It's reset every frame change.
-  double clock = 0.0;
-
-  /// Total elapsed time of this animation, in seconds, since start or a reset.
-  double elapsed = 0.0;
-
-  /// Whether the animation loops after the last sprite of the list, going back
-  /// to the first, or keeps returning the last when done.
-  bool loop = true;
-
-  /// Registered method to be triggered when the animation complete.
-  void Function()? onComplete;
-
-  /// Creates an animation given a list of frames.
-  SpriteAnimation(this.frames, {this.loop = true});
-
-  /// Creates an empty animation
-  SpriteAnimation.empty();
-
-  /// Creates an animation based on the parameters.
-  ///
-  /// All frames have the same [stepTime].
-  SpriteAnimation.spriteList(
+  SpriteAnimation(this.frames, {this.loop = true})
+      : assert(frames.isNotEmpty, 'There must be at least one animation frame'),
+        assert(
+          frames.every((frame) => frame.stepTime > 0),
+          'All frames must have positive durations',
+        );
+
+  /// Create animation from a list of [sprites] all having the same transition
+  /// time [stepTime].
+  factory SpriteAnimation.spriteList(
     List<Sprite> sprites, {
     required double stepTime,
-    this.loop = true,
+    bool loop = true,
   }) {
-    if (sprites.isEmpty) {
-      throw Exception('You must have at least one frame!');
-    }
-    frames = sprites.map((s) => SpriteAnimationFrame(s, stepTime)).toList();
+    return SpriteAnimation(
+      sprites.map((sprite) => SpriteAnimationFrame(sprite, stepTime)).toList(),
+      loop: loop,
+    );
+  }
+
+  /// Create animation from a list of [sprites] each having its own duration
+  /// provided in the [stepTimes] list.
+  factory SpriteAnimation.variableSpriteList(
+    List<Sprite> sprites, {
+    required List<double> stepTimes,
+    bool loop = true,
+  }) {
+    assert(
+      stepTimes.length == sprites.length,
+      'Lengths of stepTimes and sprites lists must be equal',
+    );
+    return SpriteAnimation(
+      [
+        for (var i = 0; i < sprites.length; i++)
+          SpriteAnimationFrame(sprites[i], stepTimes[i])
+      ],
+      loop: loop,
+    );
   }
 
-  /// Creates an SpriteAnimation based on its [data].
+  /// Create animation from a single [image] that contains all frames.
   ///
-  /// Check [SpriteAnimationData] constructors for more info.
-  SpriteAnimation.fromFrameData(
+  /// The [data] argument provides the description of where the individual
+  /// sprites are located within the main image.
+  factory SpriteAnimation.fromFrameData(
     Image image,
     SpriteAnimationData data,
   ) {
-    frames = data.frames.map((frameData) {
-      return SpriteAnimationFrame(
-        Sprite(
-          image,
-          srcSize: frameData.srcSize,
-          srcPosition: frameData.srcPosition,
-        ),
-        frameData.stepTime,
-      );
-    }).toList();
-    loop = data.loop;
+    return SpriteAnimation(
+      [
+        for (final frameData in data.frames)
+          SpriteAnimationFrame(
+            Sprite(
+              image,
+              srcSize: frameData.srcSize,
+              srcPosition: frameData.srcPosition,
+            ),
+            frameData.stepTime,
+          )
+      ],
+      loop: data.loop,
+    );
   }
 
   /// Automatically creates an Animation Object using animation data provided by
   /// the json file provided by Aseprite.
   ///
   /// [image]: sprite sheet animation image.
   /// [jsonData]: animation's data in json format.
-  SpriteAnimation.fromAsepriteData(
+  factory SpriteAnimation.fromAsepriteData(
     Image image,
     Map<String, dynamic> jsonData,
   ) {
     final jsonFrames = jsonData['frames'] as Map<String, dynamic>;
-
-    final frames = jsonFrames.values.map((dynamic value) {
-      final map = value as Map;
-      final frameData = map['frame'] as Map<String, dynamic>;
-      final x = frameData['x'] as int;
-      final y = frameData['y'] as int;
-      final width = frameData['w'] as int;
-      final height = frameData['h'] as int;
-
-      final stepTime = (map['duration'] as int) / 1000;
-
-      final sprite = Sprite(
-        image,
-        srcPosition: Vector2Extension.fromInts(x, y),
-        srcSize: Vector2Extension.fromInts(width, height),
-      );
-
-      return SpriteAnimationFrame(sprite, stepTime);
-    });
-
-    this.frames = frames.toList();
-    loop = true;
-  }
-
-  SpriteAnimation.variableSpriteList(
-    List<Sprite> sprites, {
-    required List<double> stepTimes,
-    this.loop = true,
-  }) {
-    if (sprites.isEmpty) {
-      throw Exception('You must have at least one frame!');
-    }
-    if (stepTimes.length != sprites.length) {
-      throw Exception('The length of stepTimes and sprites must be the same!');
-    }
-
-    frames = List.generate(
-      sprites.length,
-      (i) => SpriteAnimationFrame(sprites[i], stepTimes[i]),
-      growable: false,
+    return SpriteAnimation(
+      jsonFrames.values.map((dynamic value) {
+        final map = value as Map;
+        final frameData = map['frame'] as Map<String, dynamic>;
+        final x = frameData['x'] as int;
+        final y = frameData['y'] as int;
+        final width = frameData['w'] as int;
+        final height = frameData['h'] as int;
+        final stepTime = (map['duration'] as int) / 1000;
+        final sprite = Sprite(
+          image,
+          srcPosition: Vector2Extension.fromInts(x, y),
+          srcSize: Vector2Extension.fromInts(width, height),
+        );
+        return SpriteAnimationFrame(sprite, stepTime);
+      }).toList(),
     );
   }
 
@@ -233,6 +211,28 @@ class SpriteAnimation {
     return SpriteAnimation.fromFrameData(image, data);
   }
 
+  /// The frames that compose this animation.
+  List<SpriteAnimationFrame> frames = [];
+
+  /// Index of the current frame that should be displayed.
+  int currentIndex = 0;
+
+  /// Current clock time (total time) of this animation, in seconds, since last
+  /// frame.
+  ///
+  /// It's ticked by the update method. It's reset every frame change.
+  double clock = 0.0;
+
+  /// Total elapsed time of this animation, in seconds, since start or a reset.
+  double elapsed = 0.0;
+
+  /// Whether the animation loops after the last sprite of the list, going back
+  /// to the first, or keeps returning the last when done.
+  bool loop = true;
+
+  /// Registered method to be triggered when the animation complete.
+  void Function()? onComplete;
+
   /// The current frame that should be displayed.
   SpriteAnimationFrame get currentFrame => frames[currentIndex];
 
@@ -248,12 +248,14 @@ class SpriteAnimation {
   set variableStepTimes(List<double> stepTimes) {
     assert(stepTimes.length == frames.length);
     for (var i = 0; i < frames.length; i++) {
+      assert(stepTimes[i] > 0, 'All step times must be positive');
       frames[i].stepTime = stepTimes[i];
     }
   }
 
   /// Sets a fixed step time to all frames.
   set stepTime(double stepTime) {
+    assert(stepTime > 0, 'Step time must be positive');
     frames.forEach((frame) => frame.stepTime = stepTime);
   }
 
@@ -294,7 +296,7 @@ class SpriteAnimation {
   void update(double dt) {
     clock += dt;
     elapsed += dt;
-    if (isSingleFrame || _done) {
+    if (_done) {
       return;
     }
     while (clock >= currentFrame.stepTime) {

packages/flame/test/sprite_animation_test.dart

@@ -0,0 +1,61 @@
+import 'package:flame/src/sprite_animation.dart';
+import 'package:flame_test/flame_test.dart';
+import 'package:mocktail/mocktail.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group('SpriteAnimation', () {
+    test('Throw assertion error on empty list of frames', () {
+      expect(
+        () => SpriteAnimation.spriteList([], stepTime: 1),
+        failsAssert('There must be at least one animation frame'),
+      );
+    });
+
+    test('Throw assertion error on non-positive step time', () {
+      final sprite = MockSprite();
+      expect(
+        () => SpriteAnimation.spriteList([sprite], stepTime: 0),
+        failsAssert('All frames must have positive durations'),
+      );
+      expect(
+        () => SpriteAnimation.variableSpriteList(
+          [sprite, sprite, sprite],
+          stepTimes: [1, -1, 0],
+        ),
+        failsAssert('All frames must have positive durations'),
+      );
+    });
+
+    test('Throw assertion error when setting non-positive step time', () {
+      final sprite = MockSprite();
+      final animation =
+          SpriteAnimation.spriteList([sprite, sprite, sprite], stepTime: 1);
+      expect(
+        () => animation.stepTime = 0,
+        failsAssert('Step time must be positive'),
+      );
+      expect(
+        () => animation.variableStepTimes = [3, 2, 0],
+        failsAssert('All step times must be positive'),
+      );
+    });
+
+    test('onComplete called for single-frame animation', () {
+      var counter = 0;
+      final sprite = MockSprite();
+      final animation =
+          SpriteAnimation.spriteList([sprite], stepTime: 1, loop: false)
+            ..onComplete = () => counter++;
+      expect(counter, 0);
+      animation.update(0.5);
+      expect(counter, 0);
+      animation.update(0.5);
+      expect(counter, 1);
+      animation.update(1);
+      expect(counter, 1);
+    });
+  });
+}
+
+class MockSprite extends Mock implements Sprite {}

packages/flame/test/spritesheet_test.dart

@@ -1,6 +1,7 @@
 import 'package:flame/sprite.dart';
 import 'package:flame/src/extensions/image.dart';
 import 'package:flame/src/extensions/vector2.dart';
+import 'package:flame_test/flame_test.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:mocktail/mocktail.dart';
 
@@ -55,7 +56,7 @@ void main() {
             row: 1,
             stepTimes: [2.0],
           ),
-          throwsException,
+          failsAssert('Lengths of stepTimes and sprites lists must be equal'),
         );
       },
     );