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 drawPolyline. Issue #148 #149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DieterMai
Copy link

Fix for Issue #148

Copy link

github-actions bot commented Mar 7, 2025

Test Results

   338 files  ±0     338 suites  ±0   2m 6s ⏱️ -23s
 3 954 tests ±0   3 670 ✅ ±0  284 💤 ±0  0 ❌ ±0 
11 690 runs  ±0  10 805 ✅ ±0  885 💤 ±0  0 ❌ ±0 

Results for commit 067380a. ± Comparison against base commit fb29be9.

♻️ This comment has been updated with latest results.

@tmssngr
Copy link

tmssngr commented Mar 8, 2025

Do you have a snippet to quickly test-drive it before and later? What about the different join- and cap-styles?

@DieterMai
Copy link
Author

I will create a test for this

@DieterMai
Copy link
Author

I still want to implement some kind of example to better test this drawing operation. But you can also use Snippet 252 with some minor changes to see the issue

public class Snippet252 {

	public static void main(String[] args) {
		final Display display = new Display();
		final Shell shell = new Shell(display);
		shell.setText("Snippet 252");

		SWT.USE_SKIJA = true;
		shell.addListener(SWT.Paint, event -> {
			Drawing.drawWithGC(shell, event.gc, gc -> {
				gc.setLineAttributes(new LineAttributes(10, SWT.CAP_FLAT, SWT.JOIN_MITER, SWT.LINE_SOLID, null, 0, 10));
				gc.drawPolyline(new int[]{50, 100, 50, 20, 60, 30, 50, 45});

				gc.setLineAttributes(new LineAttributes(1/2f, SWT.CAP_FLAT, SWT.JOIN_MITER, SWT.LINE_DOT, null, 0, 10));
				gc.drawPolyline(new int[]{100, 100, 100, 20, 110, 30, 100, 45});
			});

		});

		shell.setSize(150, 150);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) display.sleep();
		}
		display.dispose();
	}
}

@DieterMai DieterMai marked this pull request as draft March 8, 2025 21:16
@DieterMai
Copy link
Author

I converted this to draft because this is more buggy then i thought.

The methods drawPolygon and fillPolygon are also not correct.

What the skija implementation does is it takes pairs of 3 points and draws triangles. Meaning if skija gets 6 points it will just draw 2 triangles.

@DieterMai DieterMai marked this pull request as ready for review March 9, 2025 11:05
@DieterMai
Copy link
Author

Reimplemented skijas drawPolyline, drawPolygon, and fillPolygon.

The fillPolygon function still behaves different in some cases but it is a lot closer to what the previous implementation did.

2 other changes

  • I had to fix a small bug in ToolItem. The selection values has to be set before notifying listeners.
  • I implemented a test stand for the polyline related function. The existing graphics example is not sufficient to test this. If this is too much, i can remove it. It's just for testing.

@tmssngr
Copy link

tmssngr commented Mar 10, 2025

Thanks for the "Polyline teststand". It reveals a couple of bugs in SWT:

  • Line Cap, Line Join and Line Style do not have any influence yet
  • on my 4k display (175% zoom) clicking in the draw area seems to half the relative point coordinates
    Polyline teststand

It looks like you've detected it while working on the Tree implementation. Do you need it for drawing the expansion/collapse triangles? For all horizonal/vertical lines I'd use fillRect with width or height set to 1.

@DieterMai
Copy link
Author

DieterMai commented Mar 10, 2025

  • on my 4k display (175% zoom) clicking in the draw area seems to half the relative point coordinates

Is this also the case for the native GC implementation?

It looks like you've detected it while working on the Tree implementation. Do you need it for drawing the expansion/collapse triangles? For all horizontal/vertical lines I'd use fillRect with width or height set to 1.

Yes, i detected it while working on the expanse/collapse symbols. But it is not a blocker. I just encountered it while on it.

@tmssngr
Copy link

tmssngr commented Mar 10, 2025

Is this also the case for the native GC implementation?

No, with native GC this is as expected. It looks like SkijaGC.toOpenPath lacks the DPIUtil.autoScaleUp.

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.

2 participants