[GTK] Consider transformation scale in GC.drawImage(image, x, y) #2919#3201
[GTK] Consider transformation scale in GC.drawImage(image, x, y) #2919#3201ptziegler wants to merge 1 commit intoeclipse-platform:masterfrom
Conversation
…pse-platform#2919 With e97143c, support was added on Windows to take the `Transform` scaling into consideration when calling `drawImage(image,x,y)`. Especially when used in combination with an SVG-based image, this leads to better results as the best-fitting image is used for painting, rather than relying on interpolation. This change follows a similar logic to what has been done for Windows; The call to `drawImage(image,x,y)` is delegated to `drawImage(x,y,w,h)` when a `Transform` has been set. Within this method, the width and height of the image are used as size after being multiplied by the transformation scale. Contributes to eclipse-platform#2919
|
A side-by-side comparison of how this change affects GEF: 
|
There was a problem hiding this comment.
Pull request overview
Updates GTK GC#drawImage(...) so that when a Transform with scaling is applied, SWT requests an image rendered at the effective (scaled) destination size, improving best-fit selection for multi-resolution/SVG images (parity with the earlier Windows change).
Changes:
- Add a helper to compute an effective transformation scale from the current GC transform.
- Delegate
drawImage(image, x, y)to the width/height overload when a non-identity transform is active. - Multiply destination dimensions by the transformation scale before calling
Image#executeOnImageAtSize(...).
| float scaleWidth = (float) Math.hypot(currentTransform[0], currentTransform[2]); | ||
| float scaleHeight = (float) Math.hypot(currentTransform[1], currentTransform[3]); |
There was a problem hiding this comment.
calculateTransformationScale() appears to use the wrong matrix elements for computing the effective X/Y scale. SWT/Cairo transforms are stored as {m11, m12, m21, m22, dx, dy} (see Transform#getElements), where the unit X basis vector maps to (m11, m12) and the unit Y basis vector maps to (m21, m22). Using hypot(currentTransform[0], currentTransform[2]) / hypot(currentTransform[1], currentTransform[3]) computes row lengths, which does vary with rotation when scaling is non-uniform, so the requested image size can be underestimated. Compute scaleWidth as hypot(m11, m12) and scaleHeight as hypot(m21, m22) instead.
| float scaleWidth = (float) Math.hypot(currentTransform[0], currentTransform[2]); | |
| float scaleHeight = (float) Math.hypot(currentTransform[1], currentTransform[3]); | |
| float scaleWidth = (float) Math.hypot(currentTransform[0], currentTransform[1]); | |
| float scaleHeight = (float) Math.hypot(currentTransform[2], currentTransform[3]); |
There was a problem hiding this comment.
I am quite sure we have evaluated how to properly do this calculation when doing the implementation for Windows. The Transform representation does not differ between Windows and Linux, so in case this concern is correct, the Windows implementation would probably have to be changed as well.
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/GC.java
Show resolved
Hide resolved
| public void drawImage(Image image, int x, int y) { | ||
| if (handle == 0) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED); | ||
| if (image == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); | ||
| if (image.isDisposed()) SWT.error(SWT.ERROR_INVALID_ARGUMENT); | ||
| drawImage(image, 0, 0, -1, -1, x, y, -1, -1, true); | ||
| if (currentTransform != null && !isIdentity(currentTransform)) { | ||
| Rectangle imageBounds = image.getBounds(); | ||
| drawImage(image, x, y, imageBounds.width, imageBounds.height); | ||
| } else { | ||
| drawImage(image, 0, 0, -1, -1, x, y, -1, -1, true); | ||
| } |
There was a problem hiding this comment.
This change introduces transform-aware image-size selection on GTK for drawImage(image, x, y) and drawImage(image, destX, destY, destWidth, destHeight), but there doesn’t appear to be a corresponding automated regression test enabled for GTK/Linux. There is already a Windows-only JUnit test for this behavior in Test_org_eclipse_swt_graphics_GC#test_drawImage..._withTransform; consider adding/expanding a test so the GTK implementation is exercised as well (especially for SVG-backed images).
There was a problem hiding this comment.
The change looks sound to me. The comment regarding tests made by Copilot seems valid. There are two test cases in Test...GC, one is even missing an @Test annotation, so we could take the chance of properly enabling it. The other is @EnabledOnOS(OS.WINDOWS), which we now may change to @DisabledOnOS(OS.MAC) until we have applied the same change to macOS.
With e97143c, support was added on Windows to take the
Transformscaling into consideration when callingdrawImage(image,x,y). Especially when used in combination with an SVG-based image, this leads to better results as the best-fitting image is used for painting, rather than relying on interpolation.This change follows a similar logic to what has been done for Windows; The call to
drawImage(image,x,y)is delegated todrawImage(x,y,w,h)when aTransformhas been set. Within this method, the width and height of the image are used as size after being multiplied by the transformation scale.Contributes to
#2919