GNOME Bugzilla – Bug 774807
[PATCH] Port smooth-palette plug-in to Gegl
Last modified: 2016-11-23 06:45:24 UTC
Created attachment 340467 [details] [review] port smooth-palette plug-in to gegl This patch replaces deprecated code (pixel region) by gegl buffer manipulation in floating point precision. Is a gegl operation really needed ? I have also work on a alternative sorting algorithm (not in the patch); the comparison with the current algorithm can seen here (the palettes are at the bottom of the image) : http://pasteboard.co/GVU5K5Vh.jpg This could be added to the plugin, with a new parameter (sorting algorithm).
Not checked out the code yet. Just your image and playing with this plugin a bit on my GIMP build. > Is a gegl operation really needed ? Not sure I can answer this question. What do you mean? We want GEGL-ported code, but not sure we need a specific GEGL operation just for this. Is that what you are asking? > I have also work on a alternative sorting algorithm (not in the patch); the comparison with the current algorithm can seen here (the palettes are at the bottom of the image) First the current sorting algorithm is definitely crap (I can see it on both your example image and in my own tests. Actually I don't really see any sorting done from a user point of view. I see a lot of related colors all spread through the generated image. > This could be added to the plugin, with a new parameter (sorting algorithm). I'm not sure we even need a parameter. I'd be all for just replacing with your new sorting algorithm and getting rid of the old. Unless it actually does something different and useful and I just don't see it? Finally after playing a bit with the plugin, I experienced 2 problems: 1/ I notice the current plugin only search the currently-selected layer. It may make sense, but in the same time, sometimes you want to work on the currently displayed image (as though it were merged as a single layer). This could be an option. I would call it "Sample merged" like in the color picker. 2/ From the plugin description, I initially thought it would create an actual color palette (visible in the "Palettes" dock) whereas actually it creates an image with all colors aligned next to each other. I would prefer that creating a real palette be the default, and there could be an option to make an image instead (because I can see that some people may prefer it). Even better: create a real palette (no option for image creation), but add a feature "Generate image from palette" which could create such an image from any palette and be accessed by right clicking a palette in the Palettes list. This way, we make the feature of palette image usable for every palettes, not only the ones created through this plugin. What do you think? IMO, this could be cool evolution for the feature if you are interested to provide patches. :-)
I think you should first push the attached patch to get rid of deprecated stuff, then we can think of doing more with this 100% useless plug-in :) Also, what do we tell the guy in bug 749220...
> I think you should first push the attached patch to get rid of deprecated stuff, then we can think of doing more with this 100% useless plug-in :) Yes I was actually already reviewing. Apart from all the bogus reordering and other raised issues, that looks good so I just pushed. > Also, what do we tell the guy in bug 749220... Argh! I definitely didn't realize that someone was already working on an earlier plugin. That's totally uncool. He should have had priority. This said, the one we have right now is working now. So that's a good point. And even though I don't see really the big interest of a GEGL operation for this plugin, if it exists, I don't see why not use it if someone made it. We could just get rid of most of the pixel extraction code from the GIMP plugin and use the operation. So if the person in bug 749220 wishes to finish one's plugin, I would be in favor of accepting it in too.
Review of attachment 340467 [details] [review]: The code is good. Pushed. I leave the bug report open for now. If you could submit your patch for a better reordering, I would review and push it as well. After this, I'll make some improvements to make this plugin actually useful. :-) commit 8b25891ddd9a89ef28c723ad70d29661e64bad44 Author: Thomas Manni <thomas.manni@free.fr> Date: Mon Nov 21 11:25:10 2016 +0100 plug-ins: port smooth-palette to gegl plug-ins/common/Makefile.am | 1 + plug-ins/common/smooth-palette.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------- 2 files changed, 70 insertions(+), 52 deletions(-)
Ok as proposed by Thomas on IRC, let's just close this bug. A new bug report has been opened to discuss whether it even makes sense to keep this plugin: bug 774895. As Mitch was noting on IRC, the interest is doubtful because we have a core action which basically does the same, even better.