Correct GC offset calculation #788

[HeikoKlare]

The offset calculation in the GC when having an odd line width is erroneous in case rotation transformations have been applied to the GC. With a rotation of 45°, the calculation even suffers from singularities that result in effectively nothing being drawn. With other transformation properties, off-by-one error occur.

This change inverts the offset calculation: Instead of performing a forward transformation of a dummy point whose result is applied to the expected offset, the inverse transformation is applied to the expected offset. This avoids singularities and imprecision in the calculation and ensures that when applying rotations to the GC, the drawing figures are as expected.

The behavior can be evaluated with Snippet 381.

Fixes #788.

Additional Information

This is a revision of the original fix contributed in #789, which has been reverted because of a regression in #1068. The regression is documented in #1067 and could be seen on Mac and Linux when drawing a rectangle with line width 2 into a drawing area of a size in which it should fit (and did before), but was cut off after #789. The reason was that anti-aliasing was (erroneously) considered to be activate. In addition, anti-aliasing behavior was treated equally for all OS', but differs between Windows and Mac/Linux. To ensure compatibility with existing expectations on the behavior, I have removed the additional anti-aliasing handling added in #789 in this PR. Note that the positioning behavior in SWT is not very consistent anyway: for odd line widths, it adds an offset of 0.5 pixels in x and y direction to the points, which is not done when the line width is even. While this may be reasonable for simple scenarios, it becomes complicated when transformations are applied to the GC. And, in particular, in case of a non-integer line width, the line width for offset calculation is checked for being odd or even via integer conversion, whereas the drawing engines properly round that value, which results in unexpected results for non-integer line widths anyway. In case of anti-aliasing the situation becomes even more complicated, since then depending on the offset, the shading due to the anti-aliasing changes. This does, however, only seem to be a problem on Windows, as anti-aliasing behavior is different on Mac/Linux.

In total, this PR is a reduced version of #789 to only make the offset calculation more consistent and, in particular, to avoid the singularities documented in #788. It does not change anti-aliasing offset calculation anymore, so the regression documented in #1068 is avoided.

@Phillipus It would be great if you can test this patch, so that we avoid further regressions. If there are no objections, I would like to merge this PR as soon as possible in the coming development cycle to have sufficient time to notice potential remaining regressions.

[github-actions]

Test Results

   299 files  ±0     299 suites  ±0   6m 37s ⏱️ +33s
 4 101 tests +1   4 093 ✅ +1   8 💤 ±0  0 ❌ ±0 
12 215 runs  +3  12 140 ✅ +3  75 💤 ±0  0 ❌ ±0 

Results for commit 909d448. ± Comparison against base commit 9719a2f.

♻️ This comment has been updated with latest results.

[Phillipus]

On Mac I can't reproduce the regression shown in the snippets in #1067.

However, in our RCP application, with this PR, rectangles are drawn half a pixel shifted right and down on the x and y axes. This is showing in our app because we shift positions of graphical objects using a Transform.translate(0.5, 0.5). We also use line widths of 1, 2, or 3.

I'm not sure I can recreate this in a Snippet because we use Draw2d (which is a wrapper around GC and SWT).

[HeikoKlare]

Thanks for your feedback, @Phillipus!

We also use Draw2d/GEF Classic, which is where the original issue with singularities on transformations occurred. In addition, I found that when zooming in GEF, lines are jumping around because of the currently broken offset calculation. I have not put that into this issue/PR as it can only be seen in downstream tools.

If you can easily provide some Draw2d snippet showing the problem, that would also be great. I just want to be sure that I understand the problem because this sounds to me as if you have worked around the broken offset calculation before:

we shift positions of graphical objects using a Transform.translate(0.5, 0.5).

Or can you can explain the reason for this offset of (0.5,0.5) you apply?

Here is one example from our application using Draw2d/GEF Classic. We paint a grid and rectangles on top of that grid. When zooming, as mentioned before, the lines jump around as the effective line width used for calculating the offset (in the existing implementation) changes and on some zoom levels the offset is applied while on others it is not. The following are screenshots from a 400% zoomed diagram, A red line marks the grid line in which the recangle should actually be centered.

[Phillipus]

In our app we found that when drawing a rectangle with line widths of 1, 2 or 3 the rectangle would clip depending on (1) OS, (2) Zoom and (3) whether display is hi-dpi. So I came up with a compromise here. It's a hack but kind of works.

Before this PR:

After:

If you see the gap between the figure and the selection rectangle you'll see it's different. This is on Mac so I don't know if this is the same on Windows and Linux.

Another example is that we highlight a figure with a blue rectangle, line width 2, when the user is dragging another object into it.

Before this PR:

After:

[Phillipus]

There are certainly problems with Draw2d/SWT and things don't always line up due to offset miscalculations (especially on hi-dpi screens). Over the years we've tried to work around some of these problems and lived with others. If anything is changed in SWT then it creates a new set of problems to work around. It's not easy! ;-)

[HeikoKlare]

If anything is changed in SWT then it creates a new set of problems to work around. It's not easy! ;-)

I agree that it's not easy to fix things that have been broken so long. On the other hand, we cannot stop fixing bugs just because API consumers may rely on the implementation being broken. The offset calculation is currently completely broken when applying rotating transformations to a GC. In certain ranges of rotation angles, lines will start and end at "random" positions. But just to avoid misunderstanding: I don't want to break your (or any other) application, so I am still trying to understand what happens here :-)

I am actually not sure what causes the problems in your example. If you have a line width of 2 and no transformation applied to the GC, then neither the existing nor the modified code will apply any offset. So either the (effective) line width in the example is not 2 or there is some scaling applied to the GC. I will test the change again on Mac to see if I can reproduce your behavior.

[Phillipus]

If you can easily provide some Draw2d snippet showing the problem, that would also be great.

Here it is:

import org.eclipse.draw2d.FigureCanvas;
import org.eclipse.draw2d.RectangleFigure;
import org.eclipse.draw2d.geometry.Rectangle;
import org.eclipse.swt.graphics.Color;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;

public class GCRegressionTestDraw2d {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setLayout(new FillLayout());
        shell.setSize(200, 200);

        FigureCanvas fc = new FigureCanvas(shell);
        RectangleFigure root = new RectangleFigure();
        root.setBackgroundColor(new Color(null, 255, 255, 255));
        fc.getLightweightSystem().getRootFigure().add(root);
        
        RectangleFigure rectFigure = new RectangleFigure();
        rectFigure.setBounds(new Rectangle(10, 10, 100, 100));
        rectFigure.setLineWidth(2);
        root.add(rectFigure);

        shell.open();
        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) {
                display.sleep();
            }
        }
    }
}

On Mac, before this PR:

After this PR:

The right and bottom lines are clipped.

So, this PR would mean that all Draw2d figures are offset by (I think) half a pixel.

[HeikoKlare]

Thanks for the detailed feedback and the reproducing snippet, @Phillipus!

I found that the behavior you describe and demonstrate (at least for the simple rectangle example) was caused by two bugs in my previous commit:

• GC offset data is reused across multiple GC usages and was not reset in case no offset had to be applied in the current usage. This resulted in an offset of (0.5, 0.5) probably always being applied even though actually (0, 0) would be correct. That's what we have seen in the example when using lines of width 2.
• The offset calculated by the matrix transformation was not considered at all because the transformation result was not applied to the offset variable. This was basically a copy-and-paste issue, as the Cairo (Linux) and Gdip (Windows) perform the transformation in-place, while Cocoa return the transformation result. This bug was only noticable when applying scaling and rotating transformations to the GC.

I have corrected both issues and tested with both the rectangle snippet for Draw2d from #1073 (comment) as well as with Snippet 381 that was developed for this PR.

May I ask you to test the corrected implementation again to see if you still face regressions?

[Phillipus]

@HeikoKlare I tested "before" and "after" with our RCP app (Archi) with a simple rectangular Draw2d figure zoomed in. What we're looking for is any gap between the figure and the thin black selection rectangle.

This is on Mac.

Before:

After:

I can see a difference where in the "before" screenshot the selection rectangle is flush on the left/top but there's a slight gap in the "after" screenshot. On the other hand, the "before" has more of a gap on the bottom/right edges while the "after" selection rectangle seems more centered overall with an even gap.

I'm not sure if this matters or not, but I thought I'd point it out. In fact, one could argue that the "after" is better as the gap is consistent.

On Windows it seems to be OK.

[HeikoKlare]

Thank for you the immediate testing and reply, @Phillipus!

The example you show is great, because it demonstrates one thing that, in my opinion, was broken because of the existing way the offset calculation was applied: the width of a line to draw determines whether an offset needs to be applied. If the line width is even, no offset is applied, whereas for uneven line widths the offset of (0.5, 0.5) (scaled according to whatever transformation is applied to the GC) is added. Now the difference between the old and the new state (before/after this PR) is what is considered as the line width. The original implementation takes the line width after applying the transformation, whereas the new implementation takes the line width before applying the transformation (i.e., the original line width). Drawbacks of the old implementation, taking the transformed line width, are:

1. Different scale values in x and y direction together with a line that is rotated produces completely useless results, as it depends on the direction in which a line is drawn how the scaling and the according offset needs to be applied. For example, see where the rotated line starts (first image is before this PR and second one is after).

2. While zooming in and out (especially in GEF diagrams), lines are jumping around. You can best see this when trying out yourself with Snippet 381 and changing the scale value (for example using the other values as set in the examples shown below). This is because the decision about applying an offset is not based on the original line width but based on the scaled line width which obviously changes while zooming in and out. So while zooming it switches between applying an offset and not applying it, making the line kind of "jumping around". Consider the following examples of Snippet 381 with (uneven) line width of 3 (left is before this PR and right is after), but not that it is really hard to see in the images and easier to comprehend when trying around with the snippet.
   • 100% scaling: offset applied in both cases
   • 200% scaling: offset not applied before this PR as line width is considered 6, offset applied after this PR as line width is considered 3. Line is off (to top) in the left image.
   • 300% scaling: offset applied in both cases again
   • It proceeds with higher scale values in the same way: in the state before this PR, the line is "jumping around" while scaling.

The latter behavior is what produces the difference in the gaps you have shown in Archi. You can also see the "jumping" when changing the scale value there with the existing GC implementation:

• 400%: there is a "larger" gap to the right/bottom

• 500%: there is a small gap all around

• 600%: there is a "larger" gap to the right/bottom (like on 400%)

In my opinion, both version have their drawbacks (actually the whole offset calculation idea is kind of weird). But since it is not even possible to calculate a reasonable line width after applying a transformation (as you do not know when initializing the GC in which direction the line is drawn and thus in which direction an offset has to be applied at all), the current implementation of using some scaled line width does not make any sense to me. The new implementation will at least be consistent for every possible rotation and scaling, as you already noted:

I'm not sure if this matters or not, but I thought I'd point it out. In fact, one could argue that the "after" is better as the gap is consistent.

So if this proposal looks good to you, I would proceed with some further testing on all three OSes, and if I do not find any regressions merge this and give it another try for 2024-06.

[Phillipus]

@HeikoKlare Did you really try it in Archi or are your screenshots based on the screenshots I provided?

So if this proposal looks good to you, I would proceed with some further testing on all three OSes, and if I do not find any regressions merge this and give it another try for 2024-06.

I agree!

[HeikoKlare]

Did you really try it in Archi or are your screenshots based on the screenshots I provided?

I have checked out the Archi code and tried myself, as I didn't want to bother you with trying out recent changes over and over again 🙂
So screenshots are originally taken from an Archi product and not based on your screenshots.

[Phillipus]

I have checked out the Archi code and tried myself

Hey, thanks!

[HeikoKlare]

I've now unified all three OS implementations and checked the behavior with Snippet 381 and the one provided for showing the regression of the original attempt to provide this fix in #1067. I've also checked the scenario in Archi used for discussion in this PR on Mac and Linux.

The reproducer provided for the regression in #1067 properly looks like this with this PR:

The version with the regression looked like this:

So if this proposal looks good to you, I would proceed with some further testing on all three OSes, and if I do not find any regressions merge this and give it another try for 2024-06.

I agree!

I will merge this now (as I hope it's not yet too late for M1), so that we have some time to test it in I-/M-Build environments until 2024-06 release.

[Phillipus]

@HeikoKlare Thanks for going the extra mile and trying it out in Archi, I appreciate that. 👍