GNOME Bugzilla – Bug 142318
No preview for blur plug-in
Last modified: 2004-12-22 21:47:04 UTC
Blur plug-in is lacking preview
Created attachment 27582 [details] [review] Patch adds preview to blur plug-in Only the preview area is rendered.
Created attachment 27583 [details] [review] Patch adds preview to blur plug-in Only the preview area is rendered.
This patch uses the deprecated gtk_preview widget. I'm not sure we really want to add previews to existing plug-ins since what we really want is a generic preview widget. Adding previews now we mean more work later. However, if you really feel this is important you should use the GimpOldPreview widget, so it can easily be replaced later. There are several plug-ins that use this widget already.
Using a GtkPreview is perfectly OK since there's no reasonable replacement. I don't think GimpOldPreview should be used since that code is definitely scheduled for removal.
GimpOldPreview was introduced because lots of plug-ins were using (almost) the same code to handle previews. By factoring this code out it becomes a lot easier to replace GimpOldPreview by the final plug-in preview widget. GimpOldPreview can do anything you can do with GtkPreview. Both GtkPreview and GimpOldPreview are of course scheduled for removal, but porting from GimpOldPreview to `FinalPluginPreview' will take less effort than porting from GtkPreview to `FinalPluginPreview'.
Perhaps, but GimpOldPreview is not even a widget and it's API is rather bizarre I don't think we need to insist on plug-ins using it. If people want to use a widget to display a preview, they should have the choice to use GtkPreview.
IMO the GtkPreview is a easy way to implement a preview window. It's not difficult to use and it serves it's function well. Just render whatever you want to display in the preview and draw it. The rendering should be left to the plugin maybe there should be some general support functions to render a buffer but it should not be included in the widget.
Huh? It should not be included in the widget? Are we talking about the same thing? In the long run GtkPreview is not an option. We need to provide a replacement widget. I would suggest we start with a widget that provides the functionality of GtkPreview plus the ability of handling an alpha channel. That would remove the burden to deal with the checkerboard from the plug-in author.
Maybe I didn't put it correct in words. What you are saying is exactly what I intended. At first keep the widget as simple as posible yet provide enough functionality to display from a buffer/drawable/pixelregion. I've seen some discussions where scaling/panning etc. was to be provided by the widget. This would probably neat features but not allways desirable. A plugin like the unsharp mask would take a lot of time to render a complete continious panable view.
That's why the plug-in isn't supposed to render anything before it is asked by the preview widget to render a certain part. That's the basic idea of the preview widget we have in mind and that's what the widget created by Ernst Lippe is doing.
help me out a bit please. Is it correct that the widget's doc is located at : http://refocus.sourceforge.net/preview It is not yet in CVS and therefore not yet to be used?
It's not yet in CVS. There are still some open issues about the API that is used.
So, then it's ok to add prviews with the GtkPreview widget keeping in mind that the rendering function should the same for the preview as for the final render. One suggestion thow for the preview : the patch I've made for the unsharp mask renders a larger buffer then displayed in the preview. Is this foreseen in the new preview widget. When for instance rendering the unsharp mask the radius can become larger then a preview window (if the preview window is relative small) if only the preview size is rendered then you've got a big impact on the accuracy
I'd like to add a widget to replace GtkPreview but if there's no interest from your side you can continue to use GtkPreview for now.
I tried to apply this patch but it seems to be corrupted: Hunk #1 FAILED at 83. Hunk #2 FAILED at 98. Hunk #3 FAILED at 107. Hunk #4 FAILED at 142. Hunk #5 FAILED at 238. Hunk #6 FAILED at 268. Hunk #7 FAILED at 412. Hunk #8 FAILED at 442. Hunk #9 FAILED at 449. Hunk #10 FAILED at 536. Hunk #11 FAILED at 553. Hunk #12 FAILED at 620. Hunk #13 FAILED at 657. Hunk #14 FAILED at 729. Hunk #15 FAILED at 755. Hunk #16 FAILED at 770. patch unexpectedly ends in middle of line patch: **** malformed patch at line 488:
Created attachment 28375 [details] [review] Update preview patch Update applied same changes as in unsharp preview
Created attachment 28376 [details] [review] patch after some smaller cleanups
I haven't applied this change because it seems that at least with higher values for Repeat the plug-in is quite slow and the preview makes it almost impossible to use. Sometimes the progressbar goes wild, sometimes the plug-in just stalls for a while. IMHO this is not an improvement.
well euh, IMHO it's the repeat that is the problem. If You add a repeat to other plugin's like the unsharp mask, despeckle they will all get to slow to use the preview. Maybe when showing the blur dialog we could make shure that the preview is not repeated more then once. And if the repeat count gets to high disable it.
I am aware that it is not the fault of your preview code but we cannot add a preview to the plug-in if it makes the plug-in basically unusable. Is this plug-in used at all? I have never found it particularly useful but then I am not an artist.
I would use it if it was not randomized for instance on a photo to soften it. By only using a 3x3 kernel this should be realy fast (have a look at the standard blur in photoshop. tadada: it has no preview). The thing that realy slows down the plugin is in fact the randomize function. Honestly I do not see any use of it either you apply a standard blur or use a more enhanced blur like the gaussian blur or apply a unhsarp mask. To conclude : for photoshop users I'would keep the menu option and make the plugin apply the blur (3x3 kernel) without any dialog. That way the result of the plugin is predictable and no preview is needed. btw : the preview isn't scrolling I've forgot to change /* * Setup for filter... */ - preview_x1 = x1 + pivals.delta_x; - preview_y1 = y1 + pivals.delta_y; + preview_x1 = x1 + delta_x; + preview_y1 = y1 + delta_y;
Yes, my version has this fixed already. Your suggestion about making Blur a simple and fast Blur w/o a dialog sounds like a good plan to me.
Seems like this is seconded in bugreport #134088. And even a good proposition was made to make the blur adaptive. I' would say to the author of the message go for it.
Your patch did also do some changes to the query() function that effectively revert changes we've done to the plug-in since 2.0. This part should not be applied. (This is mainly a note to myself).
Created attachment 28393 [details] [review] patch to plug-ins/common/blur.c Here is a patch that changes "blur" to make it run a single iteration, with no randomization, without popping a dialog -- also as previously suggested the menu entry is renamed "Soften". The patch also fixes a typo that has existed since 2.0.0, which caused the first row of the result to be corrupted. Patch is against current CVS, and was tested in 2.1.0. It is rather large because a lot of code became unnecessary.
I wonder if we need to provide the old functionality for backward compatibility on the PDB level. However it doesn't look like any of the scripts in the distribution use plug-in-blur-randomize nor plug-in-blur. It would certainly be a good idea not to rename the plug-in-blur procedure to plug-in-soften but to stick to the established name. Can we get a patch to fix the typo for 2.0.2 ?
Created attachment 28395 [details] [review] patch for plug-ins/common/blur.c from 2.0.1 Patch to fix typo that causes corruption of first row of image. Your comments on the previous patch seem right to me.
I would prefer to not change the menu entry to soften keep the menu entry as "blur". This should be fine for current gimp users and would also feel familiar to photoshop users.
I agree that it should probably better be kept as Blur. I've applied the fix for the first row to the stable branch: 2004-06-07 Sven Neumann <sven@gimp.org> * plug-ins/common/blur.c: fixed typo that caused corruption of first row. Spotted by William Skaggs, bug #142318.
Why to decrease the already existing functionality? I would better prefer the dialog with preview to see the results. Add new plugin, call it Blur. And rename old one into something like "Random blur". And don't forget about uniform preview dialog for it :).
Simply because the randomize and repeat functionality has proven to be slow, hard to maintain and to use. It is very difficult to add a preview to it and it's really not worth the hassle unless someone can point out a reasonable use case for this code. Also we are trying to simplify our menus. Suggesting to add yet another entry to the Blur menu is counter-productive.
Alexander: First, randomness is blurring is not actually useful. Second, it is a theorem that repetitively applying the "blur" convolution more than a few times gives a result that is indistinguishable from a Gaussian blur -- but the Gaussian blur filter is much faster. So nothing useful is being removed. On the other hand, once this filter can be applied with a single keystroke, it becomes quite handy for quickly and easily softening the contrast in an image a little bit. Also I think that having a filter called "Blur" in the "Blur" submenu is not good, but the point is not important enough to fret over.
William, since noone seriously objected, I think you can commit your patch that removes the parameters and the blur dialog now. Please stick to "plugin_blur" and "Blur" for the procedure name and menu entry though. Also, please remove the inclusion of header files that become redundant by this change (gtk.h). If you want to, please also change the dependency for the Blur plug-in in plug-ins/common/plugin-defs.pl and run mkgen.pl to recreate Makefile.am.
2004-06-16 Bill Skaggs <weskaggs@primate.ucdavis.edu> * plug-ins/common/blur.c: removed randomize and repeat options; made to run without popping a dialog. (bug #142318) 2004-06-16 Sven Neumann <sven@gimp.org> * plug-ins/common/plugin-defs.pl: changed dependencies for blur. * plug-ins/common/Makefile.am: regenerated. * plug-ins/common/blur.c: no need to include libgimpui.h any longer.