GNOME Bugzilla – Bug 783108
Make paint tools draw in all open views
Last modified: 2018-05-24 17:52:35 UTC
Created attachment 352599 [details] [review] Patch file The patch causes the paint tools (brush, pencil, eraser etc.) to draw in all of the open views (the original view plus any opened with View/New View). There doesn't appear to be much of a performance degradation as a result of this patch although any delay will be proportional to the number of views that are open.
I should have said that the patch causes the output of the paint tool to appear in all of the open views as the line is being drawn - rather than having to wait until the mouse button or pen is released/lifted.
That looks very promising. This patch should make it for 2.10. Let me review this. :-)
Review of attachment 352599 [details] [review]: Ok so that's very good and makes total sense. There were a few syntax issues, just so you know for future patches ;-) - leave a space between a function name and the opening parenthese. - don't use tab in code, space only - Use only /* comment */ style, no // comment. Apart from this, that's a good patch. I just made a minor functional change, which is that I guess there is no need to flush the current display then check for count of displays, no? Let's just loop through the display list and flush them all, be it 1 or 10 displays. :-) I will push with my changes. Thanks for this patch, and don't hesitate to keep others coming!
Ok, pushed! Thanks for this patch. :-) commit 2aa246f5e9e27d0fcb624dd66cf9ecc5a4e01f7a Author: Richard McLean <programmer_ceds@yahoo.co.uk> Date: Fri May 26 00:06:58 2017 +0100 Bug 783108 - Make paint tools draw in all open views Committed with minor fixes by the reviewer (Jehan). app/tools/gimppainttool.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
(In reply to Jehan from comment #3) > Review of attachment 352599 [details] [review] [review]: > Apart from this, that's a good patch. I just made a minor functional change, > which is that I guess there is no need to flush the current display then > check for count of displays, no? Let's just loop through the display list > and flush them all, be it 1 or 10 displays. :-) P.S.: I said "functional" change, but that was the wrong word. It does functionally the same. I meant just code logics (or something, I don't know! :P). :-)
I had written it with the flush of the current display first in the hope that it would be marginally faster when only one view was being used - which I suppose is the case for most people most of the time. If you feel that there is little time penalty for always iterating through the displays then I'm happy with your changes. I will try and remember your comments regarding formatting and will try to do better next time - can I come out of the naughty corner now please? ;¬) I still need to address the issue of using one of the paint tools with the shift key pressed - the straight line (path?) doesn't appear on the inactive displays, although the painted line does when the mouse button is pressed (this may be associated with paths that are being constructed not appearing on the additional views, something else that I will look at). I would also still like to look at getting the transform tools to preview in all of the views but at the moment I am struggling with this. As an aside - I was surprised that these changes were necessary - I had assumed that there would be one 'canvas' per image and the various views would simply be view ports on to that canvas. From what I have seen of the source code altering it so that this was the case looks like a major change. Is this situation likely to change with the eventual move to full GEGL operation?
(In reply to programmer_ceds from comment #6) > I had written it with the flush of the current display first in the hope > that it would be marginally faster when only one view was being used - which > I suppose is the case for most people most of the time. If you feel that > there is little time penalty for always iterating through the displays then > I'm happy with your changes. If there is only a single view for the image, that won't change a thing (iterating through a list of 1 is nothing). Now if there are several views and if the flush of a view is expensive somehow, it may be interesting to flush first the main view indeed. But if such a thing happens, GIMP would anyway lag behind at next iterations. That would be the least of our worries, I believe. If I turn out to be wrong and you discover later that it can actually be helpful to flush the main view first, feel free to open another bug report with a new patch. :-) > I will try and remember your comments regarding formatting and will try to > do better next time - can I come out of the naughty corner now please? ;¬) Hey no prob. :-) > I still need to address the issue of using one of the paint tools with the > shift key pressed - the straight line (path?) doesn't appear on the inactive > displays, although the painted line does when the mouse button is pressed > (this may be associated with paths that are being constructed not appearing > on the additional views, something else that I will look at). Showing drawing real-time on secondary views makes sense, but do we actually want to show structural UI? I could easily imagine using several views for proofing your image (at different zoom, etc.). In this case, you'd probably not want tool-related GUI to be displayed on views other than the one you are working on (because you want to see the drawing without all these GUI elements polluting the view). Unless you want to work in several views at once? For instance editing a path with a globale view and a zoomed-in view, next one to another, for detailed edit at the same time. This I can see the point. > I would also still like to look at getting the transform tools to preview in > all of the views but at the moment I am struggling with this. This would be quite interesting indeed. We have some bug reports about issues when you transform at a very zoomed-in level, I believe. > As an aside - I was surprised that these changes were necessary - I had > assumed that there would be one 'canvas' per image and the various views > would simply be view ports on to that canvas. From what I have seen of the > source code altering it so that this was the case looks like a major change. > Is this situation likely to change with the eventual move to full GEGL > operation? The full GEGL move is already there (as for core features) in master code. Painting though still uses a lot of non-GEGL processing because it is not fast enough yet. Anyway there was no major change planned on this piece of code for 2.10. In any case, things can be improved. That's for sure. Rather than working on every tool (painting, path, transform, etc.) individually, something could indeed be done at a lower level in order to display changes and previews on all views simultaneously. Maybe there is such code already and it should be fixed. I don't know. In any case, don't hesitate to send patches for your various issues. Open new reports when you do so. :-)
Pleas revert this, painting is already painfully slow, and we update the current stroke on the active display only on purpose.
Ok. Reverted. I think it still makes sense to have immediate visualization though I guess we could indeed wait for bug 694917 to be closed. So I create a dependency. This being said, this does not make painting slower if there is only a single view which is the common use case. And we could expect that people who paint with multi-views are doing so because they expect immediate preview on all their views. Then it is an advanced feature which indeed has a processing cost. IMO as long as it does not slow down the base feature, I would find it acceptable. Yet you are the boss. For now I would keep it open. commit f24edcded71fc56969f0e2b61e4a82b8900e3232 Author: Jehan <jehan@girinstud.io> Date: Fri May 26 17:48:34 2017 +0200 Revert "Bug 783108 - Make paint tools draw in all open views" This reverts commit 2aa246f5e9e27d0fcb624dd66cf9ecc5a4e01f7a. As per Mitch request since painting is already too slow. This may come back when we make painting fast again, I guess. app/tools/gimppainttool.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)
(In reply to Michael Natterer from comment #8) > Pleas revert this, painting is already painfully slow, and we update > the current stroke on the active display only on purpose. If this reasoning is also applied to the transform tools it reduces the usefulness of the Unified Transform tool, since if set to operate on the whole image and the active view is zoomed in the control handles disappear and the Unified Transform tool can only behave as the Move tool. With the patch for the paint tools as submitted there seemed to be almost no delay introduced when only one view was open and it didn't seem to slow things down perceptibly with three views open. Also, as submitted, the "if" statement could have included a test of a 'preview selection' flag set in the preferences that enabled or disabled the feature - the user could then choose to keep the operation as it has been until now or have the interactive updating of the extra views thereby increasing the functionality. For the time being I will therefore stop considering how a similar mod might be applied to the transform tool previews - a feature that in my opinion would be more useful than the mod for the paint tools given that a zoomed in additional view will not be updated until after the transform has been committed; thereby making detailed changes using the Unified Transform tool on a complete image hit and miss. Again the 'preview selection' flag could have enabled/disabled this feature (modifying the transform preview code would definitely impact on the processing time). As noted elsewhere the "New View" documentation could be taken to mean that all changes will be reflected in the views as they occur - albeit in this case we would be making the changes to the whole image whilst looking at a zoomed in version, rather than the example given at the end of the New View documentation.
Putting back to 2.10 milestone. We decided to simply put the very import bugs as blockers. Anything non-blocker have simply very few chances to make it unless someone decides to give it an importance (by working on it and providing working patches).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/1103.