Fix GC#drawImage with ImageGcDrawer for Cropping and Scaling

[al-noori]

The GC#drawImage method takes
(image, srcX, srcY, srcWidth, srcHeight, destX, destY, destWidth, destHeight)
as arguments and crops and scales from the source region to the destination
region.

Passing an image drawn via ImageGCDrawer led to the following issue:

The image handle from the subcall is resolved using the monitor zoom
(data.nativeZoom) and the calculated scaledImageZoom (gcZoom * scaleFactor).
This handle corresponds to an ImageData initialized at scaledImageZoom,
whereas the drawings of the second GC are performed using the monitor zoom,
subject to the auto-scale property.

This mismatch results in unaligned sizing of drawings. For example, a 200%
monitor zoom combined with a scale factor of 0.5 produces a scaledImageZoom of
100%. As a result, the ImageData is initialized at 100%, while drawing occurs
at 200%.

Furthermore, the calculation of scaledImageZoom uses fallback logic that only
allows 100% and 200% as possible outcomes, which is clearly unintended in this
context.

The fix delegates resolving the correct handle to the Image class by passing
the width/height of the full image scaled by the scaledImageZoom. This
is a space on where scaled src coordinates/width/height lie. A callback
then creates a new handle for the height/width and respects the auto-scale property.

If the returned handle matches the full image scaled to the requested
scaledImageZoom in width and height, the source region
coordinates/width/height are passed directly in pixels at that zoom. Otherwise,
the internal zoom factor is derived from the returned handle’s width relative to
the full image, and the source region coordinates are converted to pixel values
using this internal zoom.

Note: Snippet 10 used to demonstrate that method. Although the sizing issue is fixed, the method is replaced by GC#drawImage(destX, destY, destWidth, destHeight) as the use case here is only scaling and not cropping. Currently, a new snippet is in progress which will be developed to address the cropping + scaling scenario (see #2912 as a first version without drawings via the ImageGcDrawer).

[github-actions]

Test Results

  176 files  ±0    176 suites  ±0   26m 36s ⏱️ -41s
4 672 tests ±0  4 650 ✅ ±0  22 💤 ±0  0 ❌ ±0 
  482 runs  ±0    476 ✅ ±0   6 💤 ±0  0 ❌ ±0 

Results for commit 53c83c8. ± Comparison against base commit 47e2587.

[github-actions]

Test Results (win32)

   34 files  ±0     34 suites  ±0   6m 8s ⏱️ + 1m 44s
4 636 tests ±0  4 563 ✅ ±0  73 💤 ±0  0 ❌ ±0 
  170 runs  ±0    167 ✅ ±0   3 💤 ±0  0 ❌ ±0 

Results for commit 7c7f6a2. ± Comparison against base commit 079e9ae.

♻️ This comment has been updated with latest results.

[al-noori]

@arunjose696 i edited the PR such that the drawImage(srcX,srcY,...) method uses the same callback image.executeOnImageHandleAtBestFittingSize via width and height of the scaled full image as the other drawImage method.
That is why i would hesitate to write an additional overwrite. I thereby intend to capture a quick adoption mentioned in: vi-eclipse/Eclipse-Platform#468 (comment). This also made any changes to the image class redundant and i could leave out my additional methods. What is your opinion on this?

Regarding that hack: it seems to me that i does not make sense. The hack is motivated by potential rounding errors, but without further investigation, i assume that passing correct source coordinates/width/height which lie in the space of the full image, the statement src.intersect(fullImageBoundsScaled) will always return src with equal attributes as before regardless of errors.
I streamlined the getBounds call as you mentioned and would leave the hack as it is for this PR. Investigating the hack and why it was introduced in the first place might be useful in the context of a different PR.

[arunjose696]

@arunjose696 i edited the PR such that the drawImage(srcX,srcY,...) method uses the same callback image.executeOnImageHandleAtBestFittingSize via width and height of the scaled full image as the other drawImage method. That is why i would hesitate to write an additional overwrite. I thereby intend to capture a quick adoption mentioned in: vi-eclipse/Eclipse-Platform#468 (comment). This also made any changes to the image class redundant and i could leave out my additional methods. What is your opinion on this?

Using the same overload makes sense and has simplified the changes IMO.

Regarding that hack: it seems to me that i does not make sense. The hack is motivated by potential rounding errors, but without further investigation, i assume that passing correct source coordinates/width/height which lie in the space of the full image, the statement src.intersect(fullImageBoundsScaled) will always return src with equal attributes as before regardless of errors. I streamlined the getBounds call as you mentioned and would leave the hack as it is for this PR. Investigating the hack and why it was introduced in the first place might be useful in the context of a different PR.

Thanks for streamlining it , i have a some concerns in the changes though, and few more comments

[ShahzaibIbrahim]

@arunjose696 have you reviewed this already?

[arunjose696]

Testing the changes everything looks good to me I tested with cropping pngs and svgs with snippet, this also resolves the issue mentioned.

[akoch-yatta]

LGTM, I did a small adaption by moving up and re-using the src Rectangle to create one less Rectangle. I tested mostly with Snippet389 and the IDE and everything worked as expected.
Although there are now more calculations involved, I did not see an easier way to achieve the same result to use the best quality image handle available in a cropping+scaling scenario

[HeikoKlare]

I have also tested with Snippet389 (with both raster images at 100% and 200% as well as with SVGs) and found everything to work fine. Still I think the current error calculation is not correct and with that the overall calculation of the srcPixels.
Apart from that, I only have comments regarding code style.

[HeikoKlare]

Why do we change this calculation? Previously, src was used instead of srcPixels and compared against b, which seems to have been the bounds in pixels. That does not sound correct. Is that the reason for changing it? But since the srcPixels might be different later (depending on what handle we get), this new calculation is not correct either, is it?

[akoch-yatta]

src was in pixels before, the naming has changed here to differ between both of them.
But this correction should be placed somewhere later, when the actual image handle size is known.

[akoch-yatta]

@HeikoKlare I updated the PR and addressed your comments as discussed. I tested it again mostly with the Snippet on different zooms and it still works as expected. As I originally approved this PR, I would ask you to approve it as well, if the changes are fine for you.

[HeikoKlare]

The implementation works perfectly fine and I also think the code is in good shape with coherent responsibilities. I only have two very nitpicky comments left.