Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Faulty Scaling in GC #11

Closed
HeikoKlare opened this issue Jul 31, 2023 · 4 comments · Fixed by eclipse-platform/eclipse.platform.swt#789
Closed

Fix Faulty Scaling in GC #11

HeikoKlare opened this issue Jul 31, 2023 · 4 comments · Fixed by eclipse-platform/eclipse.platform.swt#789
Assignees
Labels
Bug A Derivation of Expected Behavior SWT Issue for SWT

Comments

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Jul 31, 2023

Current Behavior

The GC class provides functionality to apply a transformation (translation, scaling, rotation) to the graphics canvas.
This functionality is broken when applying rotations to the GC, leading to

  • completely lost draw operations (e.g. with 45° rotation)
  • off-by-one errors in positions
  • off-by-one errors in lines widths

The reason is in a faulty way in which the offset for odd line widths is calculated: https://github.com/eclipse-platform/eclipse.platform.swt/blob/4daf005a457fb50b929719cb5b3616e55cc2e5c4/bundles/org.eclipse.swt/Eclipse%20SWT/win32/org/eclipse/swt/graphics/GC.java#L326-L344

To reproduce an extreme case, apply a 45° rotation to the GC and try to draw something. Nothing will be drawn because a very large offset is erroneously calculated.

Expected Behavior

Offset calculation in the GC should be performed properly. No draw operations should be lost and no off-by-one errors should occur.

Additional Information

We have used the following patch which only hides the actual problem under specific usage conditions but does not solve it:

--- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
@@ -320,16 +320,19 @@
 			long /*int*/ matrix = Gdip.Matrix_new(1, 0, 0, 1, 0, 0);
 			PointF point = new PointF();
 			point.X = point.Y = 1;
+			double lengthOfInitalVector = Math.sqrt(Math.pow(point.X, 2)+Math.pow(point.Y, 2));
 			Gdip.Graphics_GetTransform(gdipGraphics, matrix);
 			Gdip.Matrix_TransformVectors(matrix, point, 1);
 			Gdip.Matrix_delete(matrix);
-			float scaling = point.X;
+			double lengthOfTransformedVector = Math.sqrt(Math.pow(point.X, 2)+Math.pow(point.Y, 2));
+			float length = (float)(lengthOfTransformedVector/lengthOfInitalVector);
+			float scaling = length;
 			if (scaling < 0) scaling = -scaling;
 			float penWidth = data.lineWidth * scaling;
 			if (penWidth == 0 || ((int)penWidth % 2) == 1) {
 				data.gdipXOffset = 0.5f / scaling;
 			}
-			scaling = point.Y;
+			scaling = length;
 			if (scaling < 0) scaling = -scaling;
 			penWidth = data.lineWidth * scaling;
 			if (penWidth == 0 || ((int)penWidth % 2) == 1) {
@HeikoKlare HeikoKlare added this to HiDPI Jul 31, 2023
@HeikoKlare HeikoKlare converted this from a draft issue Jul 31, 2023
@HeikoKlare HeikoKlare added SWT Issue for SWT Bug A Derivation of Expected Behavior labels Jul 31, 2023
@HeikoKlare HeikoKlare moved this from 🆕 New to 📋 Backlog in HiDPI Jul 31, 2023
@HeikoKlare
Copy link
Contributor Author

I have already investigated the issue and developed a snippet for reproduction here: https://github.com/HeikoKlare/eclipse.platform.swt/tree/snippet-381-gc-transformation

It is extended with a potential fix and a comparison in the snippet here: https://github.com/HeikoKlare/eclipse.platform.swt/tree/gc-transformation-comparison

I will provide these artifacts, an issue and PR with the fix proposal to the Eclipse repositories as soon as possible.

@HeikoKlare HeikoKlare moved this from 📋 Backlog to 🏗 In progress in HiDPI Aug 28, 2023
@HeikoKlare HeikoKlare self-assigned this Aug 28, 2023
@HeikoKlare
Copy link
Contributor Author

Issue is documented here: eclipse-platform/eclipse.platform.swt#788
Snippet for reproduction is provided via this PR: eclipse-platform/eclipse.platform.swt#787
Fix proposal is provided via this PR: eclipse-platform/eclipse.platform.swt#789

@HeikoKlare HeikoKlare moved this from 🏗 In Work: Short to 👀 In Review in HiDPI Dec 12, 2023
@HeikoKlare
Copy link
Contributor Author

Done by merging eclipse-platform/eclipse.platform.swt#789

@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in HiDPI Dec 12, 2023
@HeikoKlare
Copy link
Contributor Author

Original PR reverted because of regression. Improved version has been merged: eclipse-platform/eclipse.platform.swt#1073

@HeikoKlare HeikoKlare removed this from HiDPI Jan 14, 2025
@HeikoKlare HeikoKlare moved this to ✅ Done in General Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A Derivation of Expected Behavior SWT Issue for SWT
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant