Force repaint in NativeGraphicsSource to fix broken animation #445
Conversation
|
That's my take on the animation issue, I suppose. I've tested this fix against https://bugs.eclipse.org/bugs/show_bug.cgi?id=137786#c23 and while I can reproduce those artifacts with I've also tested the animations on both Windows and Linux. Both work with this fix. |
|
@azoitl There are currently three potential fixes for the animation bug.
Which one do you like the most? If you want to test the changes and how they interact with https://bugs.eclipse.org/bugs/show_bug.cgi?id=137786, you can uncomment the Bug137786.webm |
|
I'm not really sure. While I like that your change fixes it more generally, I'm a bit nervous by it's implication. Triggering an updated/repaint every time when getGraphics is called may also mean a lot of repaints. As these where partly the cause of the performance problems we investigated in the last weeks I somehow fear we turn it worse again. Or am I missunderstanding something. |
|
From my perspective, a repaint has to be done, in order to satisfy the contract of |
|
I spent more time on better understanding it and it looks like I'm more confused. With your explanation and reading the code I think it is so far the best solution. But I would really be interested what @Phillipus thinks of it. |
|
The underlying problem is that the entire animation is done inside the UI thread. So any intermediate paint operations can't be done unless explicitly invoked. Previously, this was done implicitly by calling
|
|
@azoitl
Scratch that, I'm currently writing Zest tests and one of the tests crashs with an SWT error when calling |
|
I've cherry-picked #377 and added it on top of #446. This error should not happen: I don't really like rushing things, so I want to propose the following: Neither of those PRs should be included for the M3. Instead, I'd like to finish the tests, merge them and then rebase all PRs to see whether any of them introduce regressions. |
|
Tonight I found finally time to do a bit of performance testing (Debian Testing i7 Laptop plugged in). I used one of the larger models that we show in 4diac IDE. There opening the editor without this PR takes app. 13s with the PR app. 16s (measured with a stopwatch on my phone). Opening this model includes parsing a 100MB XML file. This takes about 4s. removing this 4s with the PR our editors are opening around 25% slower. However during usage of the editors I didn't notice any noticeable delays. When I learned something in the last half year then that rushing something is definitely a bad idea. |
Out of curiosity, how long does it take with the "original" implementation? i.e. when |
As you have anticipated it takes much longer. Here we are at around 29 seconds. |
|
Hm... I'm a little curious about the test failures. I'll run some tests to see whether this is because of this PR or if they are generally not 100% stable. |
When the FigureCanvas is created with SWT.DOUBLE_BUFFERED, animations are not painted correctly. This is because an instance of NativeGraphicsSource is used internally, which doesn't paint synchronously. Because the animation is done inside the UI thread, this paint operation is only processed after the animation is done. Resolves eclipse#376
| @@ -129,7 +129,7 @@ private void doTest(Snippet annotation, Statement statement) throws Throwable { | |||
| robot = new GraphicalRobot(graph); | |||
| shell = graph.getShell(); | |||
| // Wait for layout to be applied | |||
| waitEventLoop(0); | |||
There was a problem hiding this comment.
We currently only wait a single cycle for the layout operation to complete. Calling readAndDispatch() within the animation seems to mess with scheduling, so this is no longer enough.
|
Too late for the 3.20, but I would've considered it too risky, anyway. |
When the FigureCanvas is created with SWT.DOUBLE_BUFFERED, animations are not painted correctly. This is because an instance of NativeGraphicsSource is used internally, which doesn't paint synchronously.
Because the animation is done inside the UI thread, this paint operation is only processed after the animation is done.
Resolves #376