T23543: Add support for affine transform animations by smspillaz · Pull Request #4 · endlessm/libanimation

smspillaz · 2018-08-23T13:07:06Z

This branch adds some affine transformation animations and associated helpers.

Stepping the animations is done using Stepper implementations (just a function object at the moment, an interface in the GObject interface). They can be composed as functions of each other, for instance the Reverse stepper is a composition of 1.0f - baseStepper(ms).

All the transform animations themselves are implementations of the TransformAnimation interface - the TransformAnimation class does not have any concrete implementation details. The animations themselves share code through the added header files with utility methods.

The actual matrix math is implemented using glm, a popular C++ header only library used for doing matrix transformations.

https://phabricator.endlessm.com/T23543

smspillaz · 2018-08-23T13:07:44Z

Branch is stacked on #3

If we know that two points are going to be of the same type, it is convenient to be able to compare them with the == operator as opposed to having to use agd::equals

joaquimrocha

Honestly, the whole code is still very confusing to me. So I could only pick on the style here and there.
I am a bit scared about how we will be maintaining all this code. So please @smspillaz don't use one-letter (or non-trivial abbreviations) parameter names, and write a few bits explaining what the transformations do (or point to a URL explaining it when appropriate) as well as a good description for each commit.

joaquimrocha · 2018-09-05T18:52:28Z

@@ -0,0 +1,40 @@
+/*


Commenting here because I want this to be about this commit's message: usually I like when commits explain why they exist. Maybe it becomes very clear with later commits but it'd still be nice to say a few bits about why the changes exist in this and other commits.

Yup, good point. I'll add some more detail to the message.

joaquimrocha · 2018-09-05T18:55:54Z

+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Build the libanimation library (transform animation base component


Nitpick: Seems unusual to see copyright notices on a build file, but if you feel it's necessary that's okay.

I think its a convention on some other endless projects

joaquimrocha · 2018-09-05T19:00:37Z

+  out_extremes[3].x = agd::get <0> (extremes[3]);
+  out_extremes[3].y = agd::get <1> (extremes[3]);
+  out_extremes[3].z = agd::get <2> (extremes[3]);
+  out_extremes[3].w = agd::get <3> (extremes[3]);


Seems like a lot of repeated code. Maybe an inline function would make things easier to maintain and more clear?

I suppose both satisfy the Vector4 trait, so could have aome sort of agd::assign overload for it, yeah

joaquimrocha · 2018-09-05T19:09:09Z

+                                       GParamSpec *pspec)
+{
+  switch (prop_id)
+    {


Why don't give access to the length property here?

Ah yes - to be honest this was something on my "to fix" list but kept falling off.

I'll need to add getters and setters on all the C++ classes I think, which is going to be cumbersome but oh well..

Done (with a macro, at least).

Looks like the only property for this stepper is write-only. So this method should be removed.

joaquimrocha · 2018-09-05T19:14:16Z

+  'stepper.h'
+]
+
+animation_glib_introspectable_sources += files(stepper_introspectable_sources)


You don't really need to declare the sources in a list only to add it here as files. You can declare them inside the files function directly.

True - I suppose we can do:

stepper_sources = files([ 'stepper.cpp' ]) stepper_headers = files([ 'stepper.h' ]) animation_sources += stepper_sources

joaquimrocha · 2018-09-05T19:56:57Z

+            glm::mat4 transform;
+            float progress;
+
+            animation::stepper::Stepper       stepper;


And this one is column aligned with something above I guess? But the transform and progress are not even column aligned between themselves.
It'd be nice to settle on a consistent style. I'd suggest ditching column aligned stuff (precisely because it's difficult to maintain), but it's up to you of course.

joaquimrocha · 2018-09-05T20:01:16Z

+animation_glide_animation_set_property (GObject      *object,
+                                         guint         prop_id,
+                                         const GValue *value,
+                                         GParamSpec   *pspec)


Misaligned parameters (one extra space).

joaquimrocha · 2018-09-05T20:09:16Z

+   * their value. */
+  for (const char * const *property_iter = wanted_properties;
+       *property_iter != NULL;
+       ++property_iter)


If you call it iter, maybe it'll fit all in one line?

joaquimrocha · 2018-09-05T20:13:42Z

+    }
+
+  extended_construct_params[i].pspec = append_pspec;
+  extended_construct_params[i].value = append_value;


Maybe:
You didn't have to initialize i outside of the loop just to use it here since it will be extended_n_construct_params - 1 here, and using this form (or declaring a last_param_index even) would read better.

Makes sense.

I'm a fan of the "initialize when you declare" style, but there's no harm in initializing twice - the compiler will optimize it out anyway.

Oh I see your point. Yeah that's a good idea. I'll do that.

joaquimrocha · 2018-09-05T20:17:32Z

+{
+    namespace math
+    {
+        template <typename T> T clamp (T const &v, T const &l, T const &h)


I can infer what clamp does of course, but I still had to waste more brain power here than if you had written proper names for the variables and this applies to other cases in this PR too.

Makes sense.

Since the PR is rather large, are you able to point those cases out? I will probably miss important ones if I look through myself

Fixed in this case, not sure where the others are

smspillaz · 2018-09-06T05:25:40Z

So please @smspillaz don't use one-letter (or non-trivial abbreviations) parameter names, and write a few bits explaining what the transformations do (or point to a URL explaining it when appropriate) as well as a good description for each commit.

This is definitely useful feedback and I'll take it in board for the next revision.

Unfortunately, since I had to write this in a hurry I didnt leave a lot of time for documentation but its quite clear that more of that is needed, especially around the math parts. So I'll focus on that and on adding more detail to commits (same for the other PR)

smspillaz · 2018-09-09T20:24:30Z

Okay, comments addressed and PR updated.

There's a couple of changes here since the last revision, but the biggest one is that I added support for mutable/readonly properties on the animations themselves and propagated that change to the GObject properties, meaning that all the ones marked readable and writable are actually readable and writable in practice.

The tricky thing here is substituting sensible defaults, especially considering that the default for g_param_spec_object / g_param_spec_pointer / g_param_spec_boxed is NULL. In general such substitution is done in the constructor or in the ForwardValueFromHT conversion functions.

The other tricky detail was steppers - steppers are implemented as closures with mutable state on the C++ side, but in order to wrap them in C we need to copy the closure itself. Unfortunately, due to what I think is a compiler bug (I'll need to check the C++ spec on this, but it'd be surprising if this was the behavior), closure state doesn't get copied when a closure is copy-constructed (eg, all the variables that are part of the closure's capture group just get default-constructed on copy-construction, which seems wrong). As a result, we have to work around that when "getting" the stepperproperty, since "getting" it means wrapping it in a GObject of some sort. Thus, we have a StepperHolder which actually copies (and resets) the underlying stepper and a StepperWrapper which merely observes the underlying stepper and is not meant to outlast it. The comments in the code should be clear about that.

I should note that now that I no longer work at Endless, I won't be able to do much more work on these PRs - just that I promised to address these comments before my departure as part of my work handover. That should be my side of things done now :)

cosimoc · 2018-09-12T21:24:26Z

+G_DEFINE_BOXED_TYPE (AnimationBox,
+                     animation_box,
+                     animation_box_copy,
+                     g_free);


Remove semicolon

cosimoc · 2018-09-12T21:24:48Z

+
+#define ANIMATION_TYPE_BOX animation_box_get_type ()
+
+GType animation_box_get_type ();


Should be (void)

cosimoc · 2018-09-13T03:56:13Z

+  object_class->set_property = animation_transform_animation_set_property;
+  object_class->finalize = animation_transform_animation_finalize;
+
+  animation_transform_animation_props[PROP_INTERFACE] =


Only a note at this point, since I haven't seen how the class gets used, but it feels quite weird to have this as a gobject property, as the whole point of glib bindings is for clients not to access the C++ interface, no?

It is. Actually, it may be easier to not expose it through gobject at all and just keep it as an internal member that gets set at the constructor? That would probably simplify a few things..m

cosimoc · 2018-09-13T04:01:55Z

+        animation::Point invScaleFactor (1.0 / agd::get <0> (scaleFactor),
+                                         1.0 / agd::get <1> (scaleFactor));
+
+        /* Remember that transforamtions are done back to front when postmultiplying.


transformations

cosimoc · 2018-09-13T04:04:40Z

+        auto x1 = agd::get <0> (box.topLeft());
+        auto y1 = agd::get <1> (box.topLeft());
+        auto x2 = agd::get <0> (box.bottomRight());
+        auto y2 = agd::get <1> (box.bottomRight());


I think @joaquimrocha's point is simply about factoring out box.topLeft() and box.bottomRight() into two variables before this block. (I agree)

cosimoc · 2018-09-13T04:52:50Z

+G_DEFINE_BOXED_TYPE (AnimationVector4D,
+                     animation_vector4d,
+                     animation_vector4d_copy,
+                     g_free);


Remove semicolon

cosimoc · 2018-09-13T04:56:43Z

+void animation_glide_animation_set_y_axis_location_unit (AnimationGlideAnimation *animation,
+                                                         float                    y_axis_location_unit);
+
+float animation_glide_animation_get_y_axis_location_unit (AnimationGlideAnimation *animation);


The line spacing for these two methods is mismatched

cosimoc · 2018-09-13T04:57:45Z

+      break;
+    case PROP_TARGET:
+      /* Not readable */
+      break;


Remove these two; if a property is not readable you're guaranteed it won't appear here.

cosimoc · 2018-09-13T04:58:13Z

+      break;
+    case PROP_TARGET:
+      /* Not writable, except on construction */
+      break;


Even on construction this would be invoked though, no?

It is, yeah.

Its one of those weird things where we need to keep it as a construct property in order to pass it to the underlying c++ class constructor but not as a writable thing.

Tbh though they should probably both be writable all the way through - the target position or screen width could very well change during the course of the animation and we should probably know about that...

cosimoc · 2018-09-13T04:58:59Z

+
+typedef struct _AnimationGlideAnimationPrivate
+{
+} AnimationGlideAnimationPrivate;


Does not seem that we use the private struct

smspillaz · 2018-09-13T06:38:02Z

Lets continue the discussion about the interface props here as its a bit of a bigger question and I can chime in here with the problem that I was trying to solve.

Basically, we want the GObject wrappers for all these animations to wrap some instantiation of a C++ interface. This is fine until you get to construction where things get hard quickly.

Right now, the C++ interfaces require certain construction properties in order to be constructed at all. Some of these are construct-only in the C++ sense, meaning that they get passed to the constructor and stay that way for the lifetime of the object. This means that you need to gather up all those properties somewhere and then pass them to the C++ constructor.

The first approach is to just keep a copy of the properties around during the GObject's lifetime and pass them to the C++ object on constructed like done in wobbly/model.cpp here: https://github.com/endlessm/libanimation/blob/master/animation-glib/wobbly/model.cpp#L378 . This comes with the cost of keeping a redundant copy of all those properties around.

Another approach is to construct the C++ object as soon as we have all the construct properties in the constructor, like we do above. However, this comes with a few interesting problems:

We don't have access to the Private data yet, meaning that we need somewhere to store the constructed C++ object
We need to make sure that the C++ object is assigned to the interface member before all the other construction props are set, so that calling the setters on the C++ object works correctly.

I was thinking that we could relax the requirement that properties must be passed to the C++ object on construction and allow it to be default-constructible (setting the properties later using the setters) which solves (1), but then (2) remains. Not sure how to deal with that :(

We'll use them later

The "box" type represents a simple perpendicular quadrilateral in 2D space given by its top left and bottom right co-ordinates (eg, x1, y1, x2, y2).

This is necessary in order to use it with some of the testing operators (like agd::Equals or agd::AlmostEq), since those testing operators use operator<< in order to print out the contents of the type in the event of a matching failure.

We're going to do affine transformations and glm is a particularly well respected library for things like that. Note that the dependency is a submodule, so you'll need to fetch and pull submodules before building.

Steppers encapsulate some progress change within the animation. The typical case is linear stepping, which just interpolates from 0 to 1. However, another case is reverse stepping which is composed by 1.0f - linear (ms). Future steppers can be added to add all sorts of easing parameters. https://phabricator.endlessm.com/T23543

Right now we only have 2D vector types, but some consumers like clutter want bounding volumes in a four dimensional space, so we need a type to represent such bounding volumes.

We'll do some affine transformations based on this one. There's three main methods to override here: - Step() -> Take a single step in the animation by a given number of milliseconds. Returns true if more steps need to be taken, false otherwise - Matrix() -> Get the transformation matrix for the animated surface at this point in the animation. - Extremes() -> Get the four points specifying the 3D bounding volume for this surface, usually transforming the co-ordinates of the surface in scene-relative co-ordinates that have been passed in.

We'll use these later for the affine transformation animations to calculate box offsets. Usually we want to compute where the center of a box is in both absolute co-ordinates and relative co-ordinates. ComputeBoxCenter will do it in absolute co-ordinates and ComputeBoxCenterOffset in co-ordinates relative to the top left corner of the box.

We'll use these later when implementing linear progress based animations. Right now we only have clamp(), used to ensure that the given value sits between some bounds.

These are just some simple macros to define ABI safe property get/set functions. They are accessed by calling the correct overload, for instance, to set properties: animation.Target(target).Source(source) And to get properties: auto const &target = animation.Target();

These helpers are used to quickly append a new "interface" construction prop to the existing construction properties and make it easy to lookup the props inline. Basically, we need these helpers because we are wrapping some C++ interface implementation in a GObject. GObject doesn't require all the properties to be set at construction time, whereas constructing the C++ interface implementation does. So we need to collect all the properties in the constructor and then construct the underlying C++ interface there. Doing this by looking over all the construct properties and assigning values would be cumbersome, especially doing it for every effect implementation. Instead, we put all the construct properties into a hash table (hashed by the property name) and then fetch them in O(1) using the InterfaceConstructor template. This template constructs the "Interface" type (the type passed should be the implementation of the interface you want to construct) and variadically takes in all the construction parameters. Use ForwardValueFromHT to extract a typed value from the given property name from its corresponding GValue in the construct properties hash table. For instance, supposing that MyType's constructor is defined as follows: MyType::MyType (float bar, animation::Point const &baz) { ... } You would use the following in the constructor override for the GObject wrapper: const char *interesting_props = { "bar", "baz", NULL }; g_autoptr(GHashTable) props_ht = static_hash_table_of_values_for_specs (interesting_props, construct_params, n_construct_params); auto *interface = InterfaceConstructor <MyType>::construct ( ForwardValueFromHT (props_ht, g_value_get_float, "bar"), ForwardValueFromHT (props_ht, animation_point_from_gvalue, "baz") ); g_auto(GValue) interface_value = G_VALUE_INIT; unsigned int new_n_construct_params = 0; GObjectConstructParam *new_construct_params = append_interface_prop_to_construct_params (construct_params, n_construct_params, object_class, &interface_value, interface, &new_n_construct_params); object_class->constructor (type, new_construct_params, new_n_construct_params);

This will be useful for printing out the contents of matrices when tests fail. Also necessary if we want to use comparison matchers with them.

This matcher allows the verify that one type instance is "almost equal" to another type instance. The way that is done is that the type must implement the animation::matcher::AlmostEqTrait type trait and define the apply() method.

This is a simple affine transformation that goes from one box to another. The animation runs "from" some source point "to" some target point which is the surface' "natural" position within the scene (eg, if there were a scene graph, the "to" position forms its co-ordinates and its geometry). For instance, if the window was minimizing, then the animation would have to be run in reverse, "from" the minimized point "to" the unminimized point, but stepping backwards.

This animation simulates a gentle bounce on a sine wave from the center of the window outwards. The best way to think of this animation is to think of an attenuating sine wave squeezed inwards by two bounds that converge on a single point. In thise case those lines are running from the "initialScale" to 1.0 and the "maximumScale" down to 1.0. We run the sine wave for 2pi * nBounce iterations (scaling it it to fit within the time range), but the effect of the naimation is scaled according to where we are on the bunds. Now, rotate the attenuating sine wave so that it is facing towards you on the z-axis (OpenGL co-ordinates). This is essentially what the animation is.

This animation rotates the surface from some rotated position towards a resting position where its rotation angle is 0, 0, 0 on all three axes. The axis that the surface can rotate on is configurable in unit-cordinates. Rotation on 0.5, 0.5 rotates at the center of the surface, whereas rotating from 0, 0 would rotate from the top left corner.

smspillaz · 2018-09-13T08:03:25Z

(I addressed the trivial changes since I had a free 30 mins)

smspillaz force-pushed the T23543 branch 3 times, most recently from 4017840 to d6933de Compare August 30, 2018 16:53

smspillaz mentioned this pull request Aug 30, 2018

T23697: Add GridAnimation interface and grid animations #8

Closed

smspillaz closed this Aug 31, 2018

smspillaz reopened this Aug 31, 2018

geometry: Add operator== for point types

a443cbe

If we know that two points are going to be of the same type, it is convenient to be able to compare them with the == operator as opposed to having to use agd::equals

joaquimrocha suggested changes Sep 5, 2018

View reviewed changes

smspillaz force-pushed the T23543 branch 3 times, most recently from 00eec65 to 42c5c69 Compare September 9, 2018 20:19

cosimoc force-pushed the T23543 branch from 42c5c69 to 6657b6b Compare September 12, 2018 21:59

cosimoc suggested changes Sep 13, 2018

View reviewed changes

Sam Spilsbury added 13 commits September 13, 2018 15:55

animation: Make sure to use files([]) directly for toplevel headers

6d06958

animation: Add concept of private headers and sources to build

e1c3566

We'll use them later

animation: Add box type

3468fe2

The "box" type represents a simple perpendicular quadrilateral in 2D space given by its top left and bottom right co-ordinates (eg, x1, y1, x2, y2).

tests: Add ostream operator for Vector4D

75ce54f

This is necessary in order to use it with some of the testing operators (like agd::Equals or agd::AlmostEq), since those testing operators use operator<< in order to print out the contents of the type in the event of a matching failure.

build: Add header-dependency on glm

5ef96f7

We're going to do affine transformations and glm is a particularly well respected library for things like that. Note that the dependency is a submodule, so you'll need to fetch and pull submodules before building.

animation: Add 4D vector type

926f578

Right now we only have 2D vector types, but some consumers like clutter want bounding volumes in a four dimensional space, so we need a type to represent such bounding volumes.

animation: Make sure to install geometry headers

521b4a8

animation: Add math inline helper functions

ef6ff96

We'll use these later when implementing linear progress based animations. Right now we only have clamp(), used to ensure that the given value sits between some bounds.

Sam Spilsbury added 5 commits September 13, 2018 16:02

tests: Add glm_ostream_operators.h

970b17d

This will be useful for printing out the contents of matrices when tests fail. Also necessary if we want to use comparison matchers with them.

matchers: Add AlmostEqMatcher

93253de

This matcher allows the verify that one type instance is "almost equal" to another type instance. The way that is done is that the type must implement the animation::matcher::AlmostEqTrait type trait and define the apply() method.

smspillaz force-pushed the T23543 branch from 6657b6b to 0528c28 Compare September 13, 2018 08:03

smspillaz closed this Feb 28, 2019

smspillaz mentioned this pull request Jul 9, 2019

Add Transform and Grid based animations #10

Open


		#define ANIMATION_TYPE_BOX animation_box_get_type ()

		GType animation_box_get_type ();

Conversation

smspillaz commented Aug 23, 2018

Uh oh!

smspillaz commented Aug 23, 2018

Uh oh!

joaquimrocha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smspillaz commented Sep 6, 2018

Uh oh!

smspillaz commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

smspillaz commented Sep 9, 2018 •

edited

Loading