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 2 commits into
base: main
Choose a base branch
from
Draft

feat!: New AlternatePattern argument for SequenceEffect #3322

wants to merge 2 commits into from

Conversation

TheMaverickProgrammer
Copy link
Contributor

@TheMaverickProgrammer TheMaverickProgrammer commented Sep 24, 2024 •
edited
Loading

  • This PR fixes math in MoveToEffect so that negative progress did not reflect the direction of the delta offset vec.
  • Corrected progress getter overload in SequenceEffect to correctly map to the progress when _index < 0.
  • Added onStart() in recede() initial condition so that effects like MoveToEffect can recalculate their initial data.
  • Removed unnecessary math in DurationEffectController to avoid potential negative zeros.
  • Replaced alternate boolean for SequenceEffect with a new type AlternatePattern which allows users to include or exclude the last effect in the sequence when the pattern reverses.
  • Default value is now AlternatePattern.none.
  • Updated all SequenceEffect examples and tests to reflect changes.
  • Made the TiledGame example project more aesthetically interesting.
    • The camera now pans to all 4 corners WRT the bounds of the loaded map and uses the new AlternatePattern feature along with all the new fixes.
  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

TO DISCUSS

  1. I have skipped the test checkbox until I had a review and we could discuss these changes.
  2. We may need to consider an onEnd() for Effects. Or provide onStart() with an initial progress value to calculate from.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Migration instructions

  1. Replace every alternate=true for all SequenceEffects with alternatePattern=AlternatePattern.includeLast.
  2. AlternatePattern.includeLast will recreate the exact same behavior as alternate=true in the previous Flame version.
  3. From Step 2, users can decide to switch to AlternatePattern.excludeLast for SequenceEffects to skip playing the last Effect twice in a row when alternating.

Related Issues

I looked but didn't immediately see anyone reporting these issues. I discovered them myself.

@TheMaverickProgrammer
Fixed math in MoveToEffect so that negative progress did not reflect …
80166b1 
…the direction of the delta offset vec. Corrected progress getter overload in SequenceEffect to correctly map to the progess when _index < 0. Added onStart() in the initial conidition of ecede() so that effects like MoveEffect can recalculate their initial data. Removed unnecessary math in DurationEffectController to avoid potential negative zeros. Replaced �lternate boolean for SequenceEffect with a new type AlternatePattern which allows users to include or exlude the last effect in the sequence when the pattern reverses. Default value is now AlternatePattern.none. Updated all SequenceEffect examples and tests to reflect changes. Made the TiledGame example project more aesthetically interesting. The camera now pans to all 4 corners WRT the bounds of the loaded map and uses the new AlternatePattern feature along with all the new fixes.
@TheMaverickProgrammer TheMaverickProgrammer changed the title feature!: AlternatePattern for SequenceEffect. fix: bad math in MoveToEffect. feature!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect). Sep 24, 2024
@TheMaverickProgrammer TheMaverickProgrammer changed the title feature!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect). feat!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect). Sep 24, 2024
spydon
spydon reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@spydon spydon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good idea, it needs tests and docs though.
EDIT: Ah you already wrote that in the PR description.

examples/lib/stories/effects/sequence_effect_example.dart
@@ -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.

packages/flame/lib/src/effects/move_to_effect.dart
@@ -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.

packages/flame/lib/src/effects/sequence_effect.dart
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.

@spydon spydon changed the title feat!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect). feat!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect) Sep 25, 2024
@spydon spydon changed the title feat!: AlternatePattern for SequenceEffect (and fix wrong math in MoveToEffect) feat!: New AlternatePattern argument for SequenceEffect Sep 25, 2024
@TheMaverickProgrammer TheMaverickProgrammer marked this pull request as draft September 25, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spydon spydon spydon left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TheMaverickProgrammer @spydon