Curves plugin - Support arbitrary amount of control points

[bebraw]

It would be good if curves plugin would be generic (ie. accept 2 to n cps). Apparently some GIMP plugins use this.

If you want, I don't mind providing a patch for this. Should be fairly easy one.

[meltingice]

Go for it. I'm curious how you plan to implement this.

[bebraw]

Okay. Just glanced at the code. I think I got an initial idea. The curves plugin part is relatively easy. The challenge lies in making it possible to define bezier curves in an arbitrary way. Looks like I'll have to start out with a quadratic curve (defined by three points) and then generalize that. Any better ideas?

[meltingice]

Sorry, accidentally closed this issue. I've merged in your pull requests for the curves filter. Curious if you've had any more thoughts or play with this. A quadratic curve could be a good starting point.

[confile]

@meltingice is there any progress with curves containing more than one point?

[bebraw]

@confile Thanks for the reminder. I had better get this done!

[confile]

@bebraw that would be great I am waiting for your fix!

[bebraw]

Is it ok if I break the API with this? My implementation (de Casteljau) would look like this:

@bezier: (lowBound, highBound, controlPoints...)

So first you define bounds and points after. Alternatively I could use something like

@bezier: (controlPoints, lowBound, highBound)

In this case you would have to wrap your control points within an array though. The first alternative allows you to go without. Either way the API will get broken.

[bebraw]

I think I'll stick with the latter variant for now. After all it makes sense to have lowBound and highBound as optional.

[bebraw]

@confile There's an early version at my branch in case you want to have a look. I haven't tested it a lot. It's based on de Casteljau's algorithm so that it works well with arbitrary amount of points. This is likely slower for low amount of control points (does it matter?) but on the other hand it's very handy for large amount of points so there's that.

Perhaps there's a way to make this backwards compatible after all. We could check whether lowBound and highBound are arrays and then adapt based on that. The implementation would look pretty ugly but I guess it could work.

[meltingice]

:+1: for backwards compatibility, even if it's a bit ugly. If it turns out to be a mess, we can make some API breaking changes with the 5.0 release.

[bebraw]

Ok. I'll set it up so that it's compatible with the old API. We can remove the shim on 5.0 release. Just need to document that.

[confile]

So what will be the syntax?

Is it this:

this.curves('rgb', startPoint, endPoint, [middlePoint1, middlePoint2,..., middlePointN]);

By the way I have some strange error with the curve see: https://github.com/meltingice/CamanJS/issues/112

[bebraw]

So what will be the syntax?

There will be two changes. Both to bezier and to that curves plugin (almost forgot about that, oops). So we'll end up with:

bezier(controlPoints, lowBound, highBound); // controlPoints is an array, minimum of two items. low and high bounds are optional
curves('rgb', controlPoints); // controlPoints is an array (ordered from start to end)

Putting middlePoints after ends seems a bit counterintuitive.

Maybe this will fix #112. That'll make a good test case.

[confile]

@bebraw when will you push the changes so I can test as well.

[bebraw]

@confile I'll resume work tomorrow. Getting kind of late. The work will be available at artitrarycp branch. There's a preliminary version already but it's missing that curves wrapper and the backwards compatibility stuff.

[bebraw]

I didn't have to change curves API after all as it was using ... already. All I need to was to relax certain constraints a bit. Still need to figure out how to shim out bezier in a nice way.

@confile Try giving my build a go to see if #112 is ok now.

[bebraw]

Ok, shimmed it and made a pull request. Give it a go and hit me with some feedback.

[confile]

@bebraw @meltingice There seems to be a bug in the latest commit see here: https://github.com/meltingice/CamanJS/issues/122