-
-
Notifications
You must be signed in to change notification settings - Fork 917
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
base: main
Are you sure you want to change the base?
Changes from all commits
80166b1
dd1fc7b
d107787
faa32b9
d05e3ee
ed63c41
ece8953
6dd88a1
2ea5e9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,12 +6,14 @@ 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); | ||
|
@@ -22,6 +24,24 @@ EffectController _createController({ | |
return ec; | ||
} | ||
|
||
/// Specifies how to playback an alternating [SequenceEffect] pattern. | ||
/// | ||
/// [AlternatePattern.repeatLast] will replay the first and last [Effect] | ||
/// when the sequence repeats, causing those [Effect]s to play twice-in-a-row. | ||
/// | ||
/// [AlternatePattern.doNotRepeatLast] will not replay the first and last | ||
/// [Effect] and instead jumps to the second and second-to-last [Effect] | ||
/// respectively, if available, at the start of the alternating pattern. | ||
/// This is equivalent to playing the first and last [Effect] once throughout | ||
/// the combined span of the original pattern plus its reversed pattern. | ||
enum AlternatePattern { | ||
repeatLast(1), | ||
doNotRepeatLast(2); | ||
|
||
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 | ||
|
@@ -32,6 +52,10 @@ EffectController _createController({ | |
/// If the `alternate` flag is provided, then the sequence will run in the | ||
/// reverse after it ran forward. | ||
/// | ||
/// Parameter `alternatePattern` is only used when `alternate` is true. | ||
/// This parameter modifies how the pattern repeats in reverse. | ||
/// See [AlternatePattern] for options. | ||
/// | ||
/// Parameter `repeatCount` will make the sequence repeat a certain number of | ||
/// times. If `alternate` is also true, then the sequence will first run | ||
/// forward, then back, and then repeat this pattern `repeatCount` times in | ||
|
@@ -48,6 +72,7 @@ EffectController _createController({ | |
class SequenceEffect extends Effect { | ||
SequenceEffect( | ||
List<Effect> effects, { | ||
AlternatePattern alternatePattern = AlternatePattern.repeatLast, | ||
bool alternate = false, | ||
bool infinite = false, | ||
int repeatCount = 1, | ||
|
@@ -63,6 +88,7 @@ class SequenceEffect extends Effect { | |
_createController( | ||
effects: effects, | ||
alternate: alternate, | ||
alternatePattern: alternatePattern, | ||
infinite: infinite, | ||
repeatCount: repeatCount, | ||
), | ||
|
@@ -93,6 +119,7 @@ class _SequenceEffectEffectController extends EffectController { | |
_SequenceEffectEffectController( | ||
this.effects, { | ||
required this.alternate, | ||
required this.alternatePattern, | ||
}) : super.empty(); | ||
|
||
/// The list of children effects. | ||
|
@@ -102,6 +129,11 @@ class _SequenceEffectEffectController extends EffectController { | |
/// run again in the reverse order. | ||
final bool alternate; | ||
|
||
/// If [alternate] is true, and after the sequence runs to completion once, | ||
/// this 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 | ||
/// the effect alternates, then the `_index` continues as -1, -2, ..., -n, | ||
|
@@ -114,6 +146,23 @@ class _SequenceEffectEffectController extends EffectController { | |
/// Total number of effects in this sequence. | ||
int get n => effects.length; | ||
|
||
/// If [alternate] is not set, our last index will be `n-1`. | ||
/// Otherwise, the sequence approaches 0 from the left of the | ||
/// number-line, and if our [alternatePattern] excludes the first | ||
/// [Effect], then it will reduce the destination index by 1. | ||
int get _computeLastIndex => switch (alternate) { | ||
true => switch (alternatePattern) { | ||
// index 0 is the start of the original pattern, | ||
// therefore -1 is our destination index, which will | ||
// be the exact same `Effect` as index 0. | ||
AlternatePattern.repeatLast => -1, | ||
// Since the original pattern will begin again from start, | ||
// skip index -1 and make -2 our destination index. | ||
AlternatePattern.doNotRepeatLast => -2, | ||
}, | ||
false => n - 1, | ||
}; | ||
|
||
@override | ||
bool get completed => _completed; | ||
bool _completed = false; | ||
|
@@ -124,9 +173,21 @@ class _SequenceEffectEffectController extends EffectController { | |
for (final effect in effects) { | ||
totalDuration += effect.controller.duration ?? 0; | ||
} | ||
|
||
// Abort early | ||
if (totalDuration == 0.0) { | ||
return totalDuration; | ||
} | ||
|
||
if (alternate) { | ||
totalDuration *= 2; | ||
|
||
if (alternatePattern == AlternatePattern.doNotRepeatLast) { | ||
totalDuration -= effects.first.controller.duration ?? 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come the first one is removed here too? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi. This has been explained in the PR, code comments, and here: |
||
totalDuration -= effects.last.controller.duration ?? 0; | ||
} | ||
} | ||
|
||
return totalDuration; | ||
} | ||
|
||
|
@@ -136,7 +197,7 @@ class _SequenceEffectEffectController extends EffectController { | |
} | ||
|
||
@override | ||
double get progress => (_index + 1) / n; | ||
double get progress => (_index < 0 ? -_index : _index + 1) / n; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this a bit? In what cases is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way the |
||
|
||
@override | ||
double advance(double dt) { | ||
|
@@ -147,20 +208,32 @@ class _SequenceEffectEffectController extends EffectController { | |
if (t > 0) { | ||
_index += 1; | ||
if (_index == n) { | ||
if (alternate) { | ||
_index = -1; | ||
} else { | ||
_index = n - 1; | ||
_index = _computeLastIndex; | ||
|
||
if (_index == n - 1) { | ||
_completed = true; | ||
break; | ||
} | ||
} | ||
} | ||
} else { | ||
// This case represents the reversed alternating pattern | ||
// when `alternate` is true. Our indices will be negative, | ||
// and we recede back to index 0. | ||
|
||
t = currentEffect.recede(t); | ||
if (t > 0) { | ||
_index -= 1; | ||
if (_index < -n) { | ||
|
||
var lastIndex = -n; | ||
// Iff the requested alternate policy is `repeatLast`, then we must | ||
// include and play the start Effect before considering our sequence | ||
// completed. | ||
if (alternate && alternatePattern == AlternatePattern.repeatLast) { | ||
lastIndex -= 1; | ||
} | ||
|
||
if (_index == lastIndex) { | ||
_index = -n; | ||
_completed = true; | ||
break; | ||
|
@@ -208,13 +281,14 @@ class _SequenceEffectEffectController extends EffectController { | |
|
||
@override | ||
void setToEnd() { | ||
if (alternate) { | ||
_index = -n; | ||
effects.forEach((e) => e.reset()); | ||
} else { | ||
_index = n - 1; | ||
_index = _computeLastIndex; | ||
|
||
if (_index == n - 1) { | ||
effects.forEach((e) => e.resetToEnd()); | ||
} else { | ||
effects.forEach((e) => e.reset()); | ||
} | ||
|
||
_completed = true; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
# Miscellaneous | ||
TheMaverickProgrammer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*.class | ||
*.log | ||
*.pyc | ||
*.swp | ||
.DS_Store | ||
.atom/ | ||
.buildlog/ | ||
.history | ||
.svn/ | ||
migrate_working_dir/ | ||
|
||
# IntelliJ related | ||
*.iml | ||
*.ipr | ||
*.iws | ||
.idea/ | ||
|
||
# The .vscode folder contains launch configuration and tasks you configure in | ||
# VS Code which you may wish to be included in version control, so this line | ||
# is commented out by default. | ||
#.vscode/ | ||
|
||
# Flutter/Dart/Pub related | ||
**/doc/api/ | ||
**/ios/Flutter/.last_build_id | ||
.dart_tool/ | ||
.flutter-plugins | ||
.flutter-plugins-dependencies | ||
.pub-cache/ | ||
.pub/ | ||
/build/ | ||
|
||
# Symbolication related | ||
app.*.symbols | ||
|
||
# Obfuscation related | ||
app.*.map.json | ||
|
||
# Android Studio will place build artifacts here | ||
/android/app/debug | ||
/android/app/profile | ||
/android/app/release |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progress
will be decrementingdt
units from1.0
during the reverse phase of the alternating pattern, whilepreviousProgress
retains theprogress+dt
value from before. ThereforedProgress = 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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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, thendProgress
should be-0.1
, not0.1
.Try running only the the
MoveToEffect
withalternate: true
.There was a problem hiding this comment.
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.