GNOME Bugzilla – Bug 591911
diacairo-interactive.c has problems clipping when rendering with alpha
Last modified: 2009-12-05 19:07:43 UTC
Additions of transparency as added by bug #591525 shows up problems with the interactive renderer for cairo. There are bits of objects left behind on other objects due to improper clipping of the gdk surface. The patches to fix this are included in this bug report.
Upgraded gnome bugzilla won't allow me to attach patches. Updates can be pulled from http://github.com/oblivian/dia commits 52f232b69123388a57b091c7dd56b7ae36f8afb3 and 5617b4fe1b94ffdbb995f837440632edab6fff69
Created attachment 141088 [details] [review] first patch in series cleaning up cairo renderer
Created attachment 141089 [details] [review] second patch, fixes seg fault and add DIAG_NOTE's for troubleshooting
Created attachment 141090 [details] [review] last patch, fixes redraw on window restore from minimize
Created attachment 141328 [details] [review] Correct clipping for cairo-interactive-renderer You patch series seems to unnecessarily introduce another copy of the image data (cairo_image_surface_create), also I don't follow your definition of "cleanup" ;) But it also did include the critical bits (gdk_cairo_region/cairo_clip), thanks. BTW: for patches to be easily applied with "git am" it would be nice if you could follow the pattern given in http://mail.gnome.org/archives/dia-list/2009-April/msg00079.html for your commit messages. BTW2: am I right tha the crash mentioned with the seconf patch was introduced with the first one?
My definition of cleanup is to remove the GDK paint components along with associated #if/#else sections. Maybe not the right wording... The point of having the image surface, is I was thinking about separating the grid drawing from the objects so that I could do independent object rotations (another whole set of changes I'm working on) so it kind of was a remnant/side effect of that thought process... BTW Response: Not a problem, I'll make sure I follow that in the future. BTW2 Response: Yep, I submitted the first patch and then found the crash. I could have just resubmitted a whole new patch, but I didn't think about it when generating and submitting the second patch. In the future, I'll just submit a replacement patch.
It is not the wording which did hold me from commiting the patches, it was the unnecessary copy during rendering, the removal of the comments without explaination and the impression I got from reading the series, that the third patch changes a lot of what the first patch introduced. Playing a bit more with parts I noticed, that the combination of fill_pixel_rect() with cairo and actually using a background color with transparency produces the effect shown in bug #402113. Filling must be solid to avoid it. The method fill_pixel_rect() is used to 'clear' the background to a defined state, reminds me on GIMP where the backround layer must not have alpha as well.
Would it make sense to do what I suggested? Allow for independent grid/page rendering. That way clearing and page rendering is not dependant on object rendering.
Apparently I still dont understand what you suggested. Even now clearing and page rendering is not dependentant on object rendering. If at all, it is the other way around;) If you look at ddisplay_render_pixmap() you'll see the whole sequence of drawing. The implementation split between pixel rendering (gdk usage in the interactive cairo renderer) and object rendering is indeed arbitrary. The pixel and clip functions just happened to be in pixel coordinates rather than in object coordinates. And it was easier to keep it this way instead of trying one-pixel lines with cairo. But from what I understand from your approach you should see the same artifacts as described in bug #402113.
A quick "fix" is to draw a &color_white fill_pixel_rect before the background and grid. What I am suggesting is that we could draw a page (e.g. 8x11 with gray borders) or normal like visio and that would be a separate function like drawing the grid. That function would have the effect of always clearing the screen before the background color is applied. I looked at the code and realized it is the other way around from what I thought it was :)
Your "fix" sounds like a workaround for something which IMO should not be broken in the first place. Neither the current alpha patch nor some object rotation support should require to do the extra drawing pass. Let me try to rephrase my comment: please tell me what you want to achieve from the user point of view rather than how ;)
From the users point of view, they should have a blank canvas to draw on. If they want to apply a transparent color all over the canvas with or without alpha that is the background color. The canvas should be able to take different shapes (like a piece of paper the size of the current page setting, or the current unending style).
Now I see. But this would be a completely unrelated feature deserving a bug report on it's own. So for the moment my patch should be enough to resolve this report.
My patch from above got commited to master some weeks ago, see: http://git.gnome.org/cgit/dia/commit/?id=cf021c3b0c722210f2096ac14433192959ff2aa7