bugGNUstep - Bugs: bug #27782, ...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #27782: drawInRect:fromRect:operation:fraction: doesn't work correctly in a flipped view

Submitted by:  Quentin Mathé <qmathe>
Submitted on:  Thu 22 Oct 2009 06:17:35 PM UTC  
 
Category: Gui/AppKitSeverity: 3 - Normal
Item Group: BugStatus: Ready For Test
Privacy: PublicAssigned to: Quentin Mathé <qmathe>
Open/Closed: Open

(Jump to the original submission Jump to the original submission)

Tue 01 Jun 2010 12:01:16 PM UTC, comment #19:

Committed with r30523

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Sun 16 May 2010 01:34:01 PM UTC, comment #18:

ok, great :-)
I'll check this patch still works and commit a cleaned a version which can be used a starting point.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Tue 11 May 2010 05:23:02 PM UTC, comment #17:

Now that the release is out, feel free to commit an updated version of this patch. We will all start to test it and point out any issues :-)

Fred Kiefer <FredKiefer>
Project Member
Mon 19 Apr 2010 04:33:54 PM UTC, comment #16:

NSCopyBits on Mac OS X has a behavior similar to -compositeToPoint, in flipped coordinates the destination corresponds to the bottom left of the image and not the top left unlike -drawInRect:. -compositePoint: ignores the scaling so the drawing doesn't appear upside-down like -drawInRect:, however the translation due the flipping is not cancelled so you must cancel it yourself. That's why I had to similarly cancel it in -scrollRect:by:, since the patch changes NSCopyBits semantic in Cairo to match Mac OS X.
Unlike what I expected at the time I wrote the code, this doesn't break the other backends.

In flipped coordinates, the area where NSCopyBits draws is not the same on Mac OS X than with the current backends (art, xlib, cairo). Cairo once patched works correctly I hope now.
However based on the tests I did today, NSCopyBits source rect is still not correctly interpreted, it seems there is no bounding rect computation involved. To get the source rect in the window surface, on Mac OS X NSCopyBits seems to do:
NSPoint srcOriginInBase = [sourceView convertPoint: [sourceView origin] toView: nil];
NSRect srcRectInBase = NSMakeRect(srcOriginInBase.x + srcRect.origin.x, srcOriginInBase.y + srcRect.origin.y, srcRect.width, srcRect.height);
That seems to fit well with the PScomposite doc I read. I haven't yet thought about what would need to be changed in -compositeGState to get his behavior.

For NSImageCell, I check the control view is nil just to make the code more explicit, but I'm fine with removing it.
I don't see any other way to draw the image correctly (for this case, the Apple doc suggests to use a transform). The way it was implemented with a negative height to flip the image orientation doesn't work on Mac OS X and is not supported anymore with the patch.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Sun 18 Apr 2010 10:09:18 PM UTC, comment #15:

I like the idea of removing all those isFlipped checks in gui. I always suspected that they were only compensating for one-another. But then you had to add one yourself for the NSView -scrollRect:by: method. The rest of the patch on this method I don't understand. When we do our computation on the lower methods correctly switching to another view here (in this case to the base view) should have no effect at all. If it does, we are doing something else wrong. And according tot he comment your patch wont work for other backends. Is this still true?

NSImageCell:
When the control view is nil, is_flipped will always be false. We don't need to check for both conditions.
I don't think we should mess with transformation here. As far as I understand it, you are doing this to guarantee that images are always oriented upwards, which is the way they handled by NSCell.

Fred Kiefer <FredKiefer>
Project Member
Sat 17 Apr 2010 04:18:34 PM UTC, comment #14:

I figured out why NSCopyBits was drawing at the wrong location when the destination is rotated. An updated patch is attached.

In -compositeGState, the pattern matrix was set up like that:
cairo_matrix_translate(&source_matrix, minx - x - dx, miny - y - dy - ssize.height);

when it should have been:
cairo_matrix_translate(&source_matrix, minx - x + dx, miny - y + dy - ssize.height);

(file #20255)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Sat 17 Apr 2010 03:30:44 PM UTC, comment #13:

Oops, I forgot to upload the cocoa screenshot.

Also when I write: "I just found that NSCopyBits interprets the source rect relative to the view gstate", I mean on Mac OS X.

(file #20254)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Sat 17 Apr 2010 03:27:15 PM UTC, comment #12:

Here are the promised example and screenshots.

I just found that NSCopyBits interprets the source rect relative to the view gstate unlike what I said previously. Not sure why I missed that. I didn't write any example with NSCopyBits used inside -drawInRect: (as the attached example does); the previous test code I wrote locked the focus explicitly.

I also found that NSCopyBits doesn't work correctly when the destination is rotated with my patch. The shift I observed is similar to the one previously visible for -compositeToPoint: in a rotated destination (see TestDrawing-without-patch-cairo.png). It could be a special with copyOnSelf I don't handle well.

(file #20251, file #20252, file #20253)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Thu 15 Apr 2010 02:58:16 PM UTC, comment #11:

Here is a new Gui + Cairo backend patch that corrects all the drawing issues I have found until now. I'll upload some screenshots and examples later on. This patch is currently untested with backends other than Cairo.

All flipping checks have been removed in the Cairo backend (some checks have been removed in Gui too when possible). Image drawing methods should now behave consistently even if:
- multiple flip/rotation/scaling transforms are combined
- flip transforms are applied manually instead of overriding -isFlipped.
To guiding idea is that flipping is a Gui notion that doesn't need to exist at the backend level. When the backend never checks -isFlipped or similar, it becomes much easier to support the two previous points.

imo these checks must be removed all at once to get the drawing code correct. They seem to work around each other in an unclear way. Since I had troubles to figure out where the bugs really were, I started by removing all these checks to be sure I was not going to introduce new workarounds.

-- About Composite methods --

This new patch keeps -compositeToPoint implementation exactly as is, but corrects -compositeGState in the backend to match the Mac OS X behavior of -compositeToPoint and NSCopyBits. When the destination is rotated, the content is now composited at the right location.
In addition, it retains the old behavior that takes in account the source ctm. I'm not exactly sure we should do that as discussed in my mail PScomposite questions. The only visible difference with Mac OS X is that we interpret the source rect relative the view coordinate space (rather than the base coordinate space as Mac OS X does) when we do:
NSCopyBits([view gState], sourceRect, destPoint)

My previous patch was an attempt to cancel the effect of the destination ctm on the composited content size. In other words, the destination point as it was computed was dependent on the content size. After observing my previous patch didn't work well with a rotated destination ctm and writing tests on Mac OS X, I found that the destination ctm is only applied to the the destination point but not to the content as whole.

// If r.origin is the destination point, it varies with aRect.size in the code below
NSRect r = aRect;
aRect.origin = aPoint;
[ctm boundingRect: aRect result: &r];

// Now if we rewrite that as below, we get a valid destination point
// In the patch we don't use -boundingRect:result: but [ctm transformPoint: aPoint]
NSRect r = aRect;
aRect.origin = aPoint;
aRect.size = NSZeroSize;
[ctm boundingRect: aRect result: &r];

This is the main fix to get -compositeToPoint works correctly.
I also introduced dx and dy to ensure the content from a rotated source is composited at the right location in the destination.
In the previous implementation, the distance between the composited rect bottom left and the destination point was equal to the distance between the source rect bottom left and its minimum bounding rect bottom left. With the new destination point computation, the distance can vary. Hence this delta adjustment is now required to support compositing with the same source and target (the only compositing case really used that involves a rotated and/or scaled source).
In addition, I think the delta also ensures a valid destination point when the source and the target are not the same unlike previously.

-- About Draw methods ---

For -drawInRect:, we have now two code paths. The old one that handles the scaling in Gui but has several issues:
- draw at the wrong location in flipped coordinates
- draw at the wrong location when there is a scaling with inRect and fromRect
- is slow when you want to draw a small image portion with a scaling or rotation involved.

And a new Cairo-specific code path much simpler that corrects all the previous issues.

Ultimately it is probably possible to reimplement -compositeGState on top of -drawGState, but since the semantic differences are pretty important I don't think it really matters that much. It was also easier to debug and to figure out where the bugs really were once I introduced a distinct code path for -drawInRect.
It's probably far from ideal, but when you consider the number of issues (or changes required in each backend) and the fact we'll probably switch to Opal in the long run, it sounds acceptable to me.

(file #20227)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Tue 03 Nov 2009 04:37:51 PM UTC, comment #10:

Looks like the upload went fine this time. :-) I just compiled the test app and tested it with the Cairo backend.

'draw rep' case doesn't behave as 'draw image' case in flipped coordinates, so I suppose there is something wrong here. My patch probably a small adjustment. The rest seems to be fine. NSCopyBits results are weird though. I'll check what is drawn on Mac OS X.

This test app should be committed to the GNUstep repository I think.

We also need to check cases such as rotated and scaled in flipped coordinates. Neither your test app or mine cover these yet.

The transformation manipulation doesn't exactly boil down to another flip because the translation of the flip isn't eliminated (only the scaling is).
It just does what Cocoa Drawing explains about composite/dissolve methods:
"The most important behavior is that the compositing methods undo any scale or rotation factors (but not translation factors) applied to the CTM prior to drawing. If you are drawing in a flipped view or manually applied scaling or rotation values to the current context, these methods will ignore those transformations."

In the patch, this is done with:
newTs.m11 = 1;
newTs.m12 = 0;
newTs.m21 = 0;
newTs.m22 = 1;

newTs.tX and newTs.tY are let as is because they store the translation.

Before that I do another adjustment to compensate the specific case where the graphics context is flipped. Typically when m11 sign was -1.5 and is now 1 in newTs.
[transform translateXBy: aPoint.x yBy: aPoint.y];
This is necessary because otherwise the drawing location will be interpreted in the new coordinates orientation (not flipped) and not in the initial one (flipped). Figure 3-9 in Cocoa Drawing shows what is expected. If you comment out this -translateXby:yBy: line you can more easily observe why it is needed.

An alternative to this -translateXBy:yBy: is to detect when canceling scaling/rotation involves a sign change in newTs on the y scaling (m22) by handling it as below:

float y = aPoint.y;

newTs.m11 = 1;
newTs.m12 = 0;
newTs.m21 = 0;
newTs.m22 = 1;

// NOTE: Here we could do check [ctxt isFlipped] but checking the transform struct directly
// makes possible to correctly handle the case where the flip transform was applied manually
// and where [ctxt isFlipped] will return NO.
if (didWeChangeYScalingSign(newTs.m22, oldTs.m22))
{
y = -aPoint.y;
}

// Move the drawing rectangle
//[snip]

[ctxt GScomposite: [[cache window] gState] toPoint: NSMakePoint(aPoint.x, y) fromRect: rect operation: op fraction: delta];

This code seems a bit more fragile (and less readable) to me so I used -translateXBy:yBy: in the patch I submitted. I also think it isn't really needed to have a toPoint: parameter at the backend level and it could be cleaner to always make as many CTM adjustments as possible on the AppKit side rather than splitting them between Gui and the backend.

--

There is one thing more or less related I don't understand though, the value of m22 and m11 stay consistent within an application run, but vary between each run. They oscillate between -2 and -1 when the graphics context is flipped but without any visible effect. Is this due to some user scale factor adjustment and why is the scaling not wrong? Or am I missing something related to matrix maths…

You can add NSLog(@"%0.2f %0.2f", newTs.m11, newTs.m22); before newTs is manipulated to see what I mean. Just run an app like Gorm two or three times, the logging will be different with each run.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Mon 02 Nov 2009 10:37:31 PM UTC, comment #9:

Looks like the file was to big. I repackaged it and hopefully it is now small enough.

(file #18980)

Fred Kiefer <FredKiefer>
Project Member
Mon 02 Nov 2009 07:38:29 PM UTC, comment #8:

One more try in uploading the old test code.

The cairo drawing bug you noticed seems to be unrelated to you change. I noticed it today myself, when working with Gorm.

What you report back on your test results looks great.

There is one thing I still don't understand. What is the point in all the transformation manipulation. Doesn't this just simple boil down to another flip? Sorry if I am totally off here, I need to look closer at the code.

Fred Kiefer <FredKiefer>
Project Member
Mon 02 Nov 2009 04:35:04 PM UTC, comment #7:

The test application you mention isn't listed in the attachment section. Can you try to upload it again?

For xlib, art and cairo, scroll views work well for me with the patch. I did more tests in various applications and the scrolling seems to be fine. So I suppose copying gstate isn't broken by the patch.
The composite-test in GSTest looks ok too.

I spent some time testing the patch more thoroughly.
Everything works great in art.
For xlib, drag images appears as plain gray images rather than what you expect (e.g. a button image when you drag a button from a Gorm palette).
For cairo, there are two bugs currently. Image drawing is shifted downwards in drag images and a similar issue also appear in outline/table view image cells. I haven't tried to fix them yet.

Could you test the windows backend or should we ask somebody else?

Aside of that I also added an assertion (in the main composite method) which seems too strict: Gorm reports it in the console when you run it with the patch applied to Gui.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Sun 01 Nov 2009 10:50:38 PM UTC, comment #6:

Find attached the old test application I was talking about.
I also committed the part of your patch that just changed over to the newer shared composite and dissolve code. This makes the remaining of your patch a lot smaller.

Fred Kiefer <FredKiefer>
Project Member
Thu 29 Oct 2009 10:15:32 PM UTC, comment #5:

I wont have time to look at the patch until next week, but removing the doublication in the different drawing methods is surely agood thing. I had the code in place for a long time, but never dared to switch to it.

Could you please check that copying from one gstate to another (or the same) still works as expected? One simple way to test this is to use a scroll view.
There also is an old test that checks quite a few different drawing methods at once.
I need to dig that up.

Before the patch gets submitted we need to make sure that the other backends at least don't work less then before. This may require some additional work.

Fred Kiefer <FredKiefer>
Project Member
Thu 29 Oct 2009 09:46:57 PM UTC, comment #4:

Here is the promised patch that corrects the issues discussed previously. NSImage drawing now behaves exactly as Mac OS X with the Cairo backend.

The patch ensures a correct drawing in flipped coordinates by removing any flipping adjustment in the backend. This makes drawInRect (and drawAtPoint) works correctly with the Cairo backend.

However this first change was breaking compositeToPoint and dissolveToPoint methods so I rewrote them to be simpler and match the Cocoa documentation. To make this rewrite easier and reduce code duplication, compositeToPoint and dissolveToPoint methods have been modified to share a single implementation rather than having several almost identical implementations.

The shared implementation behind these methods is in -compositeToPoint:fromRect:operation:fraction:. This new implementation first translates the CTM to the image drawing origin, then cancels both scaling and rotation. This way we don't need to compute the drawing origin once the scaling and rotation are cancelled because we have translated the CTM before, and additionally the backend now doesn't have to compensate the flipping anymore (this was breaking/contradicting drawInRect). The first argument of compositeToPoint is a point in the existing coordinate space which isn't not the same than the custom coordinate space used by compositeToPoint where scaling and rotation are cancelled. If we don't translate the CTM, we have to compute where the point is located in the custom coordinate space (which is not so simple when the scaling is not an integer value or a rotation is in use).

Although I haven't tested it, the previous implementation was probably not handling rotation and scaling correctly in flipped coordinates. The new one should work properly.

Some additional comments...
drawInRect (and drawAtPoint) still draws incorrectly in flipped coordinates with xlib and art. But this was the case previously too. So I would say it doesn't matter that much.
I don't know much about xlib and art. I suppose the problem could be that they require changes similar to cairo... or may be xlib and art have no support to draw the image upside-down (unlike cairo which handles that transparently when you set a negative scale on the CTM).
I have no Windows machine available, so I haven't tested the windows backend and whether it needs adjustments or not.

compositeToPoint and dissolveToPoint methods now all work correctly with the three backends.

I also updated the test example to test compositeToPoint and dissolveToPoint methods and uploaded some new screenshots.

I have tested the patch with the xlib, art and cairo backends in both Gorm and the attached Test app. Everything seems to work exactly as before. Scrolling works correctly too.

(file #18958, file #18959, file #18960, file #18961)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Thu 29 Oct 2009 11:29:17 AM UTC, comment #3:

I think I have figured out a correct solution inspired by the one I suggest in my previous reply. I'll post a patch later on.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Fri 23 Oct 2009 09:17:14 PM UTC, comment #2:

Hi Fred,

From what I recall my patch had some rough edges. e.g. I overlooked the fact you need to call _rebuildCoordinates each time -isFlipped returns a different value.

When I tried to figure out a fix for the drawInRect bug, I found that drawInRect behaves correctly if you comment out all the flipping related code in the cairo backend. However it breaks compositeToPoint. I have attached the patch so you can take a look.
I rewrote compositeToPoint to alter the CTM directly. To do so, I disabled the first part of the code where the cache is created and only modified the 'else' branch where drawRepresentation:inRect: is invoked but it doesn't work (the image location is shifted upwards outside of the view). Here is what I tried to do:

NSAffineTransform *transform = [GSCurrentContext() GSCurrentCTM];
NSAffineTransformStruct oldTs = [transform transformStruct];
NSAffineTransformStruct newTs = [transform transformStruct];

/* Put identity matrix 2x2 in the CTM to suppress scaling and rotation but keep translation as is */
newTs.m11 = 1;
newTs.m12 = 0;
newTs.m21 = 0;
newTs.m22 = 1;

[transform setTransformStruct: newTs];
[GSCurrentContext() GSSetCTM: transform];

rect = NSMakeRect(aPoint.x, aPoint.y, _size.width, _size.height);
[self drawRepresentation: rep inRect: rect];

[transform setTransformStruct: oldTs];
[GSCurrentContext() GSSetCTM: transform];

The Cocoa documentation leads me to think that compositeToPoint and similar methods should probably do something like that.

It seems there is also some copied/pasted code in NSImage.m that could be eliminated. e.g. drawAtPoint should just call drawInRect. Ditto for compositeToPoint methods.

(file #18936)

Quentin Mathé <qmathe>
Project MemberIn charge of this item.
Fri 23 Oct 2009 08:06:56 AM UTC, comment #1:

Hi Quentin,

first to your test code. It shows agaon your old point that we handle the flipped attribute differently from Cocoa. It should be enough to define a method isFlipped that return YES on both platforms. But currently GNUstep caches that value and has no chance to reset it later on. I think I should go ahead and push your old patch for this.

I know about these drawing problems, but your test code shows they are even worse then I thought. With my last tests I had the impression that on cairo we do things correctly now.
As for the backend knowing about the flipped state, I really have no clue here. The code to set this is older then my contribution to GNUstep and currently we only use that information in cairo and winlib. The problem here is that the coordinates to draw the image get flipped so often we really need to sort that out in the gui first, before we can implement it correctly in back. Any help here is highly appreciated.

Fred Kiefer <FredKiefer>
Project Member
Thu 22 Oct 2009 06:17:35 PM UTC, original submission:

GNUstep gui and back 28866 on Ubuntu 9.04.

As it is visible in the attached screenshots, -[NSImage drawInRect:fromRect:operation:fraction:] does not match Mac OS X behavior when the view coordinates are flipped and is also inconsistent accross backends.

-drawAtPoint:fromRect:operation:fraction: has probably the same issue although I didn't test it.

With Cocoa Drawing Guide -> Image -> Drawing Versus Compositing explanations, I tried to figure out where the bug is but I failed to. I don't really understand why the backend has to know about whether a view is flipped or not. It seems to me it is not necessary and even undesirable to have the backend knows about the view notion (e.g. for a graphics editor with its own view-like hierarchy and which would support various coordinate orientation).
The other thing I don't understand is where the scaling and rotation are suppressed on the current CTM when -compositeToPoint:XXX methods are used. It seems to me this should happen directly in these methods by altering the current CTM and restoring it before the method returns.

I attached the test app which was used to grab the screenshots. It can be compiled on both GNUstep and Mac OS X (with Xcode).

Quentin.

Quentin Mathé <qmathe>
Project MemberIn charge of this item.

 

Attached Files
file #20255:  gnustep-drawing-fixes2.patch added by qmathe (27KiB - text/x-patch)
file #20254:  TestDrawing-cocoa.tiff added by qmathe (37KiB - image/tiff)
file #20251:  TestDrawing.tar.gz added by qmathe (27KiB - application/x-gzip)
file #20227:  gnustep-drawing-fixes.patch added by qmathe (27KiB - text/x-patch)
file #18980:  gui_test3.tgz added by FredKiefer (27KiB - application/x-compressed-tar)
file #18960:  TestDrawInRect.tar.gz added by qmathe (24KiB - application/x-gzip)
file #18961:  drawInRect2-cocoa.tiff added by qmathe (41KiB - image/tiff)
file #18936:  disable-flipped-cairo.patch added by qmathe (854B - text/x-patch)
file #18926:  TestDrawInRect.tar.gz added by qmathe (23KiB - application/x-gzip)
file #18925:  drawInRect-screenshots.tar.gz added by qmathe (48KiB - application/x-gzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by FredKiefer (Posted a comment)
  • -unavailable- added by qmathe (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 17 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 01 Jun 2010 12:01:16 PM UTCqmatheStatusIn Progress=>Ready For Test
      Assigned toNone=>qmathe
    Sat 17 Apr 2010 04:18:34 PM UTCqmatheAttached File-=>Added gnustep-drawing-fixes2.patch, #20255
    Sat 17 Apr 2010 03:30:44 PM UTCqmatheAttached File-=>Added TestDrawing-cocoa.tiff, #20254
    Sat 17 Apr 2010 03:27:15 PM UTCqmatheAttached File-=>Added TestDrawing.tar.gz, #20251
      Attached File-=>Added TestDrawing-without-patch-cairo.png, #20252
      Attached File-=>Added TestDrawing-with-patch-cairo.png, #20253
    Thu 15 Apr 2010 02:58:16 PM UTCqmatheAttached File-=>Added gnustep-drawing-fixes.patch, #20227
    Mon 02 Nov 2009 10:37:31 PM UTCFredKieferAttached File-=>Added gui_test3.tgz, #18980
    Thu 29 Oct 2009 09:46:57 PM UTCqmatheAttached File-=>Added flipped-drawing-gui+back.patch, #18958
      StatusNone=>In Progress
      Attached File-=>Added testDrawInRect2-cairo.png, #18959
      Attached File-=>Added TestDrawInRect.tar.gz, #18960
      Attached File-=>Added drawInRect2-cocoa.tiff, #18961
    Fri 23 Oct 2009 09:17:14 PM UTCqmatheAttached File-=>Added disable-flipped-cairo.patch, #18936
    Thu 22 Oct 2009 06:17:35 PM UTCqmatheAttached File-=>Added drawInRect-screenshots.tar.gz, #18925
      Attached File-=>Added TestDrawInRect.tar.gz, #18926

    Back to the top


    Powered by Savane 3.1-cleanup1