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.

Sign up for GitHub

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

Jump to bottom

feat!: New AlternatePattern argument for SequenceEffect #3322

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

feat!: New AlternatePattern argument for SequenceEffect #3322

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
80166b1
Fixed math in MoveToEffect so that negative progress did not reflect …
TheMaverickProgrammer Sep 24, 2024
dd1fc7b
Removed unnecessary abs(). Forgot to revert recede() after local test…
TheMaverickProgrammer Sep 24, 2024
d107787
Merge branch 'main' of https://github.com/TheMaverickProgrammer/flame
TheMaverickProgrammer Nov 4, 2024
faa32b9
Merge branch 'main' into feature/improved_sequence_effect
TheMaverickProgrammer Nov 4, 2024
d05e3ee
renamed enum to be clearer. Restored lternate boolean. Added logic t…
TheMaverickProgrammer Nov 4, 2024
ed63c41
better documentation. code cleanup.
TheMaverickProgrammer Nov 5, 2024
ece8953
comment cleanup
TheMaverickProgrammer Nov 5, 2024
6dd88a1
restore tests
TheMaverickProgrammer Nov 5, 2024
2ea5e9f
linter did not recognize word 'numberline'
TheMaverickProgrammer Nov 5, 2024
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
View file
Open in desktop
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,
TheMaverickProgrammer marked this conversation as resolved.
Show resolved Hide resolved
infinite: true,
),
),
Expand Down
2 changes: 1 addition & 1 deletion packages/flame/lib/effects.dart
View file
Open in desktop
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';
2 changes: 1 addition & 1 deletion packages/flame/lib/src/effects/controllers/duration_effect_controller.dart
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I'm understanding your question precisely, but it seems that you've noticed the possibility of under-fitting the expected step amounts, time-wise. Yes this does happen with the viewport-aware behavior as described (the camera cannot exceed the bounds, and therefore never arrives to its destination). ATM with the changes I've added, I have not seen any noticeable problems.

Copy link
Member

Choose a reason for hiding this comment

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

Simply put; I don't think it will always be at the expected place at each time step when abs is used, that was what I tried to describe in the last comment, the delta doesn't become correct. If it overshoots its destination it should start going back, in the cases where it is alternating.

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

progress will be decrementing dt units from 1.0 during the reverse phase of the alternating pattern, while previousProgress retains the progress+dt value from before. Therefore dProgress = progress - previousProgress would be negative, which we don't want to use. It's better to recalculate the starting position and use positive scalars instead.

Copy link
Member

Choose a reason for hiding this comment

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

But it'll surely overshoot the end value then?

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi. No it never does for this effect or any as far as I can tell. The same math could have overshot in the reverse direction when dProgress was negative.

Copy link
Member

Choose a reason for hiding this comment

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

In the case that dProgress becomes negative it is supposed to go backwards. Say that the previous progress is 0.9 and it it supposed to move backwards to 0.8, then dProgress should be -0.1, not 0.1.
Try running only the the MoveToEffect with alternate: true.

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi. A negative progress value for this effect specifically is not only unintuitive but also does not solve the MoveToEffect starting vector issue.

}

@override
Expand Down
73 changes: 54 additions & 19 deletions packages/flame/lib/src/effects/sequence_effect.dart
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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.

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've corrected the default pattern behavior. The alternating pattern now plays both the first and last Effect elements in the list, instead of just the last element as it did before this PR. This default behavior is AlternatePattern.repeatLast. The term "last" used here is relative to the playback direction.

To avoid playing first and last Effects twice-in-a-row when the sequence reverses, AlternatePattern.doNotRepeatLast is appropriate.


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;
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this a bit? In what cases is the _index negative? This doesn't seem like it would follow how progress works for the rest of the effects.

Copy link
Contributor Author

@TheMaverickProgrammer TheMaverickProgrammer Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The way the SequenceEffect has been written for some time now is that it uses a negative _index to determine if the pattern is in its reverse phase. To be clear, I did not write it this way. This code was like this when I started this PR,


@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
View file
Open in desktop
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
View file
Open in desktop
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