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

Make MotionGroup semantics simpler #24

Open
sansumbrella opened this issue Jan 15, 2015 · 6 comments
Open

Make MotionGroup semantics simpler #24

sansumbrella opened this issue Jan 15, 2015 · 6 comments

Comments

@sansumbrella
Copy link
Owner

Working with MotionGroup is not fun. Come up with an alternative. Best-seeming solution is the one from previous iterations of Choreograph: make Timeline a TimelineItem.

Instead of wrapping own timeline, wrap items that are on its parent timeline. Might make it possible to append the grouped animations more simply.

At present, to append a MotionGroup after another one that moves the same output variables, we must use the first group's finishFn to trigger the second group creation. Not a huge deal, but potentially confusing since the groups have their own timelines.

@sansumbrella
Copy link
Owner Author

MotionGroup could compose a shared_ptr<Timeline>. This would make it easy and safe to store local timelines and occasionally add them to a main application timeline.

// Pass along a local timeline
auto my_timeline = make_shared<Timeline>();
main_timeline.addTimeline( my_timeline );
my_timeline->append( … );

// Create a MotionGroup with a new timeline
auto &group = main_timeline.createGroup();
// As a shared_ptr, the group's timeline is safe to store (though the group is not)
auto my_timeline = group.getTimeline();

It would also enable the weird situation where one timeline could be driven by multiple other timelines. That seems like a more reasonable tradeoff than storing all timeline items as shared_ptr's

@sansumbrella
Copy link
Owner Author

If Timelines held shared_ptrs to TimelineItems rather than unique_ptrs, it might make it easier to treat them as TimelineItems. Will need to think about how their duration is counted when adding to other Timelines (or perhaps Timelines by default are retained on other timelines, so their duration isn't as critical a piece of information).

@sansumbrella sansumbrella changed the title Consider changing MotionGroup semantics Make MotionGroup semantics simpler Apr 17, 2015
@sansumbrella
Copy link
Owner Author

WIP on the grouping branch and the grouping_unique branch.

Both enable direct composition of Timelines. Tests passing.

@sansumbrella
Copy link
Owner Author

This changes the semantics of Timelines, since TimelineItems have an idea of where they are in time, and Timelines previously did not. Hopefully, it makes Timelines easier to reason about.

@sansumbrella
Copy link
Owner Author

Proceeding with the grouping_unique branch, which minimizes the number of changes and maintains relative performance.

TODO:

  • Update samples to no longer use MotionGroup.
  • Clean up MotionGroup tests, since they are now Timeline composing tests.
  • Add some Timeline tests to confirm resetting/jumping behaves as expected.
  • Update README to reflect Timeline's knowledge of its playhead.
  • Make Timeline's finish function behave like MotionGroup's finish function did.

Maybe:

  • Cache Timeline duration; mark dirty on TimelineOptions destruction. Require user to explicitly mark dirty when modifying a shared TimelineItem.
  • Mark Timelines/TimelineItems as infinite to avoid calculating duration.

@sansumbrella
Copy link
Owner Author

Still a few loose ends, but this is mostly complete (and the working beginnings are merged in to master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant