GNOME Bugzilla – Bug 167926
Layer opacity and combine modes are not used in transform previews
Last modified: 2008-03-07 17:59:36 UTC
1) Open two images 2) Copy one image into the other -- make it a new layer 3) Decrease the opacity 4) Attempt to scale the image. Notice that the layer is shown as 100% opaque during the preview.
Actually, no layer composition modifiers are used at all - neigther opacity, nor the combine mode. And this affects all transforms, not just scaling. Changing summary from "Layer opacity is not used for scale preview" accordingly
It is just a preview. This is not a bug.
I would argue that this is a bug. If the layer is being scaled to match the size of features in another layer, it is important to be able to get the best comparison possible. That sometimes means setting one layer to somewhat transparent. From a coding perspective, it shouldn't be hard to fix.
In order to fix this one would have to completely rewrite the transform preview. Of course it would be nice to have a more accurate preview but this is clearly an enhancement request.
You mean to tell me that you guys didn't make things extensible enough to add an extra channel? Sounds like a rewrite _should_ happen.
If you want to become insulting, please go somewhere else. Someone recently contributed a long-time missing feature which is transform previews. He came up with an implementation that works nicely because it is fast enough to preview the transformation in real-time. This implementation has the drawback that it ignores the position in the layer-stack and most layer properties and just draws on top of the display. If you can come up with a better implementation which is not considerably slower, we will be happy to include it. But please refrain from judging on the code design without even looking at it and without understanding the problems that are to be solved.
Sorry about that....I'm frustrated by a separate project with a bad interface right now. You're in luck, though. I'm taking a graphics course right now and am this > < close to getting the GIMP compiled on my box (diagram is not to scale). As soon as I get it compiled, I'll look into making this an option.
*** Bug 313159 has been marked as a duplicate of this bug. ***
*** Bug 330167 has been marked as a duplicate of this bug. ***
Created attachment 68371 [details] [review] patch to provide preview opacity I've written code that creates transparent previews for the transform tools. It introduces a preview "Opacity" option to the tool option dialog. In my view the other combine modes are not very important when previewing transforms. It is more important to simply see where the thing will end up, thus a distinct "preview opacity" option rather than previewing any of the selection's actual combine modes. If the opacity==255, then basically the old code is used, and is just as fast. Otherwise the preview data is composited with what was on the display already, resulting in slower execution. Perhaps some clever person has a better way to blend pixel data! Right now it is basically doing this a gazillion times: *pptr = (*pptr * (255-opacity) + pixel[0] * opacity) >> 8; The patch also includes some much needed documentation for gimpdrawtool, gimptransformtool, and the matrix bits of libgimpmath.
Can we please get unified diffs instead of context diffs and separate patches for documentation and actual code changes. The patch as is is way too complex to apply it, especially since some parts of it are controversial. I think that we should not implement this as done in this patch. It is too late for such a change in 2.4 anyway and one of the first things we will do when 2.4 is released, is porting the display drawing code to cairo. It should be a lot easier to do transparent overlays with the cairo drawing API than it is now.
I would like to add that we really need more people contributing to GIMP who are capable of writing code on the level of this patch, and I hope you won't be discouraged by Sven's response. This is unfortunately a bad time to make this kind of change, because we are in quasi-feature-freeze for the upcoming 2.4 release, but that shouldn't last too much longer. You also shouldn't necessarily take Sven's response as the final word, since historically something can be "one of the first things we will do" and yet not get done for several years. (But I agree that porting the display code to cairo would have a lot of advantages.)
Well, in any case I just noticed that it works fine for me with the perspective, scale, and rotate tools, but not for Shear! Hardly ever use that tool, so accidentally neglected to test it. The grid lines and opacity slider both get grayed out in the options dialog no matter what the preview type is. Can't imagine why only shear does that at the moment, but this weekend I'll have time to attack it again. After using Gimp almost every other day for the last three years for my art, lack of see-through previews has long been one of my top complaints. I suppose I can always just patch my own 2.4! I'll update the above patch with a "unified" one, minus unrelated documentation, when I can get it to work for all transform tools, just in case anyone other than me is in a hurry for transparent previews. This is my first attempted contribution to any software project other than my own, so please forgive any novice mistakes I make.
If you could submit the documentation patch in a separate bug report, we could try to get at least that into CVS immidiately. One thing you could try for the preview is to hardcode the opacity to 127 and to optimize the blending routine for this. That should allow you to do the blending using an addition followed by a shift and it might give a noticably better performance.
*** Bug 338531 has been marked as a duplicate of this bug. ***
Created attachment 68694 [details] [review] patch to provide preview opacity, take 2 So here's my updated patch for a transform preview opacity. I tried hardcoding half opacity, and it didn't make a huge amount of difference surprisingly. However, I did manage to locate a huge bottleneck. The old preview code did everything per affected row of the window, so I was grabbing each row of pixel data off the window one at a time with gdk_pixbuf_get_from_drawable(), which is apparently very expensive. The current patch fetches the whole section of the window where the bounding box of the preview lies at once, and then renders back only the affected bits of each row. This makes my original patch twice as fast. I wonder if there is some even faster way to fetch what is already on the window, other than gdk_pixbuf_get_from_drawable()? My previous error about the shear tool wasn't actually about the shear tool. It was a gtk_set_sensitive() on the parent of the widget I actually wanted to set sensitive, which presented according to how the tools settings had been previously saved. The patch is for the current cvs, but also seems to work for me on 2.3.10.
That patch is already a lot more readable. But I wonder about a few things: (1) Why do we need to allow the user to change the opacity of the preview? The opacity should be taken from the layer, shouldn't it? And the original layer needs to be temporarily undrawn or the user will see the layer twice. (2) Since the opacity is constant, shouldn't the check for opacity == 255 be moved out of the loop to get better performance for the common case? (3) The check for the area parameter that you added to gimp_display_shell_draw_tri() looks suspicious to me. Is NULL a valid value for area here? Shouldn't this be a g_return_if_fail() instead? This may hold true for the original code already; no guarantee that this code is doing everything right.
1. Sometimes the content of the thing transformed is more discernable at some opacity than others. The opacity of the original layer can still be adjusted by the user to 0 if they don't want to see the original un-transformed layer. If the layer is just a selection, then not drawing the original part seems to mean not drawing the entire layer the selection is from. Not very desirable. Then if seeing the old data is ok, that would close bug #315051. It would be nice to get the original's opacity. I'm not sure how to find that, though. It would sure help if the core were adequately documented! Though sometimes a custom opacity regardless of the original opacity might be more convenient for the user. I personally would prefer custom opacity almost always. 2. Will do. But similarly, from the original code, there is an if(mask) within a for loop in gimp_display_shell_draw_tri(). I would think it hardly makes a difference, a couple hundred if statements per preview draw, but I could separate that too, if you like. 3. Most of those checks basically follow the original code. In trying to sort them out, some questions. How bulletproof are these private functions supposed to be? The original code had redundant checks in each successive function usually with g_return_if_fail(), but sometimes just the if (!). Should there be such redundant g_return_if_fail() in each function? My check for area was redundant, it had already been checked in gimp_display_shell_draw_quad() with a g_return_if_fail(). Another: the preview function is called with a GimpDisplayShell. Does such a shell always have a shell->canvas, and a shell->canvas->window? Original code checked only if (! shell->canvas->window), skipping check for shell->canvas. I'll wait for your answers to post an updated patch.
I would like to avoid adding more controls to the transform tool options, it is already way too cluttered. Thus we should use the layer's opacity here and make sure that changes to the layer opacity are immidiately reflected in the transform preview. Of course this means that we would have to fix bug #315051 first. If it turns out that loop unrolling doesn't make a noticeable difference, then don't do it. Sanity checks using g_return_if_fail() and friends should be used in every non-static function and it is OK to use them in static functions as well. That's up to the programmer to decide. But the parameters should only be checked with a simple if statement followed by a silent return if that's a valid value to call the function with and the silent return is part of the expected behaviour. Please note also that a g_return_if_fail() is less expensive (due to the use of compiler hints) and that these checks can be removed at compile-time.
Created attachment 71197 [details] [review] patch to provide preview opacity, take 3 I had a huge deadline I couldn't procrastinate about any more, but that's past, and I can continue foraying into the Gimp... Attached is my same patch, but with the minor adjustments suggested by Sven, and also with the slider running from 0..100 rather than 0..255 (but converted back to 0..255 internally). Over the last month I've been using my patch, and about 70% of the time, my original is 100% opaque, and the custom opacity comes in very handy for me. As for bug #315051, it would be better to automatically not show the original layer when it is not a selection. I am still trying to figure out Saul's patch there. I consider the added transform tool option opacity slider "necessary functionality" rather than clutter. To reduce clutter there, I personally would do away with the whole slider and drop down for "number of grid lines"/"grid line spacing". Otherwise, the preview settings could be grouped into one of those triangle drop down things like for "Rectangle Controls". Anyway, as Gimp tools go, the transform tools don't seem so cluttered. Using the original opacity is fine only for independent layers. A very common case is transforming a selection. To use the original layer's opacity in this case in no way helps in aligning the transformed area, as I frequently want to align the area on a 100% opaque layer. Here, modifying original opacity just causes too much confusion. Another case where only using the original opacity fails is transforming quickmasks or layer masks. The orignal doesn't really have any opacity here, unless I'm missing something. The transform preview then totally obscures the image beneath a black and white curtain.
Setting milestone to 2.4. We should review the patch and check if it can still be accepted for 2.4.
Looks like this patch needs to be updated: " patching file app/display/gimpdisplayshell-preview.c Hunk #4 FAILED at 107. Hunk #5 FAILED at 334. Hunk #6 FAILED at 445. Hunk #7 FAILED at 489. Hunk #8 FAILED at 532. Hunk #9 succeeded at 610 (offset 2 lines). Hunk #10 FAILED at 637. Hunk #11 succeeded at 877 (offset 2 lines). Hunk #12 succeeded at 898 (offset 2 lines). Hunk #13 FAILED at 926. " " patching file app/tools/gimptransformtool.h Hunk #1 FAILED at 60. "
Created attachment 71714 [details] [review] patch to provide preview opacity, take 4 Updated. Numerous 'x []' to 'x[]' changes in the last commit fomented chaos when applying the patch to app/display/gimpdisplayshell-preview.c. Should be ok now. The take 3 patch still works on 2.3.10, by the way.
True. It's also true that patches against recent CVS are much less trouble to commit and therefore more likely to be committed. Trying the patch out: * it all applies correctly now. as far as I can tell. * I can't seem to get opacity lower than 50%, even if the layer opacity is set to 100% and the transform tool opacity is set to 100%. Experimenting with a black square, the square is RGB (0,0,0) and the background is RGB (84,84,84), the blended result at opacity 100%+100% is RGB (50,50,50) -- which amounts to 40.4% actual opacity. * It is an extremely nifty feature, and I appreciate its general usefulness. * You've broken scaling? I can't get it to DO anything. The preview is broken (no display at all), and I cannot get it to actually do the transform. The other tools work fine. Note that there were some recent UI changes to the scale tool -- They should be unrelated, but maybe you need to change something to play nice with them. Maybe someone else broke it (I hadn't checked it lately).
Okay, it looks like the scaling thing was caused by the scale tool being set to scale paths. I'll file a separate bug about the apparent unresponsiveness of transform tools when set to scale something that there are none of.
Created attachment 72143 [details] [review] patch to provide preview opacity, take 5 This updated patch compensates for the recent changes to app/tools/gimptransformoptions.c and gimptransformtool.c. I had also accidentally reverted the opacity slider values back to [0..255], this patch makes it [0..100] again. I don't understand your example above. The original layer opacity and preview opacity are totally independent. Setting preview opacity to 100% for a black rectangle makes just that: a totally opaque black rectangle. The patch does not remove the original layer, which can sometimes cause confusion unfortunately (bug #315051, I'm still puzzling over that one). The original's opacity can still be set to 0 to get rid of it. On my machine, anyway, I can get all opacity from 0 to 100.
Okay. This patch looks good to me, now :)
*** Bug 354356 has been marked as a duplicate of this bug. ***
Just tried the patch 5, and it seems to work as intended. There are two warnings, though: gimpdisplayshell-preview.c: In function `gimp_display_shell_preview_transform': gimpdisplayshell-preview.c:136: warning: ISO C90 forbids mixed declarations and code gimpdisplayshell-preview.c:164: warning: ISO C90 forbids mixed declarations and code And the patch doesn't fix this bug, IMO. I first thought it was broken because the behaviour didn't change at all, then I noticed the opacity slider and checked the comments... it's more or less a workaround.
Let's bump this to 2.6. The patch isn't ready for commit and it is likely to introduce new problems.
i don't get an opacity slider, i just have the grid slider... did i miss something?
Created attachment 84778 [details] [review] patch to provide preview opacity, take 6 Updated my patch to the current svn. Also seem to have fixed the warnings as noted above. True my patch does not show the original layer opacity or other combine modes. My patch is not a workaround for that. It adds a feature to temporarily change opacity to something other than the actual opacity which makes it really easy to align things during transforms. This does address the major complaints of the original reporter and ALL 4 of the bugs that have been marked as duplicates of this one, as well as the recent gimp-dev post from A. Prokoudine about stacking up images for collaging. Posterity will smile upon whoever commits this patch! After using my patch the last 5 months, the one bad thing I have to say about it is that it does not simultaneously solve bug #315051.
Created attachment 84796 [details] [review] patch to provide preview opacity, take 6.1 Arg!! After fixing the warnings, forgot to do another diff. The warnings are really fixed now.
*** Bug 492627 has been marked as a duplicate of this bug. ***
Created attachment 106341 [details] [review] patch to provide preview opacity, take 7 Updated to current svn. Seems to also work on Gimp 2.4.5. I realize Gimp rendering is totally changing, but in the meantime, this feature is still very useful to me.
I've started testing this -- it seems to work cleanly and I basically like it. I'll continue to test it for a while to see if anything breaks. It makes the ugly transform tool options even a little uglier, but that isn't something that should be solved in the context of this bug report.
Oh, it is very well something that should be solved here. If possible we would not add another option. I don't understand why there has to be another user interface element here. The bug report asks about respecting the layer opacity. So let's just do that and use the layer's opacity. There is no need for adding more clutter to the tool options.
The problem with using the layer opacity is that you can't adjust it without halting the tool. In testing, even the small amount I have done, I found that it is pretty valuable to be able to adjust the opacity while the preview is up. Tom, you have by far the most experience using this -- what do you think? For what it's worth, I'm going to attach a screenshot of the transform tool options after some experiments in "beautifying". The point is not to defend every change, but just to show that there should be room for an opacity option if the rest of the dialog is cleaned up.
Created attachment 106630 [details] screenshot of modified scale tool options This screenshot shows the Scale tool options with several layout changes. The Gtk theme is Clearlooks. The "Grid lines" and "Opacity" options use a new type of control I have been working on, in which the button causes a slider to pop up.
Your screenshot doesn't really work as it doesn't give the combo-boxes enough horizontal space and doesn't take translations into account. Anyway, this is not relevant to this bug report. The obvious solution is to keep the tool from being halted when the layer opacity is changed. Can we get the patch in without the additional user interface element? We can then fix the outstanding issue based on that.
I have committed a heavily modified version of this in two commits. First some unrelated cleanup: 2008-03-05 Sven Neumann <sven@gimp.org> * app/tools/gimptransformtool.[ch]: applied some unrelated cleanups from a patch from Tom Lechner (from bug #167926). Then the main part after some cleanup and further optimization: 2008-03-05 Sven Neumann <sven@gimp.org> * app/display/gimpdisplayshell-preview.c: applied a modified and further optimized version of the patch from Tom Lechner as attached to bug #167926. This makes the transform preview take the layer opacity into account. Needs some more work...
Now in order to make this really useful we need to take care of bug #315051. And we should make up our mind whether we want a translucent preview, configurable from the transform tool options, or if the opacity should be taken from the layer opacity (as implemented now). In the first case a control would have to be added (this can be taken from Tom's patch). In the second case we would have to add code that monitors the layer opacity so that changes are immidiately reflected on the transform preview. This should be discussed on the mailing-list.
I do often have to change the preview opacity depending on the content. The original opacity is hardly ever useful to me as a default, but I don't think it would bother me if the preview starts with the original's opacity. What annoys me about my patch other than not squashing bug #315051 is that I cannot control the preview opacity with keyboard shortcuts! I use shortcuts as a workaround for that bug to adjust current layer opacity so I don't see it along with the preview, BUT it IS sometimes useful to be able to compare the placement of the transformed thing to the original, before actually applying the transform. Usually I want to keep the original opacity the same before and after. Tinkering around with the actual opacity to adjust the preview opacity would add extra steps because of having to restore the original opacity (as well as having to remember what it was in the first place).
I have thrown out almost all of your code in favor of a much simpler solution which is also faster as it removes a round-trip to the X server and pushes the blending to the graphics layer where it can be done in hardware (on systems supporting the Composite extension): 2008-03-06 Sven Neumann <sven@gimp.org> * app/display/gimpdisplayshell-preview.c: don't do the blending here, just apply the opacity on the pixbuf's alpha channel. This will also make it easier to port this code to Cairo, which is going to happen soon. I am all for adding the preview opacity as a tool-option since after all this is just a preview and doesn't even attempt to give a correct result. But I asked you to discuss this on the mailing-list, didn't I? So can we please do that, so that others have a chance to participate. Thanks.
Closing this bug as WONTFIX because we are not going to address the problems that the bug reporter outlined. At least not until the port to GEGL is complete. I have opened a new bug report for the transform tool preview opacity: bug #520690.
Tom, We just had a sort of angry discussion of this on irc, because I was pretty unhappy about the discouraging tone of the last two comments. I tried to persuade Sven that he ought to express that he is happy that you have worked on this. He feels that reviewing the code is an expression of happiness in itself, and that to say anything positive would just be "PC". I don't myself feel that way, and I would just like to say that I like the work you have done on this, and I'm particularly happy that you have been persistent about getting a valuable feature into Gimp.
I guess Tom will appreciate more that his patch has been reviewed and committed to SVN. And he will probably also like this follow-up commit which is based on his request for being able to control the preview opacity with the keyboard: 2008-03-06 Sven Neumann <sven@gimp.org> * app/actions/tools-actions.c * app/actions/tools-commands.[ch] * app/tools/gimptransformtool.c: allow to control the preview opacity by actions (defaults to < to > keys).
As a political cartoonist, I am battle hardened against those who seem to lack tact and diplomacy, though it is startling early in the morning. Anyway, seems to work great now, especially with the shortcuts! Thanks (you will notice that I said thanks, and I'm not ashamed to say it). Now to begin lobbying for: http://gimp-brainstorm.blogspot.com/2007/09/squarecircle-anchors-for-transform.html which is surely a topic for the mailing list.