Animations refactor #54
Conversation
doughsay
force-pushed
the
refactor/effects
branch
from
020bdb5 to
c17128d
Compare
Do we want to keep the layout design issue open to explore things like the keyboard-layout-editor format? |
Sure I guess; or we can create a new issue for that. The issue specifically states supporting non-grid layouts, which this PR addresses, even though we don't use it. |
| @type any_color_model :: | ||
| Chameleon.Color.RGB.t() | ||
| | Chameleon.Color.CMYK.t() | ||
| | Chameleon.Color.Hex.t() | ||
| | Chameleon.Color.HSL.t() | ||
| | Chameleon.Color.HSV.t() | ||
| | Chameleon.Color.Keyword.t() | ||
| | Chameleon.Color.Pantone.t() |
There was a problem hiding this comment.
ℹ️ (optional)
I wonder if the color model is actually a property of LED. 🤔 Not sure yet.
| try do | ||
| value = String.to_existing_atom(bin_value) | ||
| cast(option, value) | ||
| rescue | ||
| ArgumentError -> :error | ||
| end |
There was a problem hiding this comment.
ℹ️ (optional)
How about something like Enum.member? rather than try/rescue?
String.to_existing_atom might also be brittle. Wouldn't this return a false positive if I went into IEx and typed the atom name, then validated again?cast() when is_atom(value).
Also, the BEAM only allows a finite number of atoms to be created. We may want to validate options either by converting them to strings during the validation process, or storing them as strings in the first place (convert atoms to strings at compile time if we still want an animation designer to be able to use them in code).
tl;dr cast the options to binary, validate binary == binary.
There was a problem hiding this comment.
I don't think that's necessary? The whole reason to use String.to_existing_atom is to protect against leaking atoms... I understand the rescue is not ideal... Anyway, let's address this in a later refactor.
There was a problem hiding this comment.
This is a partial review, with more to come later. There are a few questions I have, some places where the documentation and typespecs should be improved.
I haven't finished going through RGBMatrix.Engine, yet. I have some confusion about RGBMatrix.Animation.Config and its submodules, but I need to look those over some more before I can ask anything. I'll sleep on it and pick this up tomorrow (today).
There was a problem hiding this comment.
😅 Finally finished. I don't see any hard blockers. 🎉
All of my comments are optional, although the recommended (:warning:) ones would be nice to address before merging if you have the bandwidth. If not, we can start tackling the items as tech debt when the PR is merged.
There was a problem hiding this comment.
Here's my concluding batch of comments on this. All of them are just comments, and you're free to take or leave them as you like.
I think we might want to try and simplify the logic of how rendering happens, maybe, but that's something that needs discussion.
Cheers!
Co-authored-by: Jesse Van Volkinburg <[email protected]>
Closes: #42, Closes: #51
TODO: