-
Notifications
You must be signed in to change notification settings - Fork 10
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
Animation refactor #12
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amclain
commented
Apr 19, 2020
amclain
commented
Apr 19, 2020
doughsay
reviewed
Apr 20, 2020
amclain
force-pushed
the
animation-refactor
branch
from
April 22, 2020 03:38
f680daa
to
34bb0d2
Compare
doughsay
previously approved these changes
Apr 22, 2020
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.
🎉
amclain
force-pushed
the
animation-refactor
branch
from
April 22, 2020 04:21
34bb0d2
to
1648338
Compare
amclain
force-pushed
the
animation-refactor
branch
from
April 22, 2020 04:26
1648338
to
d5e0caa
Compare
doughsay
previously approved these changes
Apr 22, 2020
amclain
force-pushed
the
animation-refactor
branch
from
April 22, 2020 05:32
f25fc42
to
07654b6
Compare
doughsay
approved these changes
Apr 22, 2020
Force-merging through the stuck tests. They passed on my fork here: |
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #8 (comment)
This PR is an attempt at refactoring animations based on my prior comment, #8 (comment). There were a lot of steps taken to get to this point, so if the final diff looks cluttered feel free to review the individual commits to see a cleaner sequence of steps. The goal of this refactor is to push more of the generic animation responsibilities towards
Animation
and away from the ancillary modules likeRGBMatrix
and the implementations of animations (likePinwheel
).A paradigm shift in this PR is to push animations towards a common data model, and formalizes this with the addition of the
%Animation{}
struct and increased opacity around the modules that implement specific animations. An example of the effects of this change can be seen inRGBMatrix
, where animations are initialized and the state changes occur. This new code cares less about which instance of an animation it is working with, and is able to work with animations at a more general level.And although I wasn't able to completely remove the abstract
module.function()
calls, I consolidated them into theAnimation
module, along with the different animation types (formerlyAnimations.list
). This wayAnimation
is responsible for handling all of the internal details of animations, and external consumers of animations work with them in a more opaque manner through theAnimation
module.I also pulled
Animation
and all of the implemented animations out of theRGBMatrix
namespace, which unless I'm overlooking something, should help with the mindset of having animations somewhat decoupled from the physical hardware. More on this in thePixels
section below.Future Expansion
This PR is rather complex so I'm reluctant to add additional scope to it, but in the process of this refactor I have identified some other opportunities.
Simplifying the responsibilities of
Animation
The
%Animation{}
struct contains properties like:speed
and:delay_ms
that look more like the responsibilities of a rendering engine rather than properties of the animation itself. For example, an animation may only need to care about how to change the graphics from one tick to the next, whereas a separate engine could be concerned with how fast/slow an animation plays since any animation should be able to be played faster or slower. If an implementation of an animation needs to speed up or slow down on certain ticks, maybe a speed scaling factor is a better parameter for an%Animation{}
, which could then be used as a relative recommendation for how much the render engine should adjust its speed for any given tick.Pixels
Initializing an
Animation
with the pixels from theRGBMatrix
(animation.ex#L25) feels more tightly coupled than I would like. I haven't thought up a solution to this yet, but wanted to point it out. Maybe the introduction of a render engine like mentioned above could allow animations to operate on an abstract grid, and the engine could apply that abstract grid to the physical grid of pixels in theRGBMatrix
.Introducing a
Frame
to draw on may be another applicable abstraction betweenAnimation
andRGBMatrix
(or a render engine).Animation types
Rather than being hard-coded, the animation types list could be defined in a more flexible manner, like in
config.exs
. This could allow a developer using Xebow to more easily disable animations they don't want in their project, or extend the animations via libraries rather than solely having to rely on contributing animations upstream.With all that being said, I'm curious to hear thoughts on this PR.