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

Bug 493455 - [win32] fix: source position ignored when drawing transparent image #12

Merged

Conversation

jonathan-meier
Copy link
Contributor

Fixes a regression introduced with commit 22797d3 that was reported in Bug 493455 (comment 51).

@iloveeclipse
Copy link
Member

Could you add a manual regression test / example?

@jonathan-meier
Copy link
Contributor Author

I added a JUnit test even, I hope that's fine too. I could easily convert it to a manual test if that's preferred.

@iloveeclipse
Copy link
Member

The new test fails... The another one from browser can be ignored.

java.lang.AssertionError: expected:<RGB {0, 255, 0}> but was:<RGB {111, 143, 0}>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_GC.test_bug493455_drawImageAlpha_srcPos(Test_org_eclipse_swt_graphics_GC.java:681)

@jonathan-meier
Copy link
Contributor Author

Hmm, I see. I can't reproduce the assertion, neither on my Windows nor on my Linux system, the test passes on both for me. Any hints as to what could be different on your Linux test agent? Otherwise I will just provide a manual test as you suggested.

@SyntevoAlex
Copy link
Member

Test fails on Ubuntu 21.04 for me.

I used SwtTestUtil.debugDisplayImage() to see the images and here are srcImage and image:
Source
Image

Note how image has slightly different colors on left and top edges.

I noticed that you copy 100x100 into 200x200, that involves interpolation.
I'm not sure if this is a problem or "working as intended", but anyways, interpolation results in wrong color.

If I alter the test to copy 100x100 to 100x100, test passes.

…arent image

This is a regression that was introduced with commit 22797d3.
@jonathan-meier
Copy link
Contributor Author

Ah yes of course, when stretching the green part, the red part may bleed into the green part at the border due to interpolation. Stretching is actually not necessary so I adjusted the test accordingly and now it passes. Thanks a lot for looking into this @SyntevoAlex!

@SyntevoAlex
Copy link
Member

Thanks for fixing it quickly! It's always a pleasure when my effort is not in vain :)

@SyntevoAlex
Copy link
Member

The patch makes sense to me and the test now passes.
Review+2 from me.

@iloveeclipse iloveeclipse merged commit 819a24e into eclipse-platform:master Apr 4, 2022
@iloveeclipse
Copy link
Member

@SyntevoAlex : please don't hesitate to merge if you reviewed & approved patches.

@SyntevoAlex
Copy link
Member

OK. I wasn't sure if that's an appropriate thing to do now that you already participated.

for (int i = 0; i < 100; ++i) {
for (int j = 0; j < 150; ++j) {
RGB rgb = testImageData.palette.getRGB(testImageData.getPixel(i, j));
assertEquals(green, rgb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_bug493455_drawImageAlpha_srcPos fails on both MAC platforms

See

expected:<RGB {0, 255, 0}> but was:<RGB {255, 255, 255}>

java.lang.AssertionError: expected:<RGB {0, 255, 0}> but was:<RGB {255, 255, 255}>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_GC.test_bug493455_drawImageAlpha_srcPos(Test_org_eclipse_swt_graphics_GC.java:681)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants