Simon!

[doughsay]

What's your high score!?

[vanvoljg]

I like it! A lot!

I made a couple changes on the version I'm running here.

This is all 100% optional, but this configuration is what I'm running right now, and the game feels better. There's more distinction between the call and response phases, and the flash between color displays gives the brain a signal that something different is happening.

[amclain]

What's your high score!?

Like... 8? 🙈

This is a cool and addictive animation. The question I'm mulling over is if Simon should be an animation, or if it should be an application. I can see pros and cons to each. The animation implementation is like a micro game, and it has limitations to what it can do with only interact and render as inputs and outputs. Simon as an application would have access to the entire system; It could be playing sounds along with the animations, tracking a Live View leaderboard, etc.

Not sure I have an answer yet, but this implementation does have novelty.

Also trying to weigh if we want to add another animation before tackling the tech debt of cleaning up the interfaces around animations, config, engine, etc. It's one more file to refactor that has to retain its functionality.

[amclain]

Looks good!

[amclain]

Isn't it an %Animation{} that's passed to render?

Suggested change

	def render(%{state: {:playing_sequence, []}} = state, _config) do
	def render(%{state: {:playing_sequence, []}} = animation, _config) do

[doughsay]

No, this is also the source of some confusion from your previous code review of the refactor (this is why the callbacks use any for state, not Animation.t(). I wanted to avoid passing in the actual animation struct in the refactor to keep things simpler (i.e. the animation implementer knows exactly what they're getting because they defined it themselves.)

This is open for discussion.

[doughsay]

Also, I might have contributed to that confusion here, sorry! There's actually a field called state inside the State struct. So starting from the animation struct it actually looks like animation.state.state 😅

[amclain]

I think this might have loose type checking unless it's %State{}.

[doughsay]

Not sure what this means?

[doughsay]

The question I'm mulling over is if Simon should be an animation, or if it should be an application.

We don't currently have any way to achieve this game other than as an animation? There's no support in Engine for arbitrary other applications to draw stuff to the paintables. The ONLY way for us to paint right now is to adhere to the animation behaviour.

I'm open to discussion about how to change this, but I had to work within the confines of what's possible right now.

Also trying to weigh if we want to add another animation before tackling the tech debt of cleaning up the interfaces around animations, config, engine, etc. It's one more file to refactor that has to retain its functionality.

The cost of the future refactor aside, I did this on purpose. The point is to show more possible ways to use the animation behaviour so that a refactor doesn't remove possibilities. This is exactly what happened last time and what caused this entire disrupt in the first place. Because I only originally had 60fps constant math-based animation examples to begin with, the last refactor made it so ONLY that kind animation was possible.

I'm trying to come up with as many outlandish animation ideas I can NOW so that a refactor doesn't destroy my ability to do so again in the future.

Next up: WPM heatmap! 😝 (kinda kidding, not actually working on this... lol)

[amclain]

The question I'm mulling over is if Simon should be an animation, or if it should be an application.

We don't currently have any way to achieve this game other than as an animation? There's no support in Engine for arbitrary other applications to draw stuff to the paintables. The ONLY way for us to paint right now is to adhere to the animation behaviour.

I'm open to discussion about how to change this, but I had to work within the confines of what's possible right now.

I think we do support this currently. If the application creates animations for what it wants to do, which is a solid colored pixel for Simon, it can hand those animations to the engine as it wants to play them. Here's some pseudo-code which may help illustrate what I'm trying to describe:

defmodule Simon do
  use GenServer

  # Pseudo-code - Boilerplate omitted...

  def init(_) do
    state = start_game()
    {:ok, state}
  end

  defp start_game do
    %State{
      sequence: [] # Sequence of LEDs that light up this round
      # ...
    }
    |> add_to_sequence()
    |> play_sequence()
    |> start_timer()
  end

  defp add_to_sequence(state) do
    new_sequence_element = %{
      location: random_location(),
      color: random_color()
    }

    new_sequence = state.sequence ++ [new_sequence_element]

    %State{state | sequence: new_sequence}
  end

  defp play_sequence(state) do
    state.sequence
    |> Enum.each(fn element ->
      animation = Animation.Implementation.Point.new(
        location: element.location,
        color: element.color,
        duration_in_ms: 500
      )

      Engine.play_animation(animation, async: false)
    end)
  end
end

[doughsay]

Here's some pseudo-code which may help illustrate what I'm trying to describe:

Engine.play_animation(animation, async: false)

We no longer support async: false. And I renamed it from play_animation to set_animation exactly for this reason: it's not "play this animation please and get back to me when it's done", it's "set this as the current animation".

I get your point though. It's just super clunky no? I'd rather Engine expose a painting interface directly, so instead of having to define and play an animation, you just say: "hey engine, paint these pixels these colors".

[amclain]

We no longer support async: false.

Really what I'm getting at in the pseudo-code is that the app has a way to know when to switch to the next animation. There are a handful of options for how we could do that.

It's just super clunky no?

Could you expand on this? I'm not quite following.

I saw this pseudo-code as pushing Simon "the game" into the application layer and away from modules like Animation and Engine. I think this is simplifying the architecture and making each module easier to reason about. It's separating the game (user sees and replays a pattern) from the animation (display a color on a pixel).

Simon "the animation" may appear lighter weight on the surface, but it's limited (or should be limited) in what it can do. It should never overreach the boundary of being an animation, because its render function is in the engine's hot path. It should never do non-animation things, like understand 1 or 2 player mode, play sound effects (if the Keybow had sound), or track/push stats to a leaderboard. As an animation, all it should do is describe the colors of pixels over time. The Simon animation as it's implemented is a great novelty that's fun to play; I think the temptation will be to expand out Animation and Engine when building more games (or other apps), which I think is an anti-pattern.

I'd rather Engine expose a painting interface directly, so instead of having to define and play an animation, you just say: "hey engine, paint these pixels these colors".

I don't have a good feeling about that; it would result in the engine exposing two different paradigms for how to paint pixels. Engine already has Animation as an input, and since anyone can implement an animation and output what they want in its render function, that's the painting interface. The boundaries of responsibility here are that animations paint pixels; the engine is responsible for which animation is being rendered, when it's rendered, and passing the output to devices that can display those pixels.

If we need a low-level pixel painting interface that's available in the application layer, I recommend we create an Animation that exposes more of the low-level details in its interface. For example, here's pseudo-code to illustrate the concept of using an animation that lets you paint on whatever pixels you want to:

Animation.Implementation.Paint.new(pixels: %{
  {0, 0} => "red",
  {0, 1} => "yellow",
  {1, 0} => "orange",
  {1, 1} => "green"
})

I think the more we can keep each module focused on a specific responsibility, the easier the system will be to reason about, as well as maintain.

[doughsay]

Yeah I think all that makes sense. But my original point still stands: the point of this experiment was to show another way to use the animation behaviour, with the specific intent of making sure that any future refactors maintain the ability to build such an animation. Just because we shouldn't use an animation to implement Simon, doesn't mean we shouldn't have the ability to. Does that make sense?

[amclain]

Just because we shouldn't use an animation to implement Simon, doesn't mean we shouldn't have the ability to.

[vanvoljg]

Fixed the bugs introduced by my earlier suggestions.

[doughsay]

Added invalid tag; we're going to re-work this as an "application" instead of an animation.