From 08e8eac1734a63111a5b7aba4e1bfd20d503aaf4 Mon Sep 17 00:00:00 2001 From: Pasha Stetsenko Date: Mon, 21 Feb 2022 11:16:46 -0800 Subject: [PATCH] fix: Step time in SpriteAnimation must be positive (#1387) --- packages/flame/lib/src/sprite_animation.dart | 188 +++++++++--------- .../flame/test/sprite_animation_test.dart | 61 ++++++ packages/flame/test/spritesheet_test.dart | 3 +- 3 files changed, 158 insertions(+), 94 deletions(-) create mode 100644 packages/flame/test/sprite_animation_test.dart diff --git a/packages/flame/lib/src/sprite_animation.dart b/packages/flame/lib/src/sprite_animation.dart index 9d201cb8bb..e5095de1e5 100644 --- a/packages/flame/lib/src/sprite_animation.dart +++ b/packages/flame/lib/src/sprite_animation.dart @@ -105,66 +105,68 @@ 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 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 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 sprites, { + required List 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 @@ -172,51 +174,27 @@ class SpriteAnimation { /// /// [image]: sprite sheet animation image. /// [jsonData]: animation's data in json format. - SpriteAnimation.fromAsepriteData( + factory SpriteAnimation.fromAsepriteData( Image image, Map jsonData, ) { final jsonFrames = jsonData['frames'] as Map; - - final frames = jsonFrames.values.map((dynamic value) { - final map = value as Map; - final frameData = map['frame'] as Map; - 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 sprites, { - required List 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; + 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 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 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) { diff --git a/packages/flame/test/sprite_animation_test.dart b/packages/flame/test/sprite_animation_test.dart new file mode 100644 index 0000000000..1cb8e98e41 --- /dev/null +++ b/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 {} diff --git a/packages/flame/test/spritesheet_test.dart b/packages/flame/test/spritesheet_test.dart index 24d50417d0..f4bca24d1f 100644 --- a/packages/flame/test/spritesheet_test.dart +++ b/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'), ); }, );