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

[Sylvain Beucler-4]

URL:
  <http://savannah.gnu.org/bugs/?27782>

                 Summary: drawInRect:fromRect:operation:fraction: doesn't
work correctly in a flipped view
                 Project: GNUstep
            Submitted by: qmathe
            Submitted on: Thu 22 Oct 2009 06:17:35 PM GMT
                Category: Gui/AppKit
                Severity: 3 - Normal
              Item Group: Bug
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

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.



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Thu 22 Oct 2009 06:17:35 PM GMT  Name: TestDrawInRect.tar.gz  Size:
23kB   By: qmathe

<http://savannah.gnu.org/bugs/download.php?file_id=18926>
-------------------------------------------------------
Date: Thu 22 Oct 2009 06:17:35 PM GMT  Name: drawInRect-screenshots.tar.gz
Size: 48kB   By: qmathe

<http://savannah.gnu.org/bugs/download.php?file_id=18925>

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #1, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #2, bug #27782 (project gnustep):

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)
    _______________________________________________________

Additional Item Attachment:

File name: disable-flipped-cairo.patch    Size:0 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #3, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Update of bug #27782 (project gnustep):

                  Status:                    None => In Progress            

    _______________________________________________________

Follow-up 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)
    _______________________________________________________

Additional Item Attachment:

File name: flipped-drawing-gui+back.patch Size:9 KB
File name: testDrawInRect2-cairo.png      Size:14 KB
File name: TestDrawInRect.tar.gz          Size:23 KB
File name: drawInRect2-cocoa.tiff         Size:40 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #5, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #6, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #7, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #8, bug #27782 (project gnustep):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #9, bug #27782 (project gnustep):

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

(file #18980)
    _______________________________________________________

Additional Item Attachment:

File name: gui_test3.tgz                  Size:26 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep

[Sylvain Beucler-4]

Follow-up Comment #10, bug #27782 (project gnustep):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27782>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.gnu.org/



_______________________________________________
Bug-gnustep mailing list
Bug-gnustep@...
http://lists.gnu.org/mailman/listinfo/bug-gnustep