Bug 242280 - GDI+ pen shifting for odd pen widths should be calculated using unscaled pen width
Summary: GDI+ pen shifting for odd pen widths should be calculated using unscaled pen ...
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Silenio Quarti CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-28 14:11 EDT by Randy Hudson CLA
Modified: 2019-09-06 15:30 EDT (History)
3 users (show)

See Also:


Attachments
GC and GCData patch (14.98 KB, patch)
2008-08-27 10:58 EDT, Randy Hudson CLA
no flags Details | Diff
gdiplus test package (4.89 KB, application/octet-stream)
2008-08-27 14:33 EDT, Randy Hudson CLA
no flags Details
Screenshot (50.47 KB, image/png)
2008-08-27 15:13 EDT, Randy Hudson CLA
no flags Details
Silenio's snippet running with the patch (5.84 KB, image/gif)
2008-11-25 16:00 EST, Randy Hudson CLA
no flags Details
screenshot (6.03 KB, image/png)
2008-11-25 16:38 EST, Silenio Quarti CLA
no flags Details
screenshot (12.77 KB, image/png)
2008-11-25 16:50 EST, Felipe Heidrich CLA
no flags Details
Screenshot with lineWidth = 3 (6.43 KB, image/png)
2008-11-25 16:58 EST, Silenio Quarti CLA
no flags Details
GC and GCData patch #2 (15.93 KB, patch)
2008-11-25 20:02 EST, Randy Hudson CLA
no flags Details | Diff
Advanced Graphics Test Package 2.0 (4.94 KB, application/octet-stream)
2008-11-25 20:05 EST, Randy Hudson CLA
no flags Details
screenshoot for drawPath versus drawRectangle (12.21 KB, image/png)
2008-11-26 11:14 EST, Silenio Quarti CLA
no flags Details
fillRectangle versus fillPath with patch (16.88 KB, image/png)
2008-11-26 11:27 EST, Silenio Quarti CLA
no flags Details
setTransform() versus scaling by hand with patch (10.89 KB, image/png)
2008-11-26 11:54 EST, Silenio Quarti CLA
no flags Details
Patch with API to toggle compatibility mode (11.72 KB, patch)
2008-12-11 09:39 EST, Randy Hudson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Randy Hudson CLA 2008-07-28 14:11:03 EDT
When calculating the DRAW_OFFSET in GC, the scaling is first applied to the current penwidth.  This doesn't match what the client saw when drawing unscaled.  For example:

//gc.scaleBy(1.05);
gc.setAntialias(SWT.ON);
gc.setLineWidth(5.99f);
gc.setForeground(red);
gc.drawOval(somewhere);

gc.setLineWidth(5);
gc.setForeground(yellow);
gc.drawOval(somewhere);

The above code draw a yellow oval with a thin red outline around the yellow.  As soon as you scale this such that 5.99 * scale >= 6.0, the rendering is messed up because SWT shifts the first oval by 0.5/scale, but not the second (when scaling by 1.05).

The fix is to just not apply the scale, so:
float penWidth = data.lineWidth;

Further optimization: no need to fetch the current transform unless the penwidth is even.
Comment 1 Randy Hudson CLA 2008-08-26 12:10:00 EDT
Silenio, I have a patch for this bug, as well as a snippet that shows how to draw common shapes so that they render the right way whether with/without antialiasing, and with 100%, 200%, 300% scaling transforms applied (when you would hope for even anti-aliased shapes to "grid-fit" with real pixels).

As you might hope, the "right way" to draw stuff at 100% with default settings requires no changes to code working today, whether it is invoking gdi or gdiplus. I'll try to attach the patch this afternoon. Ping me directly if you want.
Comment 2 Silenio Quarti CLA 2008-08-27 09:46:40 EDT
Please attached the patch. 
Comment 3 Randy Hudson CLA 2008-08-27 10:58:09 EDT
Created attachment 111079 [details]
GC and GCData patch

I still haven't tested TextLayout or gc.drawText(...).

This patch changes GC to use GDI+ PixelOffsetNone all the time, and removes the drawOffset approach that was causing problems. For primitive fills (all fills except for fillPath), it is necessary to pre-translate (meaning prior to any user transforms like scaling) by -1/2 pixel. But then you have to also translate any "complex" brush back by the same amount due to gdip's different treatment of brushes when doing a primitive fill vs. "draw" operations.

There is still a remaining issue with drawString, which seemed to have problems prior to this patch anyway.  Font rendering seems to behave like a fill, but when I tried the fill approach I was seeing clipping at the top of one character.
Comment 4 Randy Hudson CLA 2008-08-27 14:33:50 EDT
Created attachment 111105 [details]
gdiplus test package

Unzip this archive into your "src" folder with SWT on the classpath. Run the class AdvancedGraphics.

I added a test for drawImage(), and found that it too requires the 1/2-pixel shift. I've updated GC locally but wanted to wait for additional feedback before submitting a new patch.

Fonts seem to be working in the tests, so I'm not sure if they will require the 1/2-pixel correction or not.  If only we had a font where each character was a solid rectangle... Does anyone know the /u#### for that character??
Comment 5 Randy Hudson CLA 2008-08-27 15:13:48 EDT
Created attachment 111110 [details]
Screenshot

Here is a screenshot of the example with the previous patch applied.

Legend:
1st "column" is plain GDI painting
2nd is GDI+ at 100%
3rd is GDI+ at 200%
3th is GDI+ at 300%

Note that painting "stuff" at integral zoom levels (especially with anti-aliasing) requires that the client understand how things worked at 100%. When you call:

gc.setLineWidth(1);
gc.drawLine(0, 0, 99, 0);

...which is equivalent to...

gc.fillRectangle(0, 0, 100, 1);

You do not get a line drawn along the "x-axis" (y == 0). You get a line centered around y == 0.5, extending +/-0.5. The point (0,0) is really (0.5, 0.5). So, if you want that same line at 300% to cover the first 3 pixels, you need to scale about the "real" origin, like so:

transform.translate(-0.5, -0.5);
transform.scale(3, 3);
transform.translate(0.5, 0.5);

This even works at 200%, so there are no tests needed in SWT to see if the line width is "even".  The line falls exactly on 2 pixels, even if anti-aliased. Using the correct 200% transform yields results equivalent to calling:

gc.setLineWidth(2);
gc.drawLine(0.5, 0.5, 198.5, 0.5);
//(CAP_SQUARE should be used, for best results)

Since drawing is shifted down and to the right by 1/2 pixel, you end up with a line from (1,1) to (199,1), expanded by 1 (half the line width).  With CAP_SQUARE, the result is the rectangle (0, 0, 200, 2), as expected.
Comment 6 Randy Hudson CLA 2008-08-27 15:23:51 EDT
Note - other bugs encountered:

- when "Textured" background option is enabled, fillGradientRectangle at 200% and above does NOT result in a gradient. The Pattern appears instead.

- drawString(... transparent:FALSE) fills a rectangle starting 1-pixel to the left of the given x location, and whose right edge extends 1-pixel beyond the width of the string's extent. With advanced graphics turned on, the problem goes away (the filled rectangle is exactly the width/height of the string's extent).
Comment 7 Randy Hudson CLA 2008-10-14 13:14:11 EDT
Silenio, what do you think of the proposed patch?  This type of change could affect clients who may have worked around the current behavior, so it would be best if we could deliver it early in the release cycle.
Comment 8 Randy Hudson CLA 2008-11-17 17:18:48 EST
Silenio??
Comment 9 Silenio Quarti CLA 2008-11-20 10:49:01 EST
Sorry, it is taking so long for me to look at this bug. I have done some investigation already and I will update the bug shortly.
Comment 10 Silenio Quarti CLA 2008-11-25 11:19:06 EST
Ok, I finally understood this problem. The patch offsets fill operations by -0.5, but it does this after the user transformation. So if the user transformation scales by 2, the offset is actually -1 pixel in device coordinates. This causes fill operations to draw in the wrong x,y position when scaled. The following sample shows the problem. Change the scale factor and you will see the filled rectangle moving in the final picture.

I believe the proper code in gdiFillPrepare() is to transform it by -0.5 using Gdip.MatrixOrderAppend instead of Gdip.MatrixOrderPrepend, but when this is done, the original bug comes back.

This problem happens because subtracting -1 from the width and height of the stroked rectangle to make the stroked rectangle draw inside the filled rectangle is only correct when the line width is 1 and the transformation is the identity. I will post a snippet that shows how to do this next. 

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Rectangle;
import org.eclipse.swt.graphics.Transform;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;


public class PR242280 {
	static final int SCALE = 10;
	public static void main(String[] args) {
		final Display display = new Display();
		Shell shell = new Shell(display);
        shell.setBackground(display.getSystemColor(SWT.COLOR_WHITE));
		
		shell.addListener(SWT.Paint, new Listener() {
			public void handleEvent(Event event) {
				GC gc = event.gc;
				
				gc.drawLine(30, 30, 30, 1000);
				gc.drawLine(30, 30, 1000, 30);
				gc.drawLine(30 + 3 * SCALE, 28, 30 + 3 * SCALE, 32);
				
				Transform t = new Transform(display);
				t.translate(30, 30);
				t.scale(SCALE, SCALE);
				gc.setTransform(t);
				t.dispose();
				Rectangle r = new Rectangle(0, 0, 3, 3);
				gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
				gc.setLineWidth(1);
				gc.fillRectangle(r);
			}
		});

		shell.setBounds(10, 10, 200, 200);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}
}
Comment 11 Silenio Quarti CLA 2008-11-25 12:08:30 EST
Here is the snippet I mentioned above:

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Path;
import org.eclipse.swt.graphics.Transform;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;


public class PR242280_2 {
	static final int SCALE = 10;
	public static void main(String[] args) {
		final Display display = new Display();
		Shell shell = new Shell(display, SWT.DOUBLE_BUFFERED | SWT.SHELL_TRIM);
        shell.setBackground(display.getSystemColor(SWT.COLOR_WHITE));
		
		shell.addListener(SWT.Paint, new Listener() {			
			public void handleEvent(Event event) {
				GC gc = event.gc;
				
				int originX = 30, originY = 30;
				int x = 0, y = 0;
				int width = 10, height = 10;
				int lineWidth = 1;
				
				gc.drawLine(originX, originY, originX, 1000);
				gc.drawLine(originX, originY, 1000, originY);
				gc.drawLine(originX + width * SCALE, originX - 2, originX + height * SCALE, originX + 2);
				
				Transform t = new Transform(display);
				t.translate(originX, originY);
				t.scale(SCALE, SCALE);
				gc.setTransform(t);
				t.dispose();
				
				gc.setLineWidth(lineWidth);
//				gc.setAntialias(SWT.ON);
				gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
				gc.setForeground(display.getSystemColor(SWT.COLOR_BLACK));
				
				/*
				 * Note that if a transformation is applied, it is necessary
				 * to use path in order to have fractional shapes.
				 */
				
				{
				Path path = new Path(gc.getDevice());
				path.addRectangle(x, y, width, height);
//				path.addArc(x, y, width, height, 0, 360);
				gc.fillPath(path);
				path.dispose();
				}
				
				//Only necessary to show the stroke on top the fill
				gc.setAlpha(0x7f);
				
				{
				Path path = new Path(gc.getDevice());
				float originOffset =  lineWidth / 2f;
				float extentOffset = (float)lineWidth;
				path.addRectangle(x + originOffset, y + originOffset, width - extentOffset, height - extentOffset);				
//				path.addArc(x + originOffset, y + originOffset, width - extentOffset, height - extentOffset, 0, 360);
				gc.drawPath(path);
				path.dispose();
				}				
			}
				
		});

		shell.setBounds(10, 10, 200, 200);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}	
}
Comment 12 Randy Hudson CLA 2008-11-25 16:00:16 EST
Created attachment 118697 [details]
Silenio's snippet running with the patch

Silenio, I ran your snippet with my patch and the behavior seems completely normal. Could you describe what is wrong in the screenshots?

(I've modified the colors and turned off the alpha in favor of a dashed line)
Comment 13 Randy Hudson CLA 2008-11-25 16:21:40 EST
Silenio, your example never painted properly unscaled. If the origin is (0,0), and line width is 1, your example draws a rectangle whose top-left corner begins at (0.5, 0.5), which is *NOT* (0,0). So, when scaled to 10x, the rectangle is supposed to be ~5 pixels (or 1/2 the line width) off the axes. Instead, it touches the axes, which it shouldn't. Your snippet looks correct at 100% if antialiasing is turned off, but if you turn on anti-aliasing and remove the transform in your original snippet, you can see that the rectangle does not really start at 0,0.

Here is the correct version of the code, which works both with and without the transform, and with and without anti-aliasing enabled. Of course, you have to apply my patch for it to paint properly.

shell.addListener(SWT.Paint, new Listener() {
	public void handleEvent(Event event) {
		GC gc = event.gc;

		int originX = 30, originY = 30;
		int x = 0, y = 0;
		int width = 10, height = 10;
		int lineWidth = 1;

		gc.drawLine(originX, originY, originX, 1000);
		gc.drawLine(originX, originY, 1000, originY);
		gc.drawLine(originX + width * SCALE, originX - 2, originX + height
				* SCALE, originX + 2);

		Transform t = new Transform(display);
		t.translate(originX - 0.5f, originY - 0.5f);
		t.scale(SCALE, SCALE);
		t.translate(.5f, .5f);
		gc.setTransform(t);
		t.dispose();

		gc.setLineWidth(lineWidth);
		gc.setAntialias(SWT.ON);

		gc.setBackground(display.getSystemColor(SWT.COLOR_GREEN));

		gc.setForeground(display.getSystemColor(SWT.COLOR_BLUE));

		float originOffset = lineWidth / 2f;
		/*
		 * Note that if a transformation is applied, it is necessary to use path
		 * in order to have fractional shapes.
		 */
		{
			Path path = new Path(gc.getDevice());
			path.addRectangle(x - originOffset, y - originOffset, width, height);
			// path.addArc(x, y, width, height, 0, 360);
			gc.fillPath(path);
			path.dispose();
		}
		
		gc.setAlpha(0x7f);

		{
			Path path = new Path(gc.getDevice());
			float extentOffset = (float) lineWidth;
			path.addRectangle(x, y, width
					- extentOffset, height - extentOffset);
			// path.addArc(x + originOffset, y + originOffset, width -
			// extentOffset, height - extentOffset, 0, 360);
				LineAttributes la = gc.getLineAttributes();
				la.dash = new float[]{2f, 2f};
				la.cap = SWT.CAP_FLAT;
				la.style = SWT.LINE_CUSTOM;
				la.dashOffset = 0.5f;
				gc.setLineAttributes(la);
				gc.drawPath(path);
				path.dispose();
			}
		}

	});

	shell.setBounds(10, 10, 200, 200);
	shell.open();
	while (!shell.isDisposed()) {
		if (!display.readAndDispatch())
			display.sleep();
	}
	display.dispose();
}
Comment 14 Silenio Quarti CLA 2008-11-25 16:38:06 EST
Created attachment 118703 [details]
screenshot

This is what I get running the snippet from comment#10. Note that the red square is not at the origin (30, 30).

Is the patch you are running different than mine? The patch from comment#3 does not apply completely to HEAD. I have to comment the block that calculate the DRAW_OFFSET.
Comment 15 Felipe Heidrich CLA 2008-11-25 16:50:46 EST
Created attachment 118706 [details]
screenshot

Running PR242280_2, only changed I made was in this line:
originX = 1, originY = 1;
The SWT I used is exactly what is in HEAD right now.

Note that the rectangle (the fill and the draw) is exactly at 1,1
Comment 16 Silenio Quarti CLA 2008-11-25 16:58:45 EST
Created attachment 118710 [details]
Screenshot with lineWidth = 3

Change the code from comment#13 to lineWidth=3 and look at the output.
Comment 17 Silenio Quarti CLA 2008-11-25 17:04:28 EST
(In reply to comment #16)
> Created an attachment (id=118710) [details]
> Screenshot with lineWidth = 3
> 
> Change the code from comment#13 to lineWidth=3 and look at the output.
> 

Forgot to say to run it with the patch applied.
Comment 18 Randy Hudson CLA 2008-11-25 17:17:14 EST
Silenio, the code in comment 10 must be modified to read:

	t.translate(29.5f, 29.5f);
	t.scale(SCALE, SCALE);
	t.translate(0.5f, 0.5f);

This is something that could be hidden from the client. The patch could be modified to prepend and append the necessary 1/2 pixel shifts. I don't really understand why GDI-plus is performing scaling around the origin (0.5, 0.5).
Comment 19 Randy Hudson CLA 2008-11-25 17:24:10 EST
Here's the change to hide the 1/2 pixel translation from the user:

Along with the patch, in GC#setTransform(...), change to:
...
if (transform != null) {
+	Gdip.Matrix_Translate(identity, -0.5f, -0.5f, Gdip.MatrixOrderPrepend);
	Gdip.Matrix_Multiply(identity, transform.handle, Gdip.MatrixOrderPrepend);
+	Gdip.Matrix_Translate(identity, +0.5f, +0.5f, Gdip.MatrixOrderPrepend);
}
...

At one point I tried always using Gdip.PixelOffsetModeHalf, but I couldn't get that to work as well. I think that all of the brushes and pens had to be fixed by translating them 1/2 pixel, so NONE seemed to be the way to go.
Comment 20 Felipe Heidrich CLA 2008-11-25 17:34:00 EST
Ok, I went over the changes you made in comment #13, mainly:

1) 
t.translate(originX - 0.5f, originY - 0.5f);
t.scale(SCALE, SCALE);
t.translate(.5f, .5f);

2) You add subtracting lineWidth/2 from x and y in the fill
3) You removed adding lineWidth/2 from x and y in the draw

Given that, the figure is exactly were it was supposed to be. 
-translate 29.5 to the right, then translate more 5 (0.5*scale) to right
-then you subtracted 5 in the fill (linewidth/2 * scale), so the fill falls exactly at 29.5


------
What are you trying to do ?

I thought you were trying to have the draw within the fill. Am I right ?

Do you also want your figure to be antialiased ?
Note that SWT snap all draws and fill in the middle of the pixel to make advance and non-advance drawing consistent.
If that is the case all you have to do is to add .5 to the very first translate, the one before the scale.
Comment 21 Randy Hudson CLA 2008-11-25 20:02:16 EST
Created attachment 118731 [details]
GC and GCData patch #2

Here's an updated patch.

Changes:
- Merged with HEAD
- Updated setTransform so that scaling is about the origin
Comment 22 Randy Hudson CLA 2008-11-25 20:05:41 EST
Created attachment 118733 [details]
Advanced Graphics Test Package 2.0

Updated version of the test package.
Changes:
- removed translations that were rolled into GC
- Added additional "Tranpose" test setting
Comment 23 Randy Hudson CLA 2008-11-25 20:14:12 EST
> What are you trying to do ?
> 
> I thought you were trying to have the draw within the fill. Am I right ?

It's Silenio's snippet. I've uploaded a much larger snippet which visually tests the following assumptions:

1) fillOval(...) should behave the same with/without an identity transform applied (i.e., turning on GDI+)
2) drawOval(...) should behave the same with/without .....
3) same for fillRect(...), drawRect(...), roundedRect, gradientRect, etc.
4) drawPolygon(...) should behave the same as a Path with the same points

Please take a look at the test package with and without the patch applied. Also, let me know if the above assumptions are wrong. For example, maybe these two approaches to drawing a rectangle are supposed to behave differently:

gc.drawRectangle(0,0,10,10);
// vs.
path.moveTo(0,0);
path.lineTo(10,0);
path.lineTo(10,10);
path.lineTo(0,10);
path.lineTo(0,0);
gc.drawPath(path);

(Without my patch, the path's rectangle is shifted down and right 1/2 pixel)
Comment 24 Randy Hudson CLA 2008-11-26 00:08:53 EST
Here's another assumption which fails (without the patch):
- anti-aliased drawRectangle(0,0,10,10) translated by (2,2) should look the same as before, only shifted down and right exactly 2 pixels. In 3.4 at least, the translation would cause a non-zero gdipX/YOffset, causing bogus results.
Comment 25 Silenio Quarti CLA 2008-11-26 11:07:53 EST
(In reply to comment #24)
> Here's another assumption which fails (without the patch):
> - anti-aliased drawRectangle(0,0,10,10) translated by (2,2) should look the
> same as before, only shifted down and right exactly 2 pixels. In 3.4 at least,
> the translation would cause a non-zero gdipX/YOffset, causing bogus results.
> 

This has been fixed in HEAD. We are calculating the scaling factor with Graphics.TransformPoints() instead of Graphics.TransformVectors().
Comment 26 Silenio Quarti CLA 2008-11-26 11:13:20 EST
(In reply to comment #23)
> > What are you trying to do ?
> > 
> > I thought you were trying to have the draw within the fill. Am I right ?
> 
> It's Silenio's snippet. I've uploaded a much larger snippet which visually
> tests the following assumptions:
> 
> 1) fillOval(...) should behave the same with/without an identity transform
> applied (i.e., turning on GDI+)
> 2) drawOval(...) should behave the same with/without .....
> 3) same for fillRect(...), drawRect(...), roundedRect, gradientRect, etc.
> 4) drawPolygon(...) should behave the same as a Path with the same points
> 
> Please take a look at the test package with and without the patch applied.
> Also, let me know if the above assumptions are wrong. For example, maybe these
> two approaches to drawing a rectangle are supposed to behave differently:
> 
> gc.drawRectangle(0,0,10,10);
> // vs.
> path.moveTo(0,0);
> path.lineTo(10,0);
> path.lineTo(10,10);
> path.lineTo(0,10);
> path.lineTo(0,0);
> gc.drawPath(path);
> 
> (Without my patch, the path's rectangle is shifted down and right 1/2 pixel)
> 

Please give me a snippet that shows this. I believe both drawRectangle() and drawPath() shift down and right 1/2 pixel. So the drawing is identical.

This is the snippet I tried (running against HEAD). We have to play with the usePath flag. I will attach a screenshot.

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Path;
import org.eclipse.swt.graphics.Transform;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;


public class PR242280_4 {
	static final int SCALE = 1;
	public static void main(String[] args) {
		final Display display = new Display();
		Shell shell = new Shell(display, SWT.DOUBLE_BUFFERED | SWT.SHELL_TRIM);
        shell.setBackground(display.getSystemColor(SWT.COLOR_WHITE));
		
		final boolean usePath = true;
		if (usePath) {
			shell.setText("drawPath");
		} else {
			shell.setText("drawRect");
		}
        
		shell.addListener(SWT.Paint, new Listener() {			
			public void handleEvent(Event event) {
				GC gc = event.gc;
				
				int originX = 30, originY = 30;
				int x = 0, y = 0;
				int width = 10, height = 10;
				int lineWidth = 1;
				
				gc.drawLine(originX, originY, originX, 1000);
				gc.drawLine(originX, originY, 1000, originY);
				gc.drawLine(originX + width * SCALE, originX - 2, originX + height * SCALE, originX + 2);

				Transform t = new Transform(display);
				t.translate(originX, originY);
				t.scale(SCALE, SCALE);
				gc.setTransform(t);
				t.dispose();
				
				gc.setLineWidth(lineWidth);
				gc.setAntialias(SWT.ON);
				gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
				gc.setForeground(display.getSystemColor(SWT.COLOR_RED));

				
				if (usePath) {
					Path path = new Path(gc.getDevice());
					path.addRectangle(x, y, width, height);
					gc.drawPath(path);
					path.dispose();
				} else {
					gc.drawRectangle(x, y, width, height);
				}
		
			}
				
		});

		shell.setBounds(10, 10, 200, 200);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}	
}
Comment 27 Silenio Quarti CLA 2008-11-26 11:14:16 EST
Created attachment 118808 [details]
screenshoot for drawPath versus drawRectangle
Comment 28 Silenio Quarti CLA 2008-11-26 11:26:18 EST
With the patch, fillPath() does not shift up and left so it draws different from fillRectangle() (which does).

Again play with the usePath flag. I will attach a screenshot.

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Path;
import org.eclipse.swt.graphics.Transform;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;


public class PR242280_3 {
	static final int SCALE = 10;
	public static void main(String[] args) {
		final Display display = new Display();
		Shell shell = new Shell(display, SWT.DOUBLE_BUFFERED | SWT.SHELL_TRIM);
        shell.setBackground(display.getSystemColor(SWT.COLOR_WHITE));
        
		final boolean usePath = true;
		final boolean fill = true;
		if (fill) {
			if (usePath) {
				shell.setText("fillPath");
			} else {
				shell.setText("fillRect");
			}
		} else {
			if (usePath) {
				shell.setText("drawPath");
			} else {
				shell.setText("drawRect");
			}
		}
		
		shell.addListener(SWT.Paint, new Listener() {			
			public void handleEvent(Event event) {
				GC gc = event.gc;
				
				int originX = 30, originY = 30;
				int x = 0, y = 0;
				int width = 10, height = 10;
				int lineWidth = 1;
				
				gc.drawLine(originX, originY, originX, 1000);
				gc.drawLine(originX, originY, 1000, originY);
				gc.drawLine(originX + width * SCALE, originX - 2, originX + height * SCALE, originX + 2);

				Transform t = new Transform(display);
				t.translate(originX, originY);
				t.scale(SCALE, SCALE);
				gc.setTransform(t);
				t.dispose();
				
				gc.setLineWidth(lineWidth);
				gc.setAntialias(SWT.ON);
				gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
				gc.setForeground(display.getSystemColor(SWT.COLOR_RED));

				
				if (fill) {
					if (usePath) {
						Path path = new Path(gc.getDevice());
						path.addRectangle(x, y, width, height);
						gc.fillPath(path);
						path.dispose();
					} else {
						gc.fillRectangle(x, y, width, height);
					}
				} else {
					if (usePath) {
						Path path = new Path(gc.getDevice());
						path.addRectangle(x, y, width, height);
						gc.drawPath(path);
						path.dispose();
					} else {
						gc.drawRectangle(x, y, width, height);
					}
				}
		
			}
				
		});

		shell.setBounds(10, 10, 200, 200);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}	
}
Comment 29 Silenio Quarti CLA 2008-11-26 11:27:02 EST
Created attachment 118813 [details]
fillRectangle versus fillPath with patch
Comment 30 Silenio Quarti CLA 2008-11-26 11:53:21 EST
With the patch applied, the stroke is not centered in the path edge when scaling using transforms (it is aligned on the inside of the edge). The user should be able to scale the path manually and have the same output as applying the equivalent transformation.

Play with the useTransform flag. I will attach screenshot.

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Transform;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;


public class PR242280_5 {
	static final int SCALE = 10;
	public static void main(String[] args) {
		final Display display = new Display();
		Shell shell = new Shell(display, SWT.DOUBLE_BUFFERED | SWT.SHELL_TRIM);
        shell.setBackground(display.getSystemColor(SWT.COLOR_WHITE));
		
        final boolean useTransform = false;
        if (useTransform) {
        	shell.setText("setTransform");
        } else {
        	shell.setText("scale by hand");
        }
        
		shell.addListener(SWT.Paint, new Listener() {			
			public void handleEvent(Event event) {
				GC gc = event.gc;
				
				int originX = 30, originY = 30;
				int x = 0, y = 0;
				int width = 10, height = 10;
				int lineWidth = 1;
				
				gc.drawLine(originX, originY, originX, 1000);
				gc.drawLine(originX, originY, 1000, originY);
				gc.drawLine(originX + width * SCALE, originX - 2, originX + height * SCALE, originX + 2);

				if (useTransform) {
					Transform t = new Transform(display);
					t.translate(originX, originY);
					t.scale(SCALE, SCALE);
					gc.setTransform(t);
					t.dispose();
				} else {
					x += originX;
					y += originY;
					width *= SCALE;
					height *= SCALE;
					lineWidth *= SCALE;
				}
				
				gc.setLineWidth(lineWidth);
//				gc.setAntialias(SWT.ON);
				gc.setForeground(display.getSystemColor(SWT.COLOR_RED));

				gc.drawRectangle(x, y, width, height);
			}
				
		});

		shell.setBounds(10, 10, 200, 200);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}	
}
Comment 31 Silenio Quarti CLA 2008-11-26 11:54:31 EST
Created attachment 118818 [details]
setTransform() versus scaling by hand with patch
Comment 32 Randy Hudson CLA 2008-12-10 11:13:41 EST
Here is another snippet which produces strange results without the patch:

shell.addListener(SWT.Paint, new Listener() {
	public void handleEvent(Event event) {
		GC gc = event.gc;
		gc.setLineWidth(1);
		Transform t= new Transform(null);
		t.scale(6, 6);
		gc.setTransform(t);
		gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
		gc.fillRectangle(0, 0, 10, 10);
		gc.drawRectangle(1, 1, 7, 7);
		t.translate(15, 5);
		t.rotate(180);
		t.translate(-15, -5);
		gc.setTransform(t);
		gc.setBackground(display.getSystemColor(SWT.COLOR_BLUE));
		gc.fillRectangle(10, 0, 10, 10);
		gc.drawRectangle(11, 1, 7, 7);
	}
});

What's happening is the 1/2-pixel shift should be in the up+left direction due to the 180-degree rotation. I mentioned this via e-mail. I already have the fix incorporated in with my other changes.
Comment 33 Randy Hudson CLA 2008-12-10 11:55:45 EST
We need to decide what set of options to expose to clients. I think the choices are:

1) RAW - do nothing
2) HALF - Shift all operations by 1/2 pixel (equivalent to PixelOffsetNone)
3) SHIFT_ODD_DRAWING - When line width is odd, shift all drawing by (+.5,+.5).
4) SHIFT_EVEN_DRAWING - When line width is even, shift drawing by (-.5, -.5). This isn't really useful for clients, but it is how one implements (3) on top of a "PixelOffsetNone" drawing system.

Scope of the shifting (when to shift):
A) "INTEGER_DRAWING" - Only shift for drawing API which takes integers. If Paths are used, the behavior is RAW.
B) "INTEGER_DRAWING_AND_FILLS" - Same as (A), but fills, especially fillPolygon(int[]), need to be shifted too. The shifting for fills could be sensitive to the linewidth being odd. Example:
    //Trying to draw an "down" triangle
    int triangle[] = new int[]{0,0,  8,0,  4,4};
    gc.setBackground(blue); gc.setForeground(black);
    gc.fillPolygon(triangle);
    gc.drawPolygon(triangle);
Without shifting both the draw and fill, the triangle will look bad, with anti-aliasing on or off.

IMHO, I would only ever want the following two modes:

MODE1 : (1) RAW
MODE2 : (3+B) When line width is odd and using integer API, shift both strokes and fills.

MODE2 is basically the "win32-GDI compatibility mode", but it is also the "convenience" mode, allowing clients to work with integers and still get crisp output.

I already have these two modes implemented for win32, but I don't like the way that I have to do the shifting. I think GDI+ and WPF both have API which takes floats. It would be much easier if we could call those methods instead of playing with the transforms.

Silenio-
Can you guys decide IF and WHICH modes you want? It's getting pretty far along in the milestones and GEF would need some time afterwards to benefit from these improvements in SWT. I can work on a patch for win32 and Cocoa once I hear back from you, although I don't know how to add new native methods to OS/Gdip.
Comment 34 Felipe Heidrich CLA 2008-12-10 14:58:14 EST
Hi Randy, Silenio is on vacation. He will be back on the first week of January.

Last time I talked to him we talked about adding a pixel "mode" for GC.

The two modes we would have:

1) the current one (unmodified), or win32-GDI compatibility mode as you called.

2) PixelOffsetModeHalf, which you called RAW I believe. The implementation would be the same in all platforms: don't compute drawXOffset, drawYOffset in checkGC(); don't ever translate anything for free. Let the native drawing draw where it wants. GDI+, cairo, and mac are consistent in this matter.

We will have to wait SSQ to get back before making any decision. But can you try (2) and let us know if it would work for you ?
Note: this is how GDI+, cairo, and mac defined the drawing natively. If it worked for them, it shold work for us.
Comment 35 Randy Hudson CLA 2008-12-11 09:39:28 EST
Created attachment 120194 [details]
Patch with API to toggle compatibility mode

Felipe,
here's the patch which adds the API for toggling pixel offset compability mode.
Comment 36 Eclipse Webmaster CLA 2019-09-06 15:30:26 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.