-
Notifications
You must be signed in to change notification settings - Fork 49
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
Conversation
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);
} |
Originally I only had it in the notifyPainting method and especially on zooming I noticed quite some speedup, as it cancels unnecessary paints. |
@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? |
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. |
@Phillipus did you find some time to read my comment and re-review my code? |
Sorry, not yet. Will do it this week! :-) |
@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: Is rendered like this: |
@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. |
This was on Windows. |
Ok. Strange. |
@Phillipus could you try without my the last commit in my PR? |
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. |
@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? |
@azoitl I tested on Windows and Mac and it all seems to be working OK. |
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. |
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.