T23534: Rename project to libanimation by smspillaz · Pull Request #3 · endlessm/libanimation

smspillaz · 2018-08-09T15:36:33Z

https://phabricator.endlessm.com/T23534

smspillaz · 2018-08-09T15:39:58Z

Argh, looks like I based this work off the wrong fork, hence the mountain of conflicts.

They're resolvable, but I'll need to look into it in the morning, I suspect.

smspillaz · 2018-08-14T06:25:01Z

@@ -1,3 +0,0 @@
-[submodule "wobbly/third_party/allow_move_optional"]
-	path = wobbly/third_party/allow_move_optional
-	url = git://github.com/smspillaz/CopyMoveConstrainedOptional


I should probably split out this change into a separate commit

This isn't being used anywhere

"libwobbly" is now a subset of this library, we're going to be adding more stuff here. Due to the name change the version is also reset to 0.

We'll be using this in other subdirectories

We'll be using them elsewhere.

smspillaz · 2018-08-23T13:09:31Z

Jenkins is failing because it still expects the package name to be libwobbly - merging https://github.com/endlessm/eos-build/pull/844 will fix that.

joaquimrocha

Just a couple of comments. Note that I reviewed this quicker than usual because it was mostly the name replacement, which should be fine.

joaquimrocha · 2018-08-24T09:13:39Z

-# Build the libwobbly library.

-api_version = '0.3'
+api_version = '0'


It'd be fine if you kept it. I assume nothing will depend on the 0.3 version of it then.

Yeah the API version got reset to 0 since we changed the name. Or at least I assume that's how things are meant to work.

joaquimrocha · 2018-08-24T09:24:29Z

+}
+
+AnimationWobblyAnchor *
+animation_wobbly_anchor_new_for_native_anchor_rvalue (wobbly::Anchor &&anchor)


Do you really need to specify the rvalue in the function name?

Probably not, though I guess I just wanted to make it clear that it wants an rvalue reference in case anyone was wondering why things might not compile when they pass it an lvalue :)

I left this as is for now

joaquimrocha · 2018-08-24T09:34:44Z

-                                      wgd::get <1> (tileSize) * column);
-        wgd::pointwise_subtract (start, deltaToTopLeft);
+        animation::Point deltaToTopLeft (agd::get <0> (tileSize) * row,
+                                      agd::get <1> (tileSize) * column);


Nitpick: Misaligned line.

joaquimrocha · 2018-08-24T09:36:32Z

-                                                   config::Width));
+    return animation::PointView <double> (mPoints,
+                                          CoordIndex (x, y,
+                                                      config::Width));


Nitpick: This could go in the line above as well. I don't think the whole code is strictly aligned to 80 chars (I may be wrong though).

It isn't. Well, it used to be but I ditched that rule since it was getting too unwieldy to comply with :)

joaquimrocha · 2018-08-24T09:37:14Z

-#include <animation/wobbly/wobbly.h>    // for PointView, Vector, Point, etc/wobbly_internal.h
+#include <animation/geometry.h>                   // for PointView, PointModel, etc
+#include <animation/geometry_traits.h>            // for assign, scale, etc
+#include <animation/wobbly/wobbly.h>    // for PointView, Vector, Point, etc


Nitpick: You could aligned all the comments too.

Interesting note: Those comments are actually generated by include-what-you-use, which I did have enabled on this project earlier, but turned off since its a pain to manage.

joaquimrocha · 2018-08-24T11:12:36Z

+            auto const x1 = dimension::get <0> (tl);
+            auto const x2 = dimension::get <0> (br);
+            auto const y1 = dimension::get <1> (tl);
+            auto const y2 = dimension::get <1> (br);


If you're changing the corners used for extracting the limits, and you're not changing the comparison, then it means the comparison is wrong now or before. Seems like it was wrong before. Or was it just the name of the vars that was incorrect? If so, then maybe making more emphasis on that fact in the commit message would be good?

In this case, the values themselves didn't actually change, only the names, which were wrong. I'll re-emphasize that in the commit message.

Commit message fixed.

It is 2018, manual headerguards are out of fashion.

Previous, this was "tr" and "bl" for "topRight" and "bottomLeft", but that makes no sense because they actually represented the top left and bottom right values. It should be tl and br.

smspillaz force-pushed the T23534 branch from 537eb63 to c12169c Compare August 14, 2018 06:23

smspillaz commented Aug 14, 2018

View reviewed changes

smspillaz force-pushed the T23534 branch 2 times, most recently from edafedb to 293ea2f Compare August 14, 2018 06:30

Sam Spilsbury added 3 commits August 17, 2018 16:36

wobbly: Remove third_party/CopyMoveConstrainedOptional

0c3f272

This isn't being used anywhere

animation: Rename the entire project to libanimation

54202d9

"libwobbly" is now a subset of this library, we're going to be adding more stuff here. Due to the name change the version is also reset to 0.

animation-glib: Move vector.cpp/h out into animation-glib toplevel

7a2c556

We'll be using this in other subdirectories

smspillaz force-pushed the T23534 branch from 293ea2f to c4318aa Compare August 17, 2018 13:52

Sam Spilsbury added 2 commits August 22, 2018 03:20

animation: Hoist geometry types into animation namespace

4ba6295

We'll be using them elsewhere.

animation: Fixup template argument whitespace in geometry.h

e453fa1

smspillaz force-pushed the T23534 branch from c4318aa to 3644555 Compare August 23, 2018 12:53

smspillaz mentioned this pull request Aug 23, 2018

T23543: Add support for affine transform animations #4

Closed

joaquimrocha approved these changes Aug 24, 2018

View reviewed changes

smspillaz force-pushed the T23534 branch from 3644555 to 9507937 Compare August 24, 2018 15:04

Sam Spilsbury added 4 commits August 29, 2018 03:53

matchers: Hoist matchers into animation namespace

8860bae

animation: Switch to using #pragma once

7dae498

It is 2018, manual headerguards are out of fashion.

geometry: Rename "box" members to something more sensible

a49efa1

Previous, this was "tr" and "bl" for "topRight" and "bottomLeft", but that makes no sense because they actually represented the top left and bottom right values. It should be tl and br.

tests: Hoist ostream_point_operator.h into tests directory

6344ca4

smspillaz force-pushed the T23534 branch from 9507937 to 6344ca4 Compare August 28, 2018 19:54

smspillaz merged commit df59f8f into master Aug 28, 2018

ptomato deleted the T23534 branch August 29, 2018 17:05

Conversation

smspillaz commented Aug 9, 2018

Uh oh!

smspillaz commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants