Add live-view with real-time animation view #56

doughsay · 2020-07-06T05:12:09Z

Closes: #39

TODO:

Add animation configuration UI
Fix register_paintable and unregister_paintable
Investigate what happens when paintable functions accumulate and don't unregister
Live-view color-picker component (nice-to-have / stretch goal)

amclain

Requesting changes because I think I see a bug in inform_all_configurables and would like you to verify before merging.

That being said, I've got the UI running on the hardware and it looks really cool! Nice work! 😍 🎉

This PR is huge, so I'm going to submit this partial review now and pick it up again when I can. It would also be nice to have a second reviewer on this if possible since there are so many changes. Due to both the size of this feature and how valuable it is, I'm erring on the side of being lax in this review (optional changes instead of recommended/required) with the expectation that we'll incur tech debt. If we can at least get the UI merged in, the rest of the team will be able to help with the tech debt, and we'll have a working demo of the UI synced with the hardware.

amclain · 2020-07-07T04:08:09Z

lib/xebow_web/router.ex

+    pipe_through :browser
+
+    live "/", PageLive, :index
+    live "/matrix", MatrixLive, :index


⚠️ (recommended)

How about putting this at "/" since it's the centerpiece of the UI?

yes, definitely!

amclain · 2020-07-07T04:09:09Z

lib/xebow_web/router.ex

+
+    scope "/" do
+      pipe_through :browser
+      live_dashboard "/dashboard", metrics: XebowWeb.Telemetry


ℹ️ (optional)

How do you feel about moving this to /telemetry, since we may want /dashboard for user stuff in the future?

this was just for fun, and it's the standard route used by phoenix; I guess we can move it anywhere we want?

Yeah, it can go wherever we want it by changing its path here. We don't have to move it yet, but I thought I'd point it out in case you wanted to.

amclain · 2020-07-07T04:22:03Z

config/config.exs

+  http: [port: 80, ip: {0, 0, 0, 0}],
+  url: [host: "xebow.local", port: 80],
+  server: true,
+  secret_key_base: "M6xyyGOeCywsLjrSclRl8aNucNyqPe6JV2g3nZIs2+S+NZ2TujWfIL8T69qwYC+G",


ℹ️ (no action required)

We should generate and persist the secrets to flash at some point, otherwise everyone with a xebow will share the same ones.

Could we just regenerate it at compile time?

I think that would affect a user who has info for the system saved in their browser when they go to flash a new version of firmware. 🤔

Phoenix config in general needs a full cleanup, including what's needed for running on host. I'll make an issue to track it.

amclain · 2020-07-07T04:30:25Z

lib/rgb_matrix/animation/config.ex

@@ -65,7 +65,11 @@ defmodule RGBMatrix.Animation.Config do
        schema = schema()

        Enum.reduce(params, config, fn {key, value}, config ->
+          # TODO: better key casting


⚠️

This would be good to log as tech debt (refactor) if it gets merged.

amclain · 2020-07-07T04:40:08Z

lib/rgb_matrix/engine.ex

+  Register a paint function for the engine to send frames to. This function is
+  idempotent.


ℹ️ (optional)

FWIW, when I wrote this doc the second sentence intentionally started on the second line. The first sentence was the summary and the second was ancillary information. Plus ending a line with a single word is the equivalent of a code smell for typographers. ^link 😉

However, mix docs will ruin that formatting anyway, so do what you will with this. I was trying to make the source code look nice.

It won't ruin it if there's an empty line. There was an example farther down where it wasn't on its own line, so I wasn't sure of your intent.

As for formatting: yeah I get your point, but I use a plugin in my editor that auto-wraps comments and docstrings at line 80; I can't (easily) put in exceptions, because the formatter will just always re-format them...

There was an example farther down where it wasn't on its own line, so I wasn't sure of your intent.

Eh, do what you will with it.

amclain · 2020-07-07T05:37:24Z

lib/rgb_matrix/engine.ex

+  def handle_call({:register_configurable, configurable}, _from, state) do
+    state = add_configurable(configurable, state)
    {:reply, :ok, state}
  end
+
+  @impl GenServer
+  def handle_call({:unregister_configurable, key}, _from, state) do
+    state = remove_configurable(key, state)
+    {:reply, :ok, state}
+  end


ℹ️

In one of these functions the second element of the tuple is named configurable, and in the other it's named key. Same with add_configurable and remove_configurable below.

Ok, you've explained this in another comment. key = handle to the registered function. 👍

lib/rgb_matrix/engine.ex

amclain · 2020-07-07T05:54:19Z

lib/xebow/leds.ex

+  @impl GenServer
+  def terminate(_reason, state) do
+    SPI.close(state.spidev)
+    Engine.unregister_paintable(self())


⚠️ (recommended)

If you capture the paintable function in the state, couldn't you use it to unregister here? If you've got the handle to an anonymous function it should match itself, like if you're trying to find and delete it out of the list of registered paintables.

iex(1)> f = fn -> "it works" end #Function<45.97283095/0 in :erl_eval.expr/5> iex(2)> f == f true iex(3)> g = fn -> "it works" end #Function<45.97283095/0 in :erl_eval.expr/5> iex(4)> f == g false iex(5)> h = g #Function<45.97283095/0 in :erl_eval.expr/5> iex(6)> h == g true iex(7)> h == f false

Yes, this would clean things up a lot and remove the confusion over the use of a "key".

amclain · 2020-07-07T06:00:34Z

mix.exs

@@ -53,7 +54,8 @@ defmodule Xebow.MixProject do
      dialyzer: "do cmd mkdir -p _build/#{Mix.target()}_#{Mix.env()}/plt, dialyzer",
      "docs.show": "do docs, cmd xdg-open doc/index.html",
      loadconfig: [&bootstrap/1],
-      upload: "cmd ./upload.sh"
+      upload: "cmd ./upload.sh",
+      setup: ["deps.get", "cmd npm install --prefix assets"]


⚠️ (recommended)

It would be nice to alias "cmd npm install --prefix assets" and run it with mix firmware so the static assets compile into the firmware image.

It's possible the --prefix assets version of this npm command won't run correctly on Windows. The Phoenix docs are pretty explicit about that warning. I'm not sure if it's an npm version thing, or something else.

I think this should also run node assets/node_modules/webpack/bin/webpack.js --mode development.

The sequence recommended by the Phoenix generator is deps.get, cd assets && npm install && node node_modules/webpack/bin/webpack.js --mode development.

Also, it seems to be using a target of host when Mix gets the deps, so at least that task must be run with MIX_TARGET=keybow in the environment.

Previously captured in #58.

Also, it seems to be using a target of host when Mix gets the deps, so at least that task must be run with MIX_TARGET=keybow in the environment.

FYI we'll ideally want to target host for development of the UI because it'll be a faster development cycle to see the changes locally than to flash the hardware each time we want to test changes.

Captured in #46.

amclain · 2020-07-07T06:11:11Z

lib/xebow_web/live/matrix_live.ex

+      register_with_engine!()
+    end
+
+    {config, config_schema} = Engine.get_animation_config()


ℹ️

Tying this back to one of my prior comments, this feels like animations are being coupled to the engine when they probably shouldn't be. This feels more like wanting to get information from the application. How does this subtle change in naming feel when making this function call?

{config, config_schema} = Xebow.get_current_animation |> Animation.get_config

But wouldn't the implementation of Xebow.get_current_animation need to reach into the engine state to get the current animation struct anyway?

Animations are an RGBMatrix concern, not a Xebow concern right?

I think we're having some confusion over what owns what, let's have a realtime conversation about this.

But wouldn't the implementation of Xebow.get_current_animation need to reach into the engine state to get the current animation struct anyway?

What I'm trying to point out with this name is that the engine doesn't have to be responsible for providing public information about the current animation. The application layer (Xebow in this example) could do that.

I think we're having some confusion over what owns what, let's have a realtime conversation about this.

Definitely. We should probably work out a new architecture diagram for what the system should look like as part of that discussion.

Oh, I've also got a crazy idea that involves storing the animations, or more specifically their config, in an ETS table if we need a better model of shared access to them (wrap the ETS access with helper functions, like in Animation, to expose a clean public interface). I think ETS tables are good at shared read access but we need to validate that, because I also hear people tend to create bottlenecks around ETS and we don't want to fall into that trap.

Indeed, http://erlang.org/doc/man/ets.html#new_2_read_concurrency

I think I'm starting to see what you're pushing for, but there's still a big problem. The animation struct Animation.t() is what holds the state, which is constantly updating, 60 times per second for some animations. The engine HAS to be the thing that holds on to that struct and passes it in and receives it back from the animation implementation.

If you want the Xebow application to be responsible for which animation is the currently selected one and what its current config it, then we may need to split this struct up into two: Animation.t() which holds the config and can be owned by Xebow and ActiveAnimation.t() which can hold the current state and is owned by Engine. But this starts feeling a bit weird to me, because then they're not linked and can theoretically become out of sync. Xebow could lose track of which animation is actually being currently "run" by the engine...

It truly feels to me that Engine 100% owns the current animation, it's state, and it's configuration.

vanvoljg

The live view portion has brought something out in the way the render happens. When there are a bunch of live views, the render loop seems to bog down. It works fine with a single live view, but gets slower with more rendering targets.

vanvoljg · 2020-07-07T07:02:09Z

mix.exs

@@ -53,7 +54,8 @@ defmodule Xebow.MixProject do
      dialyzer: "do cmd mkdir -p _build/#{Mix.target()}_#{Mix.env()}/plt, dialyzer",
      "docs.show": "do docs, cmd xdg-open doc/index.html",
      loadconfig: [&bootstrap/1],
-      upload: "cmd ./upload.sh"
+      upload: "cmd ./upload.sh",
+      setup: ["deps.get", "cmd npm install --prefix assets"]


It's possible the --prefix assets version of this npm command won't run correctly on Windows. The Phoenix docs are pretty explicit about that warning. I'm not sure if it's an npm version thing, or something else.

I think this should also run node assets/node_modules/webpack/bin/webpack.js --mode development.

The sequence recommended by the Phoenix generator is deps.get, cd assets && npm install && node node_modules/webpack/bin/webpack.js --mode development.

Also, it seems to be using a target of host when Mix gets the deps, so at least that task must be run with MIX_TARGET=keybow in the environment.

vanvoljg · 2020-07-07T07:20:16Z

config/config.exs

+  secret_key_base: "M6xyyGOeCywsLjrSclRl8aNucNyqPe6JV2g3nZIs2+S+NZ2TujWfIL8T69qwYC+G",
+  render_errors: [view: XebowWeb.ErrorView, accepts: ~w(html json), layout: false],
+  pubsub_server: Xebow.PubSub,
+  live_view: [signing_salt: "JbJukpOp"]


We should consider disabling code reloading. There probably won't be much iteration happening directly on the device, and it's more overhead we don't need.

Suggested change

live_view: [signing_salt: "JbJukpOp"]

live_view: [signing_salt: "JbJukpOp"],

code_reloader: false

Good catch, except we'll still want reloading on for local development. In other words we want the reloader on for Mix.target == :host.

A good way to do this would be in separate config files for the host and keybow configs...

FYI, @doughsay has a convention of organizing the config files here that's worked:
https://github.com/doughsay/keyboard/tree/master/firmware/config

mix.exs

@@ -53,7 +54,8 @@ defmodule Xebow.MixProject do
      dialyzer: "do cmd mkdir -p _build/#{Mix.target()}_#{Mix.env()}/plt, dialyzer",
      "docs.show": "do docs, cmd xdg-open doc/index.html",
      loadconfig: [&bootstrap/1],
-      upload: "cmd ./upload.sh"
+      upload: "cmd ./upload.sh",
+      setup: ["deps.get", "cmd npm install --prefix assets"]


amclain

I finished reviewing the files I wasn't able to get to yesterday. 😅

amclain · 2020-07-07T17:24:32Z

lib/xebow_web/live/matrix_live.ex

+      current_animation_index: 0
+    ]
+
+    {:ok, assign(socket, initial_assigns)}


ℹ️ (optional)

Since the live view doesn't need to remember the state of the LEDs (i.e. hold them in memory for that socket) after it's pushed an update, should we add them to the temporary_assigns?

I had no idea this was a thing. Can we do this in the performance refactor PR?

amclain · 2020-07-07T17:30:57Z

lib/xebow_web/live/matrix_live.ex

+  end
+
+  @impl Phoenix.LiveView
+  def handle_info({:render_config, {config, config_schema}}, socket) do


ℹ️ (optional)

Rather than :render_config, what do you think of a name like :config_updated or :animation_config_updated? If I've got this correct, that's what happens to cause this message to be received.

No this is actually the event telling the live-view to update the config UI. Yes you're right it's triggered when config is updated, but this specific event is "please redraw the config, thx". The message is being sent to itself from inside the config_fn that was registered with the engine. I don't feel strongly though, so if you like another name, that's fine...

amclain · 2020-07-07T18:19:39Z

lib/xebow_web/live/matrix_live.ex

+  # TODO: the following two are duplicated between `Keyboard` and here.
+
+  @impl Phoenix.LiveView
+  def handle_event("next_animation", %{}, socket) do
+    next_index = socket.assigns.current_animation_index + 1
+
+    next_index =
+      case next_index < Enum.count(socket.assigns.animation_types) do
+        true -> next_index
+        _ -> 0
+      end
+
+    animation_type = Enum.at(socket.assigns.animation_types, next_index)
+
+    RGBMatrix.Engine.set_animation(animation_type)
+
+    {:noreply, assign(socket, current_animation_index: next_index)}
+  end


ℹ️

The point of #50 was to keep this logic out of the engine. 😉

We can refactor it again. The design idea is that the engine is responsible for playing animations. Knowing which animations are available would be an application responsibility and would tell the engine which animation to play with play_animation(animation). So the application manages which animation should be played next when a user wants to cycle through them.

(cc @vanvoljg)

Not sure I follow this comment, I am not adding this logic back to Engine here. This is still outside Engine. It's just a secondary copy of what should be an application concern.

amclain · 2020-07-07T18:27:33Z

lib/xebow_web/live/matrix_live.ex

+      end
+    end
+
+    :ok = Engine.register_paintable(pid, paint_fn)


ℹ️ (optional)

Here's another idea for how to write this which keeps the implementation details closer to the call to register the function. I've also modified the return value to receive the anonymous function, since in one of my other comments I mentioned we could unregister by passing the function handle into the corresponding unregister_ function.

pid = self() {:ok, paint_fn} = Engine.register_paintable(fn frame -> if Process.alive?(pid) do send(pid, {:render, frame}) :ok else :unregister end end)

Note that I removed pid from the Engine.register_paintable args. Can't you use the result of pid = self() on line 108 in the anonymous function without passing the pid into register_paintable as an arg?

amclain · 2020-07-07T23:26:43Z

lib/xebow_web/templates/layout/root.html.leex

+    <meta http-equiv="X-UA-Compatible" content="IE=edge"/>
+    <meta name="viewport" content="width=device-width, initial-scale=1.0"/>
+    <%= csrf_meta_tag() %>
+    <%= live_title_tag assigns[:page_title] || "RGBMatrix", suffix: " · Phoenix Framework" %>


⚠️ (recommended)

Considering this is the title of the application/product, how about removing Phoenix Framework from the page title and changing the suffix to - Xebow? (i.e. display a title like Dashboard - Xebow)

Or use a flashy product name for the UI like Xebow Studio. 😁

nickdichev · 2020-07-08T02:09:31Z

lib/xebow_web/live/matrix_live.ex

+
+  @impl Phoenix.LiveView
+  def handle_info({:render, frame}, socket) do
+    {:noreply, assign(socket, leds: make_view_leds(frame))}


I've found this pattern of updating the assigns directly in the LiveView can get unwieldy as the project gets larger. A new pattern I'm following that I like more is to create a "model" module which manages the state of the live view and updating with Phoenix.LiveView.update/3.

You can see an example in this blog post: https://hostiledeveloper.com/2020/06/19/organizing-liveview-logic-with-presentation-models.html

We can refactor this later, just something to keep in mind!

@nickdichev that's a cool example in the article! I like how it improves the readability of the handle_* functions as well!

nickdichev · 2020-07-08T02:15:30Z

lib/xebow_web/live/matrix_live.ex

+  def handle_event("next_animation", %{}, socket) do
+    next_index = socket.assigns.current_animation_index + 1
+
+    next_index =


do you think it reads cleaner to do next_index = mod(socket.assigns.current_animation_index + 1, len(socket.assigns.animation_types)) on line 68?

I copy-pasta'd this from keyboard.ex. I don't know what @vanvoljg's original reason was to remove the use of mod, but it turned out to remove a dependency and a file. (I delete Xebow.Utils, because nothing needed mod anymore; there is still an RGBMatrix.Utils that has mod in it that's used by the animations, but the entire RGBMatrix namespace is eventually going to be pulled out into a separate library, so using its mod here would be awkward. I really wish Elixir had a mod built in that acted like all other languages...)

The logic was to make it more readable. It does the same as what mod would have done, since there's a conditional in there, too, but @amclain thought this was more explicit, and I agreed. Plus, it means we don't need to use the additional utils.

The logic was to make it more readable [...] @amclain thought this was more explicit

Yeah, my reasoning is that it would be easier for someone to understand the value is rolling over if they see it set back to 0, rather than having to decipher the result of mod.

the entire RGBMatrix namespace is eventually going to be pulled out into a separate library, so using its mod here would be awkward

That turned out to be the other nice benefit. I think we legitimately need mod in the animation implementations due to animation math things (that's a technical term). It felt less necessary on this side of the app when handling the rollover, so it was nice to be able to drop the dependency on it.

nickdichev · 2020-07-08T02:16:59Z

lib/xebow_web/live/matrix_live.ex

+  def handle_event("previous_animation", %{}, socket) do
+    previous_index = socket.assigns.current_animation_index - 1
+
+    previous_index =


same mod trick works here, -1 mod 4 = 3 (assuming len(animation_types) = 4 for example)

vanvoljg · 2020-07-10T02:05:03Z

lib/xebow_web/live/matrix_live.html.leex

+  <%= for led <- @leds do %>
+    <span
+      class="<%= led.class %>"
+      <%= if led.class == "key" do %>
+      phx-click="key_pressed"
+      phx-value-key-id="<%= led.id %>"
+      <% end %>
+      style="
+        left: <%= led.x %>px;
+        top: <%= led.y %>px;
+        width: <%= led.width %>px;
+        height: <%= led.height %>px;
+        background-color: <%= led.color %>
+      "
+    ></span>
+  <% end %>


This has me concerned. I checked my messages in both Chrome and Firefox (just to make sure) and clocked the data transfer for ONE keypress of Solid Reactive with default settings at ~290Kb. It sends one frame every ~17 ms, and each one was 1622 bytes ... It adds up quick!

Heartbeat was less intensive.

I wonder if there's a way to make each display tell the engine how frequently it wants updates, and/or reduce the amount of data transferred in each diff.

Nice debugging so far!

The biggest question in my mind is if LiveView is only supposed to be sending the properties that have changed when an update is performed, why does it not seem to be doing it in this case? If the property changing during an animation is background-color, why is a bunch of other data being sent down the wire when the LED colors are updated? For updating 12 LEDs each with a 6 character hex color value and serialized to JSON, with a little extra overhead in there, I would expect 100-200 bytes to be in the ballpark. We've probably done something unintentionally do deoptimize LiveView's diffing. I don't see anything in the docs that stands out yet.

I wonder if there's a way to make each display tell the engine how frequently it wants updates, and/or reduce the amount of data transferred in each diff.

If we can't figure out how to optimize our LiveView code, then we could debounce/drop frames in the LiveView process. This could either be done in handle_info, in which case the LiveView process will still have the overhead of receiving the messages in its mailbox, or in the callback function, which could drop frames before send is called.

vanvoljg · 2020-07-10T06:13:31Z

config/config.exs

+  http: [port: 80, ip: {0, 0, 0, 0}],
+  url: [host: "xebow.local", port: 80],
+  server: true,
+  secret_key_base: "M6xyyGOeCywsLjrSclRl8aNucNyqPe6JV2g3nZIs2+S+NZ2TujWfIL8T69qwYC+G",


Could we just regenerate it at compile time?

Co-authored-by: Alex McLain <[email protected]>

amclain

Thanks for fixing the bug! I believe all of my other comments were optional, so I'm approving.

doughsay · 2020-07-12T01:45:21Z

Thanks for fixing the bug! I believe all of my other comments were optional, so I'm approving.

There're still things that I wanted to clean up; working on them now.

doughsay · 2020-07-12T03:46:05Z

lib/rgb_matrix/engine.ex

+      %State{state | animation: animation}
+      |> schedule_next_render(render_in)

    {:noreply, %State{state | animation: animation}}


oops, this is updating the state struct twice...

Ohhh, that's just fine, nobody will notice! I wonder if the compiler optimized it away...

vanvoljg

I like the sneaky fix.

✔️ Approved!

Add live-view with real-time animation view (WIP)

6c78703

doughsay changed the title ~~Add live-view with real-time animation view (WIP)~~ Add live-view with real-time animation view Jul 6, 2020

doughsay marked this pull request as draft July 6, 2020 05:12

doughsay added the feature A new feature or enhancement to an existing feature label Jul 6, 2020

doughsay added 2 commits July 6, 2020 18:22

Fix paint function (un)registration

9433dd5

Animation configuration!

d01e4b7

amclain assigned doughsay Jul 7, 2020

amclain requested changes Jul 7, 2020

View reviewed changes

vanvoljg reviewed Jul 7, 2020

View reviewed changes

amclain reviewed Jul 7, 2020

View reviewed changes

nickdichev reviewed Jul 8, 2020

View reviewed changes

vanvoljg reviewed Jul 10, 2020

View reviewed changes

Base automatically changed from refactor/effects to master July 11, 2020 21:42

doughsay and others added 2 commits July 11, 2020 18:10

Merge branch 'master' into feature/live-view

0ea3416

Apply suggestions from code review

0eb212f

Co-authored-by: Alex McLain <[email protected]>

amclain approved these changes Jul 12, 2020

View reviewed changes

doughsay added 3 commits July 11, 2020 20:01

Merge branch 'master' into feature/live-view

580e3fe

Code review feedback

3dbb789

Fix code format check

fba5f28

doughsay force-pushed the feature/live-view branch from 81fcb3b to fba5f28 Compare July 12, 2020 03:33

doughsay marked this pull request as ready for review July 12, 2020 03:36

doughsay requested review from amclain and vanvoljg July 12, 2020 03:36

doughsay commented Jul 12, 2020

View reviewed changes

amclain approved these changes Jul 12, 2020

View reviewed changes

sneaky fix

b50b44d

vanvoljg approved these changes Jul 12, 2020

View reviewed changes

doughsay merged commit a28a188 into master Jul 12, 2020

doughsay deleted the feature/live-view branch July 12, 2020 04:14

amclain mentioned this pull request Jul 16, 2020

Refactor casting in Animation.Config module #84

Closed

2 tasks

		Register a paint function for the engine to send frames to. This function is
		idempotent.

	live_view: [signing_salt: "JbJukpOp"]
	live_view: [signing_salt: "JbJukpOp"],
	code_reloader: false

Add live-view with real-time animation view #56

Add live-view with real-time animation view #56

Conversation

doughsay commented Jul 6, 2020 • edited Loading

amclain left a comment • edited Loading

Choose a reason for hiding this comment

amclain Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doughsay Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

vanvoljg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

amclain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdichev Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclain left a comment

Choose a reason for hiding this comment

doughsay commented Jul 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanvoljg left a comment

Choose a reason for hiding this comment

doughsay commented Jul 6, 2020 •

edited

Loading

amclain left a comment •

edited

Loading

amclain Jul 7, 2020 •

edited

Loading

amclain Jul 7, 2020 •

edited

Loading

amclain Jul 7, 2020 •

edited

Loading

doughsay Jul 12, 2020 •

edited

Loading

amclain Jul 10, 2020 •

edited

Loading

nickdichev Jul 8, 2020 •

edited

Loading