Implement the morph method of SVG.PathArray #561

RmiTtro · 2016-12-20T11:11:33Z

This pull request adds an implementation to the method morph of SVG.PathArray. When calling morph, if the passed path uses the same commands in the same order as the object's path, then it is easy to animate between the two as mentioned in the SVG spec.

But, when the two paths differ, it is a bit more complex as a conversion need to be performed on the two paths for them to have the same commands. In order to implement this conversion operation, I have used the plugin Interpolate of Inkscape as a reference. Basically, the approach of this plugin is to convert all the segments of the two paths into cubic Bezier curves and then add points on them as necessary for them to have the same commands.

The implementation of this algorithm is in its own file (makepathsmorphable.js) as it is quite lengthy. And to use it, I have added its function (makePathsMorphable) in the SVG.utils object.

To see the it in action: http://plnkr.co/edit/9gjlHvtYbisruPxEZxAu?p=preview.

Also add methods to SVG.Point that allow to perform operations between two points.

coveralls · 2016-12-20T11:13:52Z

Coverage increased (+2.1%) to 88.133% when pulling 1c4e6f0 on RmiTtro:path-morph into 9982651 on svgdotjs:master.

Fuzzyma · 2016-12-20T11:25:37Z

Did you took a look at https://github.com/Fuzzyma/svg.pathmorphing.js? This addon seems to achieve what you want while also allowing different lengths of path commands. Well - but it cannot convert arcs.

Yours looks more clean and has same/more/less features :D. I am biased - dunno what to do ;)

RmiTtro · 2016-12-20T11:36:08Z

Damn, I didn't know you had already one implemented XD. I will have to take a look at it! Le 20 déc. 2016 6:25 AM, "Ulrich-Matthias Schäfer" <notifications@github.com> a écrit :

…

Did you took a look at https://github.com/Fuzzyma/svg.pathmorphing.js? This addon seems to achieve what you want while also allowing different lengths of path commands. Well - but it cannot convert arcs. Yours looks more clean and has same/more/less features :D. I am biased - dunno what to do ;) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#561 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUTFoveviH56FG5SVYCTklBmAt52IDodks5rJ7s1gaJpZM4LRuNh> .

wout · 2016-12-20T13:44:56Z

We chose to provide this as a plugin to keep the size of the library down.

RmiTtro · 2016-12-20T19:09:23Z

It makes sense, I must admit that it requires a lot of code to make this work. But may I suggest that in the library we had some basic functionality for when the paths use the exact same commands. This just require 2 small for loop (basically what I made in the method haveSameCommands and at of the class SVG.PathArray). This basic functionality is the way to go in the SVG spec when animating path.

The method expect the paths to use the exact same commands. It will not attempt to modify them if they do not. Any more complex algorithm shall be provided as a plugin instead in order to keep the size of the library down.

coveralls · 2016-12-21T02:56:12Z

Coverage increased (+0.5%) to 86.531% when pulling 80acdb3 on RmiTtro:path-morph into 9982651 on svgdotjs:master.

RmiTtro · 2016-12-21T03:14:55Z

In this last commit, I remove almost all of my changes and I just keep the code that was in the method at and the method haveSameCommands. As for morph, I implemented the most basic algorithm possible: set the destination attribute only if the passed path uses the same commands in the same order as the object's path. I also keep 2 changes that I made in the SVG.Point class. The first one allows setting both x and y by passing only one number to the SVG.Point constructor. The second one allows passing x and y coordinates to the morph method.

As for the rest of my code, I think I will move it into a plugin so that all my work don't go into the trash XD.

Fuzzyma · 2016-12-21T12:05:55Z

@wout @dotnetCarpenter - So what do you guys think? Imo its a straight forward implementation which does not take more space then other morph methods. Also its well tested. Plugins always can overide behavior so when someone needs another implementation he can just use a plugin

wout · 2016-12-21T12:08:46Z

My only concern is size and since it's not too heavy, I suggest we go ahead and include it into the core.

dotnetCarpenter · 2016-12-22T04:08:18Z

I have only skimmed the code but there is a couple of places that perplex me. I don't really have time to go trough it until Friday, though.

dotnetCarpenter · 2016-12-22T04:10:00Z

src/patharray.js

@@ -100,6 +100,63 @@ SVG.extend(SVG.PathArray, {

    return this
  }
+  // Test if the passed path array use the same commands as this path array
+, haveSameCommands: function(pathArray) {


Not sure why this is needed. Are we looking for a base class, if so then we could use instanceof or do we want to duck type pathArray?

Don't worry, it's nothing that messy! This method just compares 2 path array to see if they use the same path data commands (commands like: M, C, A, etc...). Looking back at the name of this method, I can see how it can be misleading. Maybe I should change it to something else like: useSameCommands, useSamePathCommands, useSamePathDataCommands or something else, I'm open to suggestion.

dotnetCarpenter · 2016-12-22T04:15:40Z

src/point.js

      // store new destination
-      this.destination = new SVG.Point(point)
+      this.destination = new SVG.Point(x, y)


Changing signature but I don't see the signature is changes in the SVG.Point constructor.

Well this change is just to allow morph to be passed the coordinates as parameters. With this change, all of the following are valid ways to call morph:

pt.morph({x:3, y:4}) pt.morph([3, 4]) pt.morph(3, 4)

There is no need to make any changes to the constructor since it can already be passed two parameters. In fact, I made this change mostly to make the method morph more consistent with the constructor.

dotnetCarpenter · 2016-12-22T04:17:28Z

@RmiTtro I'm not against this in any way. Just need some time to understand it before I say ok. But if @wout and @Fuzzyma say it's fine then I trust them, that it's fine.

wout · 2016-12-22T14:27:36Z

I'll have to take a closer look at the functionality. If you can only morph between paths with equal point lengths, it's not a very useful implementation. I'll let you know something later today.

Fuzzyma · 2016-12-22T14:29:07Z

Its the most simple and most fool prooven way to morph paths. As already mentioned: Thats the way svg does path morphing which is also stated in the specs. I see it as fast and simple. Whoever needs more just can use a plugin instead

wout · 2016-12-22T14:48:31Z

Ok, I suppose that's a welcome addition then. Just checked, SVG.js is 16K gzipped, Snap is 27K. So we have space left to add some goodness. :)

Fuzzyma

Added some comments :)

Fuzzyma · 2016-12-23T18:53:34Z

dist/svg.js

@@ -2499,9 +2557,9 @@ SVG.Point = SVG.invent({
      return new SVG.Point(this)
    }
    // Morph one point into another
-  , morph: function(point) {
+  , morph: function(x, y) {


Allows a nice shortcut so I guess its fine ^^

Glad you like it!

Fuzzyma · 2016-12-23T18:53:39Z

dist/svg.js

+    if(this.haveSameCommands(pathArray)) {
+      this.destination = pathArray
+    } else {
+      this.destination = undefined


is this needed? I mean - its undefined anyway. And sice every dude can do something like undefined = true, its rather uncommon to use undefined in an assignment.

This is for the case when a user call morph multiple times, it is to prevent the attribute destination to remain set to a previous value when the test haveSameCommands is unsuccessful. Maybe I should use null instead?

jep - null is imo better

Fuzzyma · 2016-12-23T18:55:30Z

dist/svg.js

@@ -705,6 +705,63 @@ SVG.extend(SVG.PathArray, {

    return this
  }
+  // Test if the passed path array use the same commands as this path array
+, haveSameCommands: function(pathArray) {


In case you wanna rename it: Something like equalCommands?
If not: I, at least, would name it hasSameCommands. Because its one patharray which has the same commands

equalCommands sounds good to me!

Rename the method haveSameCommands to equalCommands and replace undefined by null.

coveralls · 2016-12-23T20:31:37Z

Coverage increased (+0.5%) to 86.531% when pulling c8afde0 on RmiTtro:path-morph into 9982651 on svgdotjs:master.

RmiTtro · 2016-12-23T20:36:53Z

The new commit rename the method haveSameCommands to equalCommands and
replace undefined by null.

dotnetCarpenter · 2016-12-23T20:55:19Z

@RmiTtro I'll try to look this through tonight.

dotnetCarpenter · 2016-12-24T06:31:16Z

Just wondering if we should throw a meaningful error message if the commands are not the same. Maybe hint about https://github.com/Fuzzyma/svg.pathmorphing.js

Simple things going wrong has been confusing for people before and if I just copy/paste from our documentation and remove one parameter for a command, I get lots of Error: <path> attribute d: Expected number, "…100 900 102 898 NaN ".

Come to think of it, shouldn't equalCommands be a guard against such errors, @RmiTtro?

dotnetCarpenter · 2016-12-24T06:31:36Z

Other than that it looks good to me.

RmiTtro · 2016-12-24T09:51:16Z

I don't think its equalCommands role to be a guard against those errors as they happen way before equalCommands is called. Also, don't forget that equalCommands work on paths that have passed through the SVG.PathArray constructor, so if we want to perform some verification on the path, that would be the place to do it.

I would also like to add that it is to be expected that passing a bad formatted path string return an error, especially for the code you copied from the doc since it try to render the passed path string. But, should svg.js implement its own set of custom exceptions is another question entirely. On the positive side, it would allow svg.js to return more meaningful error messages that would help users of this library in their debugging process. On the negative side, that would be a lot of work to implement. That might be something to consider, but that would definitely require its own pull request.

dotnetCarpenter · 2016-12-24T14:14:32Z

@RmiTtro You're right. Now I've played around with the code a little and stepped through the debugger, I see what's going on.

I like to write code a little more terse but that might be unreadable to others. E.g.
Instead of writing this:

morph: function(pathArray) {
  pathArray = new SVG.PathArray(pathArray)

  if(this.equalCommands(pathArray)) {
    this.destination = pathArray
  } else {
    this.destination = null
  }

  return this
}

I would write:

morph: function(pathArray) {
  pathArray = new SVG.PathArray(pathArray)

  this.destination = this.equalCommands(pathArray) ? pathArray : null

  return this
}

I think we should merge in this PR. Who wants to do the honors? 🎅
Happy holidays to your all 🦌

Oh, I almost forgot. We should really document this. Both because it's a new feature but also because we have a plugin (maybe soon two - @RmiTtro?) that does much more work in case the user gets put off by our mini implementation.

wout · 2016-12-24T16:06:20Z

@dotnetCarpenter You go ahead sir :)

As for documentation, I'm nearly ready with the new SVG.js website which really is a documentation page. More on that soon!

Merry everything and Happy always!

Fuzzyma · 2016-12-24T17:56:50Z

I like to write code a little more terse but that might be unreadable to others.

I love doing that but only when it remains clean. In case of the ?: operator I personally prefer the good old if-else. But whatever: merge this already and happy xmas! :P

RmiTtro · 2016-12-24T20:59:09Z

Personally, I also like to use ?: operator. I didn't use it in this case because, originally, that if had more code in it and when modifying my changes I decided to let it live on in case I would have to add stuff to it again.

Anyway, thanks for accepting my pull request and the rest of my code is definitely going into a plugin sometime in the future.

Merry xmas to you all! 🎄

…- closing svgdotjs#561

…- closing #561

Implement the morph method of SVG.PathArray

1c4e6f0

Also add methods to SVG.Point that allow to perform operations between two points.

Implement a more basic morph method for SVG.PathArray

80acdb3

The method expect the paths to use the exact same commands. It will not attempt to modify them if they do not. Any more complex algorithm shall be provided as a plugin instead in order to keep the size of the library down.

dotnetCarpenter reviewed Dec 22, 2016

View reviewed changes

Fuzzyma reviewed Dec 23, 2016

View reviewed changes

Make some fixes in SVG.PathArray

c8afde0

Rename the method haveSameCommands to equalCommands and replace undefined by null.

dotnetCarpenter added a commit to dotnetCarpenter/svg.js that referenced this pull request Dec 25, 2016

update documentation to reflect our new basic path animation support …

75b6352

…- closing svgdotjs#561

dotnetCarpenter merged commit c8afde0 into svgdotjs:master Dec 25, 2016

dotnetCarpenter added a commit that referenced this pull request Jan 14, 2017

update documentation to reflect our new basic path animation support …

0c69855

…- closing #561

Implement the morph method of SVG.PathArray #561

Implement the morph method of SVG.PathArray #561

Conversation

RmiTtro commented Dec 20, 2016

coveralls commented Dec 20, 2016

Fuzzyma commented Dec 20, 2016

RmiTtro commented Dec 20, 2016 via email

wout commented Dec 20, 2016

RmiTtro commented Dec 20, 2016

coveralls commented Dec 21, 2016

RmiTtro commented Dec 21, 2016

Fuzzyma commented Dec 21, 2016

wout commented Dec 21, 2016

dotnetCarpenter commented Dec 22, 2016

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

dotnetCarpenter commented Dec 22, 2016

wout commented Dec 22, 2016

Fuzzyma commented Dec 22, 2016

wout commented Dec 22, 2016

Fuzzyma 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

coveralls commented Dec 23, 2016

RmiTtro commented Dec 23, 2016

dotnetCarpenter commented Dec 23, 2016

dotnetCarpenter commented Dec 24, 2016

dotnetCarpenter commented Dec 24, 2016

RmiTtro commented Dec 24, 2016

dotnetCarpenter commented Dec 24, 2016

wout commented Dec 24, 2016

Fuzzyma commented Dec 24, 2016 • edited Loading

RmiTtro commented Dec 24, 2016

Fuzzyma commented Dec 24, 2016 •

edited

Loading