WIP: Feature/wombat animation #361

elibosley · 2017-08-10T15:06:38Z

PR for #274 .

PR Status

IN DEVELOPMENT

Ready for PR?

I have updated the CHANGELOG with an entry including a description, a link to my profile, and a link to the issue ticket. This change is filed under an Enhancement or Bug Fix, is shown under the relevant release tag name, and is included in my PR branch.
I have run lein run-lint to check for linting errors.

Changes Proposed:

Breaking changes?

Screenshot of feature:

@emilyseibert @oconn @dehli

This will aid in adding a separate draw function and eventually merging the two together`

…ke them reliable

dehli · 2017-08-10T15:19:34Z

src/cljs/wombats_web_client/components/arena.cljs

+    ;; Iterate through all of the arena rows
+    (doseq [[y row] (map-indexed vector arena)]
+      (doseq [[x cell] (map-indexed vector row)]
+        (let


I would put x-coord on this line.

dehli · 2017-08-10T15:27:24Z

src/cljs/wombats_web_client/components/arena.cljs

+  "Given the start and the end dimension object, calculate which direction
+  has movement"
+  [start end]
+  (if (pos? (Math/abs (- (:x start) (:x end))))


This function is a bit confusing and would probably be easier to follow like:

(defn- get-direction-key [{x0 :x} {x1 :x}] ;; you could also use [start end] here (if (= x0 x1) ;; and then pull out the values here. :y :x))

dehli · 2017-08-10T17:12:46Z

src/cljs/wombats_web_client/components/arena.cljs

+        width  (/ (canvas/width  canvas-element) (count (get arena 0)))]
+
+    ;; Iterate through all of the arena rows
+    (doseq [[y row] (map-indexed vector arena)]


So I think this could still be done cleaner by combining draw-arena-canvas into draw-arena-canvas-animated.

I think the best way would be to sort all of the elements by the order that they should be drawn (such that the animated elements are always drawn after the non-animated). You would need to merge in their x/y coordinates. Then you can simply draw the element at the correct location. Does that make sense?

dehli · 2017-08-11T17:17:22Z

What's the reasoning for still having the two for loops?

elibosley · 2017-08-11T17:19:11Z

@dehli the draw arena method is still being used - I haven't depreciated it yet so the dual loops are still used for rendering the game-play, not the simulator.

dehli · 2017-08-11T17:51:51Z

src/cljs/wombats_web_client/components/arena.cljs

+          (* height y))}
+    {:x (if (= direction-key :x)
+          (* width
+             (+ -1


dec should work here.

elibosley · 2017-08-18T16:52:27Z

@dehli What's the status with this PR? If you get a chance to review this in the next few weeks, I'd be more than happy to update the code and make sure everything is good to go!

dehli · 2017-08-18T17:23:01Z

Hey @elibosley! It's on my backlog. I will get around to this either this weekend, or sometime next week! Hope you had a good week! 😄

elibosley · 2017-08-19T01:14:38Z

Awesome! Thanks, I had a great week! 😆

dehli

Hey @elibosley! Here's some suggestions. I think it's a good start! Feel free to address these whenever you have time and I'll do another review afterwards. Good luck with classes!

dehli · 2017-08-21T19:39:35Z

src/cljs/wombats_web_client/components/arena.cljs

  ;; Draw background first
-  (draw-background canvas-element x y width height)
+  (when (= background true)


You could just use (when background

dehli · 2017-08-21T19:40:19Z

src/cljs/wombats_web_client/components/arena.cljs


  (let [{contents :contents
         meta :meta} cell
         cell-type (:type contents)]
-
+    (when (nil? cell-type)


Remove these prn calls.

dehli · 2017-08-21T19:46:40Z

src/cljs/wombats_web_client/components/arena.cljs

+  ;; step size is neg if moving up or left, so wrap to the largest dimension,
+  ;; this logic is reversed because step-size is calculated as the opposite
+  ;; sign when wrapping occurs
+  (if (pos? step-size)


This would be a lot easier to follow if you had the (pos? step-size when you actually calculate the values for :x and :y. Additionally, it would allow you to remove code that you have replicated.

dehli · 2017-08-21T19:47:32Z

src/cljs/wombats_web_client/components/arena.cljs

+  "Add local :x and :y coordinates to arena matrix"
+  [arena]
+  (map-indexed
+   (fn [y row] (map-indexed


Indentation is off in this function.

dehli · 2017-08-21T19:50:55Z

src/cljs/wombats_web_client/components/arena.cljs

+  (get-in item [:contents :uuid]))
+
+(defn- dimensions
+  [item]


This function isn't needed since item already has the proper attributes.

It does - but I need the previous dimensions to be stored somewhere - I think I figured it was easier to just pull both at once. Would you suggest putting all of that information on the :cell attribute for the vector?

Answered down below

dehli · 2017-08-21T20:00:46Z

src/cljs/wombats_web_client/components/arena.cljs

+        ;; Get the coordinates of the animated items on the grid
+        prev-coords (filter-arena prev-frame-locs [:zakano :wombat])
+        next-coords (filter-arena next-frame-locs [:zakano :wombat])
+        canvas-element (.getElementById js/document canvas-id)


I think it would be better to use ref instead of (.getElementById

dehli · 2017-08-21T20:02:04Z

src/cljs/wombats_web_client/components/arena.cljs

+;; Need to get two sets of coordinates -
+;; one to run wombat off the screen, one to wrap the new wombat on
+
+(defn- draw-arena-canvas-animated


I think this method draws all cells (not just animated). If so, it would make sense to rename this method.

dehli · 2017-08-21T20:05:51Z

src/cljs/wombats_web_client/components/arena.cljs

+                             :step-size step-size})]
+                ;; Draw a blank tile before the animated item to fix
+                ;; weird drawing issues
+                (draw-background


You shouldn't need to draw a blank tile if the elements are ordered according to when they should be drawn.

dehli · 2017-08-21T20:07:16Z

src/cljs/wombats_web_client/components/arena.cljs

+  ;; this when subtracts one from the end because the animation runs once
+  ;; before it gets a callback
+  (when (<= (:progress animation-progress) (dec (:end animation-progress)))
+    (.requestAnimationFrame js/window


I think it would be cleaner to just do a forceUpdate every animation frame so that the arena completely rerenders.

Not sure how to do this - would you replace the .requestAnimationFrame ? and what would I call force-update on to make sure I could increment the animation progress?

What I'm thinking (might not actually work) is doing the following:

(.requestAnimationFrame js/window (.forceUpdate this))

Then you're triggering a rerender every animation frame so you don't need to have any logic inside of the callback. Does that make sense?

dehli · 2017-08-21T20:09:01Z

src/cljs/wombats_web_client/components/arena.cljs

+                total-step-size (get-step-size start end direction-key)
+                step-size (/ total-step-size frame-time)]
+            (if (> (Math/abs total-step-size) 1) ;; wrap-around
+              (let [cell-coords (get-wrapped-coord


Why do we have all these coords? I'd imagine just two would be enough for wrapping (current coord, and wrapped coord), otherwise we'd just need one coordinate.

So one set of coords draws the wombat out of the frame and the other one draws it in - that was the best way that I could see to have the transition be smooth - lmk what you think!

So I guess my question is what is cell-coords, vs. wrapped-coords, vs. same-side-wrap. Makes sense though!

elibosley · 2017-09-27T17:37:52Z

Ugh so I'm finally getting around to this and having crazy setup issues with Ubuntu - hopefully I can figure out what's wrong with my configuration but I just figured I'd update you about it!

dehli · 2017-09-27T18:24:27Z

Sounds good! @elibosley Let me know if you need anything!

elibosley · 2017-10-03T05:24:44Z

@dehli Should I fork this repo to push my commit back up? I've updated the code a bit but I'm not sure how I should get it back into a PR

dehli · 2017-10-03T12:33:41Z

Hey @elibosley! You can just push directly to this branch and it will update the PR appropriately 😄

Eli Bosley added 14 commits August 1, 2017 11:37

Added initial framework for animating wombat movement

72680f6

Started work on cell-based animation

7e9a53b

Added wombat locating ability

d916d68

Merge branch 'develop' into feature/wombat-animation

9267eab

Fixed merge issue

2542bc0

Added function to remove the animated items

e3968f6

This will aid in adding a separate draw function and eventually merging the two together`

Updated with initial data structure for an animation object

a3a2ab2

More animation work

7b93779

Animations slightly working, need to tweak some data structures to ma…

19b38f1

…ke them reliable

Animations working - wrapping logic and frame-redrawing needs work

040ed74

Changed an atom to use reduce

0b4856a

Beginning work on animating every frame

761f9b8

refactoring for animation to work differently

5b5880c

Animations re-implemented in a much simpler way

9e0d221

elibosley requested a review from dehli August 10, 2017 15:06

dehli changed the title ~~Feature/wombat animation~~ WIP: Feature/wombat animation Aug 10, 2017

dehli added the in progress label Aug 10, 2017

dehli reviewed Aug 10, 2017

View reviewed changes

Eli Bosley added 2 commits August 10, 2017 11:19

Fix build issues

f8801ae

Fixed formatting issue

c9b5065

dehli reviewed Aug 10, 2017

View reviewed changes

Updated a function to be more clear

13947f5

dehli reviewed Aug 10, 2017

View reviewed changes

Eli Bosley added 7 commits August 10, 2017 13:28

Slightly better drawing

d37c819

Refactored to use a 1d array for drawing the canvas

e7ac7cb

Fixed long lines

d98d5dc

Fixed a couple styling issues

c3be54f

Initial wrapping support

5605e94

Wrapping animated on second half

c2d2621

First part of wrapping done

c0811a8

Added exit animation

e8cff8e

dehli reviewed Aug 11, 2017

View reviewed changes

Eli Bosley added 3 commits August 11, 2017 14:17

Updated arena to be drawn with draw-arena-animated

2bdfc70

Removed draw-arena-canvas

3944c05

Changed some -1 to dec

61f9c37

dehli suggested changes Aug 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Feature/wombat animation #361

WIP: Feature/wombat animation #361

elibosley commented Aug 10, 2017

dehli Aug 10, 2017

dehli Aug 10, 2017 •

edited

Loading

dehli Aug 10, 2017

dehli commented Aug 11, 2017

elibosley commented Aug 11, 2017 •

edited

Loading

dehli Aug 11, 2017

elibosley commented Aug 18, 2017

dehli commented Aug 18, 2017

elibosley commented Aug 19, 2017

dehli left a comment

dehli Aug 21, 2017

dehli Aug 21, 2017

dehli Aug 21, 2017

dehli Aug 21, 2017

dehli Aug 21, 2017

elibosley Oct 1, 2017 •

edited

Loading

dehli Aug 21, 2017

dehli Aug 21, 2017

dehli Aug 21, 2017

dehli Aug 21, 2017

elibosley Oct 1, 2017

dehli Oct 3, 2017

dehli Aug 21, 2017

elibosley Oct 1, 2017

dehli Oct 6, 2017

elibosley commented Sep 27, 2017

dehli commented Sep 27, 2017

elibosley commented Oct 3, 2017

dehli commented Oct 3, 2017

WIP: Feature/wombat animation #361

Are you sure you want to change the base?

WIP: Feature/wombat animation #361

Conversation

elibosley commented Aug 10, 2017

PR for #274 .

PR Status

Ready for PR?

Changes Proposed:

Breaking changes?

Screenshot of feature:

Choose a reason for hiding this comment

dehli Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dehli commented Aug 11, 2017

elibosley commented Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

elibosley commented Aug 18, 2017

dehli commented Aug 18, 2017

elibosley commented Aug 19, 2017

dehli 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

elibosley Oct 1, 2017 • 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

elibosley commented Sep 27, 2017

dehli commented Sep 27, 2017

elibosley commented Oct 3, 2017

dehli commented Oct 3, 2017

dehli Aug 10, 2017 •

edited

Loading

elibosley commented Aug 11, 2017 •

edited

Loading

elibosley Oct 1, 2017 •

edited

Loading