PropertyAnimation #51

carson-katri · 2018-01-21T04:21:08Z

Add the PropertyAnimation (convenience). I added it to the scroll_layer as an example implementation. I plan to go through and animate a lot of things.

Also made animation_destroy actually free the animation.

jwise · 2018-01-21T05:16:32Z

rwatch/ui/animation/animation.c

@@ -60,9 +63,14 @@ void _animation_update(Animation *anim)
    if (progress > anim->duration) {
        /* Ok, we're done. */
        if (anim->impl.update)
-            anim->impl.update(anim, ANIMATION_NORMALIZED_MAX);
+            //anim->impl.update(anim, ANIMATION_NORMALIZED_MAX);


This seems incorrect.

jwise · 2018-01-21T05:19:04Z

rwatch/ui/animation/animation.c

        if (anim->impl.teardown)
            anim->impl.teardown(anim);
+
+        // Free the prop anim
+        if (anim->property_anim)


Hmm, I wonder if it's possible to have animation itself not depend on property_animation. I think that, for instance, this property_animation_destroy could be part of an AnimationImplementation's teardown routine -- and similarly, the update routine, the same. In particular, I'm kind of thinking that an AnimationImplementation should contain an Animation -- not the other way around?

jwise · 2018-01-21T05:19:41Z

rwatch/ui/animation/property_animation.c

+    new_rect.origin.y = from->origin.y + (((to->origin.y - from->origin.y) / 100.0)*progress);
+    if (property_animation->impl.accessors.setter.gpoint)
+        property_animation->impl.accessors.setter.gpoint(layer, new_rect.origin);
+    //layer_set_frame(layer, new_rect);


remove comment

jwise · 2018-01-21T05:23:22Z

rwatch/ui/animation/property_animation.c

+    GRect *from = &property_animation->values.from.grect;
+    GRect *to = &property_animation->values.to.grect;
+
+    GRect new_rect = GRect(from->origin.x + (((to->origin.x - from->origin.x) / 100.0)*progress), from->origin.y + (((to->origin.y - from->origin.y) / 100.0)*progress), from->size.w + (((to->size.w - from->size.w) / 100.0)*progress), from->size.h + (((to->size.h - from->size.h) / 100.0)*progress));


Better to multiply through by progress first, then divide by 100. Floating point arithmetic is very expensive. i.e., (to->origin.x - from->origin.x) * progress / 100. Or, better, to just integrate the progress change there: (to->origin.x - from->origin.x) * distance_normalized / ANIMATION_NORMALIZED_MAX (which works, because we know that x * ANIMATION_NORMALIZED_MAX will not be larger than a uint32_t.)

One way that I did made this easier in Dali Pebble was to define a linear-interpolation macro, along the lines of:

#define LERP(a, b) ((a) + ((b) - (a)) * distance_normalized / ANIMATION_NORMALIZED_MAX)

Which can then be used later as: GRect(LERP(from->origin.x, to->origin.x), LERP(from->origin.y, to->origin.y) ...).

jwise · 2018-01-21T05:23:51Z

rwatch/ui/animation/property_animation.c

+    GRect *to = &property_animation->values.to.grect;
+
+    GRect new_rect = *from;
+    new_rect.origin.x = from->origin.x + (((to->origin.x - from->origin.x) / 100.0)*progress);


Similar in this case -- avoid floating point arith, by doing your interpolation as fixed point.

jwise · 2018-01-21T05:25:14Z

rwatch/ui/animation/property_animation.c

+    new_gcolor.g = from->g + (((to->g - from->g) / 100.0)*progress);
+    new_gcolor.b = from->b + (((to->g - from->b) / 100.0)*progress);
+    new_gcolor.a = from->a + (((to->a - from->a) / 100.0)*progress);
+    if (property_animation->impl.accessors.setter.gcolor)


One thing of note -- perhaps we can save all of this computation entirely by returning early if the setter is null?

jwise · 2018-01-21T05:25:46Z

rwatch/ui/animation/property_animation.c

+    property_animation->impl.accessors.setter.uint32(uint32, (int)(((to - from) / 100.0)*progress));
+}
+
+void property_animation_update_gcolor8(PropertyAnimation * property_animation, const uint32_t distance_normalized)


~~local functions can be static~~

oh I didn't realize this is an API function, ignore that

jwise · 2018-01-21T05:30:33Z

rwatch/ui/animation/property_animation.c

+    property_animation->subject = subject;
+
+    // Check the type of the values
+    if (((GRect *) from_value)->size.w != NULL)


I think we can just store pointers to from_value and to_value, rather than having to guess the type

They'll probably be destroyed when the function the animation is created in returns (maybe I'm thinking of that wrong). It depends on how the developer wrote their part.

jwise · 2018-01-21T05:32:14Z

rwatch/ui/layer/scroll_layer.c

@@ -29,6 +29,8 @@ ScrollLayer *scroll_layer_create(GRect frame)
    return slayer;
 }

+PropertyAnimation *prop_anim;


Er, hmm. This should presumably be a property of the ScrollLayer structure, right?

jwise · 2018-01-21T05:33:28Z

Looks really good. I left a handful of relatively small comments that I anticipate you should be able to address readily. Thanks!

(and use it in the scroll_layer)

carson-katri · 2018-01-23T22:39:55Z

Okay I made your changes. Should be good to go.

jwise · 2018-01-24T07:27:01Z

rwatch/ui/animation/property_animation.c

+
+void property_animation_update_grect(PropertyAnimation * property_animation, const uint32_t distance_normalized)
+{
+    if (property_animation->impl.accessors.getter.grect != NULL && property_animation->impl.accessors.setter.grect != NULL)


(informational; I'll merge anyway)

Better to invert the sense of this:

if (blah == NULL || blah2 == NULL) return; everything else...

Or perhaps:

if (!blah || !blah2) return; everything else...

jwise · 2018-01-24T07:28:09Z

rwatch/ui/animation/property_animation.h

+#include "appmanager.h"
+
+// Use to easily calculate changes
+#define LERP(a, b)  ((a) + ((b) - (a)) * distance_normalized / ANIMATION_NORMALIZED_MAX)


(informational; I'll merge anyway)

Would be better not to expose this symbol in the global namespace (other people may want a LERP define of their own). Better to have this only inside the .c file.

jwise · 2018-01-24T07:29:10Z

Overall looks very clean. Good work!

carson-katri force-pushed the propertyanimation branch from 1fb501c to 5896210 Compare January 21, 2018 04:45

jwise reviewed Jan 21, 2018

View reviewed changes

carson-katri force-pushed the propertyanimation branch from 16a5301 to 639cc83 Compare January 23, 2018 00:43

PropertyAnimation

c0dfbe5

(and use it in the scroll_layer)

carson-katri force-pushed the propertyanimation branch from d347ed8 to c0dfbe5 Compare January 23, 2018 01:14

jwise reviewed Jan 24, 2018

View reviewed changes

jwise merged commit 11dda6c into ginge:master Jan 24, 2018

carson-katri deleted the propertyanimation branch February 11, 2018 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PropertyAnimation #51

PropertyAnimation #51

carson-katri commented Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018 •

edited

Loading

jwise Jan 21, 2018

jwise Jan 21, 2018

carson-katri Jan 21, 2018

jwise Jan 21, 2018

jwise Jan 21, 2018

jwise commented Jan 21, 2018

carson-katri commented Jan 23, 2018

jwise Jan 24, 2018

jwise Jan 24, 2018

jwise commented Jan 24, 2018

PropertyAnimation #51

PropertyAnimation #51

Conversation

carson-katri commented Jan 21, 2018

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

jwise Jan 21, 2018 • 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

jwise commented Jan 21, 2018

carson-katri commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwise commented Jan 24, 2018

jwise Jan 21, 2018 •

edited

Loading