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

Animation API refactor #49

Open
vanvoljg opened this issue Jul 3, 2020 · 6 comments
Open

Animation API refactor #49

vanvoljg opened this issue Jul 3, 2020 · 6 comments
Labels
refactor Refactoring code or tech debt repayment

Comments

@vanvoljg
Copy link
Member

vanvoljg commented Jul 3, 2020

Currently, animations are hard-coded in the Animation module. We should think about some way of populating the list of types and the list of animations dynamically, based on the animations which exist at startup.

Good topic for the next planning meetup.

https://github.com/ElixirSeattle/xebow/blob/abb857f8d76ed5ac2b969de1e81911972b2e8c63/lib/xebow/animation.ex#L39-L55

@amclain
Copy link
Member

amclain commented Jul 4, 2020

Thanks for the point-out! There's something else I noticed while reviewing this code. Looking at animation_type and types...

https://github.com/ElixirSeattle/xebow/blob/abb857f8d76ed5ac2b969de1e81911972b2e8c63/lib/xebow/animation.ex#L39-L43

https://github.com/ElixirSeattle/xebow/blob/abb857f8d76ed5ac2b969de1e81911972b2e8c63/lib/xebow/animation.ex#L45-L55

types does not return all of the animations like its description states; it's missing __MODULE__.Static. The wider problem is that the solution is not to add this type to the types list, or else Xebow will cycle to a boring static animation with nothing happening (and that needs to be initialized slightly differently, with a color fill). So I think what might be going on here is that Animation.types is actually Xebow-specific application logic that snuck into Animation.

@amclain amclain added the refactor Refactoring code or tech debt repayment label Jul 4, 2020
@doughsay
Copy link
Member

doughsay commented Jul 6, 2020

Another thing to keep in mind: I'm adding animation configuration options in #54. The configs are persistable (to disk or wherever). I suggest that instead of having a "list of all available animations + base configs", we should instead have a dynamic list of user-chosen animations + custom configs. We can start with defining them in config.exs with the intent to eventually make it runtime configurable.

Maybe this idea is a little farther in the future, but maybe not? Food for though...

P.S. I removed Static in my refactor, so the issue regarding it goes away (albeit with the new issue of: how do we replace it).

@amclain
Copy link
Member

amclain commented Jul 6, 2020

we should instead have a dynamic list of user-chosen animations + custom configs

👍

This may be low-hanging fruit with toolshed's load_term! and save_term!:

https://hexdocs.pm/toolshed/Toolshed.Misc.html#load_term!/1

@amclain
Copy link
Member

amclain commented Jul 6, 2020

I removed Static in my refactor, so the issue regarding it goes away (albeit with the new issue of: how do we replace it).

I may have a few ideas based on when I was trying to refactor it towards the Effect-based system. tl;dr It may be conflating too many concepts and we could split it into effects like:

  • Solid - All LEDs are a solid color
  • Dot - One pixel is a certain color (very flexible once the engine can stack & blend animations)
  • Sequence - A pre-defined sequence of frames to cycle through (which is where the pain of Static came from that we weren't using yet, except now we have internal state that can hold that sequence)

@doughsay
Copy link
Member

doughsay commented Jul 6, 2020

Solid - All LEDs are a solid color

I have that one already: https://github.com/ElixirSeattle/xebow/pull/54/files#diff-ad03796b3c85bd75b4b87dba1bfc0582R1

@doughsay
Copy link
Member

doughsay commented Jul 6, 2020

This may be low-hanging fruit with toolshed's load_term! and save_term!:

https://hexdocs.pm/toolshed/Toolshed.Misc.html#load_term!/1

Yeah! This is really convenient, exactly what I wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring code or tech debt repayment
Projects
None yet
Development

No branches or pull requests

3 participants