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

Reworked Thumbnail to not use scaled graphics anymore addresses #106 #198

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

azoitl
Copy link
Contributor

@azoitl azoitl commented Apr 29, 2023

The thumbnail is now using only the SWTGraphics and now scaled graphics anymore. Furthermore we stop now active updates of the thumbnail runable if during a repaint request it is running.

@Phillipus I would be happy on your feedback. I've the slight feeling if the performance got worse with this change but not fully sure.

@github-actions
Copy link

github-actions bot commented Apr 29, 2023

Unit Test Results

    9 files      9 suites   16s ⏱️
307 tests 305 ✔️ 2 💤 0
921 runs  915 ✔️ 6 💤 0

Results for commit 76d59e5.

♻️ This comment has been updated with latest results.

@Phillipus
Copy link
Contributor

I tested it on Mac and it works pretty much the same as before.

Did you notice a difference by adding this?

	@Override
	public void repaint(int x, int y, int w, int h) {
		if (updater.isRunning()) {
			updater.stop();
		}
		super.repaint(x, y, w, h);
	}

@azoitl
Copy link
Contributor Author

azoitl commented Apr 29, 2023

Did you notice a difference by adding this?

	@Override
	public void repaint(int x, int y, int w, int h) {
		if (updater.isRunning()) {
			updater.stop();
		}
		super.repaint(x, y, w, h);
	}

Originally I only had it in the notifyPainting method and especially on zooming I noticed quite some speedup, as it cancels unnecessary paints.

@azoitl
Copy link
Contributor Author

azoitl commented Jun 12, 2023

@Phillipus, I spent some time today looking into the performance issue of this and in 4diac IDE with a medium sized diagram it feels noticeable slower until the thumbnail is updated. Unfortunately I found no good way to reliably measure.

I have a strong feeling that the scale line in the code below is the problem:

org.eclipse.draw2d.geometry.Point p = getSourceRectangle().getLocation();
tileGraphics.translate(-p.x * getScaleX() - sx1, -p.y * getScaleY() - sy1);
tileGraphics.scale(getScaleX());
sourceFigure.paint(tileGraphics);
tileGraphics.popState();

I looked into the the code of both and the SWTGraphics seems to be more complicated. So what I would try and I'm not sure if possible is to set the scale only once. What do you think?

@azoitl
Copy link
Contributor Author

azoitl commented Jun 12, 2023

I found a way to move the scale method to the tileGraphics creation. For 4diac IDE it didn't change a lot and I think it is a different problem we have in 4diac IDE (has to do with our incremental growing canvas). But what I noticed is that the quality of the thumbnail is much better without the scaled graphics. So if @Phillipus agrees I think this PR can be submitted and it will lead to an improvement of GEF Classic.

@azoitl
Copy link
Contributor Author

azoitl commented Jul 2, 2023

@Phillipus did you find some time to read my comment and re-review my code?

@Phillipus
Copy link
Contributor

@Phillipus did you find some time to read my comment and re-review my code?

Sorry, not yet. Will do it this week! :-)

@Phillipus
Copy link
Contributor

@azoitl I did a very quick check on this and, unfortunately, the thumbnail is rendering a diagram multiple times, just like it did on Mac before the Mac workaround was added.

For example, this diagram:

1

Is rendered like this:

2

@azoitl
Copy link
Contributor Author

azoitl commented Jul 3, 2023

@Phillipus this is intersting. Because I thought I kept your code and just changed it such that it uses the SWTGraphics instead of the Scaled graphics. Let me look again on the code to find out where I messed up. Unfortunately I have no mac for testing.

@Phillipus
Copy link
Contributor

Unfortunately I have no mac for testing.

This was on Windows.

@azoitl
Copy link
Contributor Author

azoitl commented Jul 3, 2023

Unfortunately I have no mac for testing.

This was on Windows.

Ok. Strange.

@azoitl
Copy link
Contributor Author

azoitl commented Jul 3, 2023

@Phillipus could you try without my the last commit in my PR?

@Phillipus
Copy link
Contributor

Commit b90f8f6 is better. However, the thumbnail is initially blank and is only drawn when resized.

@azoitl
Copy link
Contributor Author

azoitl commented Jul 3, 2023

Commit b90f8f6 is better. However, the thumbnail is initially blank and is only drawn when resized.

Ok back to concept. I fear I really have to setup a windows dev environment for GEF Classic in my VM.

@azoitl
Copy link
Contributor Author

azoitl commented Jul 31, 2023

@Phillipus I reworked my patches to contain only the most minimal changes required for not using ScaledGraphics. At least for the Thumbnail example it seems to work for me under Linux and Windows. Could you be so kind and test as well?

@Phillipus
Copy link
Contributor

@azoitl I tested on Windows and Mac and it all seems to be working OK.

@azoitl
Copy link
Contributor Author

azoitl commented Aug 1, 2023

Then I would merge this for now, so that it gets into 3.17.0 M2 which is due tomorrow. I would not re-introduce any of my "performance" improvements for now.

Also 4diac IDE now uses GEF Classic latest for its test builds. With that we should get early feedback on any issues here.

@azoitl azoitl merged commit ca2506f into eclipse-gef:master Aug 2, 2023
4 checks passed
@azoitl azoitl deleted the thumbnail_update branch August 2, 2023 12:18
@azoitl azoitl added this to the 3.17.0 milestone Aug 2, 2023
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