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

Effect controllers restructuring #1134

Merged
merged 59 commits into from
Dec 4, 2021

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Nov 23, 2021

Description

Given the new requirement that effect controllers must be able to run backwards in time, and owing to the considerable complexity this entails, this PRs decomposes effect controllers into multiple small building blocks. The main API of EffectController remains mostly unchanged (with few new properties added), but the implementation of various effect controllers changes radically. In particular, there is no more class StandardEffectController. Instead, there are several smaller classes:

  • LinearEffectController,
  • CurvedEffectController,
  • ReverseLinearEffectController,
  • ReverseCurvedEffectController,
  • DelayedEffectController,
  • RepeatedEffectController,
  • InfiniteEffectController,
  • PauseEffectController,
  • SequenceEffectController.

In addition, factory constructor EffectController() has exactly the same API as old StandardEffectController, and can be used in its place.

This new structure gives several advantages:

  • Implementing "backwards in time" functionality is much easier for each small building block. My attempts at implementing the same functionality within the old StandardEffectController were producing an incomprehensible mess.
  • SimpleEffectController is no longer needed, the EffectController() constructor will use as little or as many building blocks as necessary to create the effect controller.
  • The new system allows to build custom effect controllers very easily.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors and are passing (See Contributor Guide).
  • My PR does not decrease the code coverage, or I have a very special case and explained on the PR description why this PR decreases the coverage.
  • I updated/added relevant documentation (doc comments with ///) and updated/added examples in doc/examples.
  • I have formatted my code with flutter format and the flutter analyze does not report any problems.
  • I read and followed the Flame Style Guide.
  • I have added a description of the change under [next] in CHANGELOG.md.
  • I removed the Draft status, by clicking on the Ready for review button in this PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md).

@st-pasha st-pasha changed the title [WIP] Effect controllers restructuring Effect controllers restructuring Dec 1, 2021
@st-pasha st-pasha marked this pull request as ready for review December 1, 2021 18:35
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Looks good, but docs are missing

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion

packages/flame_test/lib/src/fails_assert.dart Show resolved Hide resolved
Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

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

Awesome work @st-pasha!

@spydon spydon enabled auto-merge (squash) December 4, 2021 15:48
@spydon
Copy link
Member

spydon commented Dec 4, 2021

I'll merge this, and agree with Luan, great job! We still need to update the docs for the effects though, will you do that @st-pasha?

@spydon spydon merged commit bfcda07 into flame-engine:main Dec 4, 2021
@st-pasha st-pasha deleted the ps/effect-controllers branch December 4, 2021 17:55
@st-pasha
Copy link
Contributor Author

st-pasha commented Dec 4, 2021

yep

spydon pushed a commit that referenced this pull request Dec 27, 2021
Added SequenceEffect, which performs a series of other effects.

The biggest challenge in implementing this feature came from the need to run the sequence in reverse, due to the alternate flag. This required that every effect and every controller supported running "back in time", which is not as simple as it sounds.

The following breaking changes were introduced:

    The Effect class no longer supports .reverse() method and .isReversed flag.

    This flag was added only 2 weeks ago (

Effect controllers restructuring #1134), with the idea that it will be necessary for the SequenceEffect. However, as it turned out, this flag is not as helpful as I thought it would be. In fact, given the user's ability to change it any point, it makes the implementation very error-prone.

To be clear, the ability for each effect to run in reverse remains -- only now it can no longer be triggered by the user manually. Instead, SequenceEffect triggers that ability itself at the alternation point. If there is demand in the future to manually force any effect to run backwards, we could restore this flag, but this would require thorough testing to make it work correctly.

Infinite effects now return duration = double.infinity instead of null, which seems more appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants