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 snippet in #787 allows to play around with the offset behavior in different transformation scenarios and to compare it with the existing behavior if once started with the existing and once with the code proposed in this PR.

The PR includes the fix for all three SWT implementations and provides a regression test for the existing singularity that fails without the fix. It also adds a snippet for demonstrating the original bug and the effects of this change.

Fixes #788.

[github-actions]

Test Results

     299 files  ±0       299 suites  ±0   6m 14s ⏱️ +8s
  4 096 tests +1    4 088 ✔️ +1    8 💤 ±0  0 ❌ ±0 
12 200 runs  +3  12 127 ✔️ +3  73 💤 ±0  0 ❌ ±0 

Results for commit 239b050. ± Comparison against base commit 425f14e.

♻️ This comment has been updated with latest results.

[HeikoKlare]

@fedejeanne would be great if you can validate this one, particularly on other systems than Windows.

In case this PR get approved for merging, it would be good if that happens early in the development cycle to see if there is some consumer depending on the existing (incorrect) behavior, as it has been there for around 20 years now.

[fedejeanne]

I'll try to take a look at it today/tomorrow :)

[fedejeanne]

In Mac

• macOS Ventura (13.2.1)
• M1 Chip

Summary

• Tested in 2 Displays: integrated display and external (Dell) display
• I did "runs" with the following Rotations: 45°, 90°, 135°, 180°, 225°, 270°, 315°, 360° and 405°
• Before this PR, there are singularities (the arrow doesn't show) at rotations starting at 45° and increases of 90° i.e: 45°, 135°, 225°, 315° and 405°.
• Those singularities only happen with Line Width = 0 --> i.e. starting at Line Width = 0.1 there are no singularities even before applying this PR
• There are no singularities after applying this PR, regardless of the Line Width 🥇
• Use anti aliasing doesn't make any difference
• Applying Horizontal / Vertical Scaling doesn't make any difference (I used a factor of 2.0 for each, both separately and simultaneously i.e. I did 3 runs)
• The check-boxes Draw horizontal / vertical line didn't show up in the snippet (they do in Linux though).

Some screenshots

In "the big display": DELL U2518D (25'', 3840x2160)

Before this PR

After this PR

In "the small integrated Display": Integrated Retina Display (13.3'', 2560x1600)

Before this PR

After this PR

[fedejeanne]

In Linux

• Ubuntu 20.04.2 LTS (Yes, it's time for an upgrade)
• GNOME 3.36.8

Summary

Same as for Mac except for the last item:

• The check-boxes Draw horizontal / vertical line do show up in the snippet ✔️

Some screenshots

In "the big display": DELL U2518D (25'', 3840x2160)

• On the left: Before this PR
• On the right: After this PR

With Line Width == 0

With Line Width > 0

In "the small display": HP (17'', 1920x1080)

• On the left: Before this PR
• On the right: After this PR

With Line Width == 0

With Line Width > 0

[fedejeanne]

The check-boxes Draw horizontal / vertical line didn't show up in the snippet (they do in Linux though).

Scratch that, I just restarted the snippet and they are being shown in Mac too.

All A-OK ✅

[fedejeanne]

Linux and Mac look good

[HeikoKlare]

Thank you for that detailed validation, @fedejeanne! Great to see that the patch also works fine on Linux and Mac.

I also made two validations with downstream consumers:

1. (Automated) I ran the GEF classic tests (org.eclipse.draw2d.tests and org.eclipse.gef.tests). Several tests run through the changed code and all tests still succeed locally on a Windows 11 system.
2. (Manual) I applied the changed code to our RCP product, which also uses GEF classic, and neither found issues with the SWT-based UI in general nor within the diagrams rendered with GEF.

I would like to give this a try and merge in the next days, so that there is sufficient time in the current release cycle to detect potential regressions.
@sravanlakkimsetti @iloveeclipse @lshanmug Do you as codeowners have any objections to this change and to merging it?

[HeikoKlare]

I integrated the commit with the snippet to visualize the original issue and the fix originally provided in #787 into this PR.

Since no objections have been made to this PR, I will merge it now.