Skip to content
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

feat!: New AlternatePattern argument for SequenceEffect #3322

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/lib/stories/effects/sequence_effect_example.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class SequenceEffectExample extends FlameGame {
ScaleEffect.by(Vector2.all(1.5), duration(0.7)),
MoveEffect.to(Vector2(400, 500), duration(0.7)),
],
alternate: true,
alternatePattern: AlternatePattern.excludeLast,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done additionally to alternate, so that it's not a breaking change, and also follows the same pattern as the other effects.

infinite: true,
),
),
Expand Down
2 changes: 1 addition & 1 deletion packages/flame/lib/effects.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,6 @@ export 'src/effects/provider_interfaces.dart'
export 'src/effects/remove_effect.dart';
export 'src/effects/rotate_effect.dart';
export 'src/effects/scale_effect.dart';
export 'src/effects/sequence_effect.dart' show SequenceEffect;
export 'src/effects/sequence_effect.dart' show SequenceEffect, AlternatePattern;
export 'src/effects/size_effect.dart';
export 'src/effects/transform2d_effect.dart';
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ abstract class DurationEffectController extends EffectController {
double recede(double dt) {
_timer -= dt;
if (_timer < 0) {
final leftoverTime = 0 - _timer;
final leftoverTime = _timer.abs();
_timer = 0;
return leftoverTime;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/flame/lib/src/effects/effect.dart
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ abstract class Effect extends Component {
double recede(double dt) {
if (_finished && dt > 0) {
_finished = false;

/// Effects such as [MoveToEffect] must recalculate the direction vector.
onStart();
}
final remainingDt = controller.recede(dt);
if (_started) {
Expand Down
2 changes: 1 addition & 1 deletion packages/flame/lib/src/effects/move_to_effect.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class MoveToEffect extends MoveEffect {
@override
void apply(double progress) {
final dProgress = progress - previousProgress;
target.position += _offset * dProgress;
target.position += _offset * dProgress.abs();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this really correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it some more yesterday I think the issue wasn't the reflected vector itself, but rather that the direction vector was incorrect when the camera has a viewport-aware bounds behavior. This causes it to never reach its destination and so the reflective vector is incorrect. That's why taking the absolute value was ok after I added the step to recalculate the vector.

The camera won't be the only component that may never complete its effect with regards to MoveToEffect so this matter should be resolved in the PR imo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will potentially overshoot the destination right? And also it won't be matching the time anymore?
If you are at 0.99 progress and the delta progress is -0.02 when the effect is going forward it should have moved 0.03 "steps", since it'll start going back again, but with this it will simply move to 1.01.

}

@override
Expand Down
73 changes: 54 additions & 19 deletions packages/flame/lib/src/effects/sequence_effect.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import 'package:flame/src/effects/effect.dart';

EffectController _createController({
required List<Effect> effects,
required bool alternate,
required AlternatePattern alternatePattern,
required bool infinite,
required int repeatCount,
}) {
EffectController ec = _SequenceEffectEffectController(
effects,
alternate: alternate,
alternatePattern: alternatePattern,
);
if (infinite) {
ec = InfiniteEffectController(ec);
Expand All @@ -22,6 +22,32 @@ EffectController _createController({
return ec;
}

/// Specifies how to alternate a [SequenceEffect] pattern.
///
/// If [AlternatePattern.none] is provided, then
/// the [SequenceEffect] does not repeat in reverse when the child
/// [Effect]s have all completed playing.
///
/// When [SequenceEffect] is provided a pattern other than
/// [AlternatePattern.none], the sequence will repeat in the
/// reverse order, doubling the length of [EffectController.duration].
///
/// [AlternatePattern.includeLast] will replay the last [Effect] at the start
/// of the alternate pattern, effectively playing it twice in a row.
///
/// [AlternatePattern.excludeLast] will not replay the last [Effect] and will
/// jump to the second-to-last [Effect], if available, at the start of the
/// alternate pattern instead, effectively playing the last [Effect] only once
/// throughout the original and alternating pattern.
enum AlternatePattern {
none(0),
includeLast(1),
excludeLast(2);
Copy link
Member

@spydon spydon Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If infinite (or N runs) is used together with alternate, shouldn't the first also be excluded on runs that are after the first and not the last?

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already behaved this way. Only the last one was ever being repeated.


final int value;
const AlternatePattern(this.value);
}

/// Run multiple effects in a sequence, one after another.
///
/// The provided effects will be added as child components; however the custom
Expand All @@ -48,7 +74,7 @@ EffectController _createController({
class SequenceEffect extends Effect {
SequenceEffect(
List<Effect> effects, {
bool alternate = false,
AlternatePattern? alternatePattern = AlternatePattern.none,
bool infinite = false,
int repeatCount = 1,
super.onComplete,
Expand All @@ -62,7 +88,7 @@ class SequenceEffect extends Effect {
super(
_createController(
effects: effects,
alternate: alternate,
alternatePattern: alternatePattern!,
infinite: infinite,
repeatCount: repeatCount,
),
Expand Down Expand Up @@ -92,15 +118,16 @@ class SequenceEffect extends Effect {
class _SequenceEffectEffectController extends EffectController {
_SequenceEffectEffectController(
this.effects, {
required this.alternate,
required this.alternatePattern,
}) : super.empty();

/// The list of children effects.
final List<Effect> effects;

/// If this flag is true, then after the sequence runs to the end, it will
/// run again in the reverse order.
final bool alternate;
/// If this flag is not [AlternatePattern.none], then after the sequence runs
/// to the end, it will run again in the reverse order according to the policy
/// of the [AlternatePattern] value provided.
final AlternatePattern alternatePattern;

/// Index of the currently running effect within the [effects] list. If there
/// are n effects in total, then this runs as 0, 1, ..., n-1. After that, if
Expand All @@ -124,7 +151,7 @@ class _SequenceEffectEffectController extends EffectController {
for (final effect in effects) {
totalDuration += effect.controller.duration ?? 0;
}
if (alternate) {
if (alternatePattern != AlternatePattern.none) {
totalDuration *= 2;
}
return totalDuration;
Expand All @@ -136,7 +163,7 @@ class _SequenceEffectEffectController extends EffectController {
}

@override
double get progress => (_index + 1) / n;
double get progress => (_index < 0 ? -_index : _index + 1) / n;

@override
double advance(double dt) {
Expand All @@ -147,10 +174,13 @@ class _SequenceEffectEffectController extends EffectController {
if (t > 0) {
_index += 1;
if (_index == n) {
if (alternate) {
_index = -1;
} else {
_index = n - 1;
_index = switch (alternatePattern) {
AlternatePattern.includeLast => -1,
AlternatePattern.excludeLast => -2,
AlternatePattern.none => n - 1,
};

if (_index == n - 1) {
_completed = true;
break;
}
Expand Down Expand Up @@ -208,13 +238,18 @@ class _SequenceEffectEffectController extends EffectController {

@override
void setToEnd() {
if (alternate) {
_index = -n;
effects.forEach((e) => e.reset());
} else {
_index = n - 1;
_index = switch (alternatePattern) {
AlternatePattern.includeLast => -n,
AlternatePattern.excludeLast => -n + 1,
AlternatePattern.none => n - 1,
};

if (_index == n - 1) {
effects.forEach((e) => e.resetToEnd());
} else {
effects.forEach((e) => e.reset());
}

_completed = true;
}

Expand Down
14 changes: 7 additions & 7 deletions packages/flame/test/effects/sequence_effect_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ void main() {
[
MoveEffect.to(Vector2(10, 10), EffectController(duration: 3)),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
);
expect(effect.controller.duration, 6);
expect(effect.controller.isRandom, false);
Expand All @@ -37,7 +37,7 @@ void main() {
[
MoveEffect.to(Vector2.zero(), EffectController(duration: 1)),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
infinite: true,
);
expect(effect.controller.duration, double.infinity);
Expand All @@ -55,7 +55,7 @@ void main() {
);
final effect = SequenceEffect(
[randomEffect],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
repeatCount: 1000,
);
expect(
Expand Down Expand Up @@ -156,7 +156,7 @@ void main() {
MoveEffect.by(Vector2(10, 0), EffectController(duration: 1)),
MoveEffect.by(Vector2(0, 10), EffectController(duration: 1)),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
);
expect(effect.controller.duration, 4);

Expand Down Expand Up @@ -186,7 +186,7 @@ void main() {
MoveEffect.by(Vector2(1, 0), controller()),
MoveEffect.by(Vector2(0, 1), controller()),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
);

final component = PositionComponent()..add(effect);
Expand Down Expand Up @@ -254,7 +254,7 @@ void main() {
MoveEffect.to(Vector2(x2, y2), duration(1)),
MoveEffect.to(Vector2(x3, y3), duration(1)),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
repeatCount: 2,
),
MoveEffect.by(Vector2(x4 - x1, y4 - y1), duration(2)),
Expand All @@ -266,7 +266,7 @@ void main() {
repeatCount: 5,
),
],
alternate: true,
alternatePattern: AlternatePattern.includeLast,
);
expect(effect.controller.duration, 42);

Expand Down
43 changes: 29 additions & 14 deletions packages/flame_tiled/example/lib/main.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import 'dart:math';

import 'package:flame/components.dart';
import 'package:flame/effects.dart';
import 'package:flame/experimental.dart';
import 'package:flame/flame.dart';
import 'package:flame/game.dart';
import 'package:flame_tiled/flame_tiled.dart';
Expand All @@ -22,23 +25,35 @@ class TiledGame extends FlameGame {

@override
Future<void> onLoad() async {
camera.viewfinder
..zoom = 0.5
..anchor = Anchor.topLeft
..add(
MoveToEffect(
Vector2(1000, 0),
EffectController(
duration: 10,
alternate: true,
infinite: true,
),
),
);

mapComponent = await TiledComponent.load('map.tmx', Vector2.all(16));
world.add(mapComponent);

// Create a camera-panning sequence across the 4 corners
// of the map
camera.viewfinder.add(
SequenceEffect(
[
for (int i = 0, j = 0; j < 2; i++, j = i ~/ 2)
MoveToEffect(
Vector2(
mapComponent.width * min(1, i % 3),
mapComponent.height * j,
),
EffectController(
duration: 3,
),
),
],
alternatePattern: AlternatePattern.excludeLast,
infinite: true,
),
);

camera.setBounds(
Rectangle.fromLTRB(0, 0, mapComponent.width, mapComponent.height),
considerViewport: true,
);

final objectGroup =
mapComponent.tileMap.getLayer<ObjectGroup>('AnimatedCoins');
final coins = await Flame.images.load('coins.png');
Expand Down
Loading