Force repaint in NativeGraphicsSource to fix broken animation

[ptziegler]

trafficstars

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 https://github.com/eclipse/gef-classic/issues/376

[ptziegler]

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 canvas.update(), I can't with this fix. Do note that those issue only seem to appear on Windows.

I've also tested the animations on both Windows and Linux. Both work with this fix.

[ptziegler]

@azoitl There are currently three potential fixes for the animation bug.

• https://github.com/eclipse/gef-classic/pull/377, which only fixes the animation but nothing else.
• https://github.com/eclipse/gef-classic/pull/378, which fixes it in general, but -as far as I can tell- has an excessive update when a BufferedGraphicsSource is used.
• And this one, of course, which I believe is the closest to the previous implementation.

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 canvas.update() and try out the example project that was added to the Bugzilla item. circleEditor.zip

Bug137786.webm

[azoitl]

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.

[ptziegler]

From my perspective, a repaint has to be done, in order to satisfy the contract of LayoutManager.performUpdate(). Alternatively, we can always go with the solution proposed by Phillipus (PR 377), which only forces a repaint within the animation.

[azoitl]

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.

[ptziegler]

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 canvas.update() for the NativeGraphicsSource or explicitly by directly painting the (buffered) image for the BufferedGraphicsSource.

Display.readAndDispatch() does effectively the same as canvas.update(), except that it is a lot more conservative (as it only executes one event) and the main topic of discussion is now whether it makes sense to only call it for the animations or in general, when using this graphics source.

[ptziegler]

@azoitl ~~Speaking about performance: At least on Linux, my solution seems to be slower. When you try it out with the ZoomSnippet, you have a 4-5 seconds warmup time where GTK is doing... something.~~

~~My guess is because the update manager does some additional operations like layouts and validation. Which seems to cause additional synchronization, depending on the operating system.~~

Scratch that, I'm currently writing Zest tests and one of the tests crashs with an SWT error when calling readAndDispatch() inside the Animation class.

[ptziegler]

I've cherry-picked https://github.com/eclipse/gef-classic/pull/377 and added it on top of https://github.com/eclipse/gef-classic/pull/446.

This error should not happen:

2024-05-22T16:20:35.5200700Z org.eclipse.zest.tests.examples.GraphSWTTests.testZoomSnippet -- Time elapsed: 0.119 s <<< ERROR!
2024-05-22T16:20:35.5212790Z java.lang.IllegalArgumentException: Argument not valid
2024-05-22T16:20:35.5213270Z 	at org.eclipse.swt.SWT.error(SWT.java:4903)
2024-05-22T16:20:35.5213600Z 	at org.eclipse.swt.SWT.error(SWT.java:4837)
2024-05-22T16:20:35.5213930Z 	at org.eclipse.swt.SWT.error(SWT.java:4808)
2024-05-22T16:20:35.5214300Z 	at org.eclipse.swt.widgets.Widget.error(Widget.java:853)
2024-05-22T16:20:35.5215190Z 	at org.eclipse.swt.widgets.Widget.checkParent(Widget.java:570)
2024-05-22T16:20:35.5215780Z 	at org.eclipse.swt.widgets.Widget.<init>(Widget.java:145)
2024-05-22T16:20:35.5216200Z 	at org.eclipse.swt.widgets.Item.<init>(Item.java:76)
2024-05-22T16:20:35.5216650Z 	at org.eclipse.zest.core.widgets.GraphItem.<init>(GraphItem.java:47)
2024-05-22T16:20:35.5217180Z 	at org.eclipse.zest.core.widgets.GraphNode.<init>(GraphNode.java:120)
2024-05-22T16:20:35.5217680Z 	at org.eclipse.zest.core.widgets.GraphNode.<init>(GraphNode.java:116)
2024-05-22T16:20:35.5218230Z 	at org.eclipse.zest.core.widgets.GraphContainer.<init>(GraphContainer.java:94)
2024-05-22T16:20:35.5218880Z 	at org.eclipse.zest.examples.swt.ZoomSnippet.createContainer(ZoomSnippet.java:48)
2024-05-22T16:20:35.5219490Z 	at org.eclipse.zest.examples.swt.ZoomSnippet.main(ZoomSnippet.java:119)
2024-05-22T16:20:35.5220120Z 	at org.eclipse.zest.tests.examples.AbstractGraphTest.doTest(AbstractGraphTest.java:134)
2024-05-22T16:20:35.5220830Z 	at org.eclipse.zest.tests.examples.AbstractGraphTest$1.evaluate(AbstractGraphTest.java:79)
2024-05-22T16:20:35.5221430Z 	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
2024-05-22T16:20:35.5221990Z 	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
2024-05-22T16:20:35.5222560Z 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
2024-05-22T16:20:35.5223120Z 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
2024-05-22T16:20:35.5223760Z 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
2024-05-22T16:20:35.5224420Z 	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
2024-05-22T16:20:35.5224950Z 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
2024-05-22T16:20:35.5225480Z 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
2024-05-22T16:20:35.5225990Z 	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
2024-05-22T16:20:35.5226470Z 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
2024-05-22T16:20:35.5226960Z 	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
2024-05-22T16:20:35.5227430Z 	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
2024-05-22T16:20:35.5227860Z 	at org.junit.runners.Suite.runChild(Suite.java:128)
2024-05-22T16:20:35.5228230Z 	at org.junit.runners.Suite.runChild(Suite.java:27)
2024-05-22T16:20:35.5228630Z 	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
2024-05-22T16:20:35.5229090Z 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
2024-05-22T16:20:35.5229580Z 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
2024-05-22T16:20:35.5230070Z 	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
2024-05-22T16:20:35.5231090Z 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
2024-05-22T16:20:35.5231570Z 	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
2024-05-22T16:20:35.5232210Z 	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
2024-05-22T16:20:35.5232760Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
2024-05-22T16:20:35.5233440Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
2024-05-22T16:20:35.5234270Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
2024-05-22T16:20:35.5234960Z 	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
2024-05-22T16:20:35.5235580Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2024-05-22T16:20:35.5236280Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
2024-05-22T16:20:35.5237090Z 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2024-05-22T16:20:35.5237740Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
2024-05-22T16:20:35.5238390Z 	at org.apache.maven.surefire.api.util.ReflectionUtils.invokeMethodWithArray2(ReflectionUtils.java:137)
2024-05-22T16:20:35.5239190Z 	at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:148)
2024-05-22T16:20:35.5239930Z 	at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:88)
2024-05-22T16:20:35.5240650Z 	at org.eclipse.tycho.surefire.osgibooter.OsgiSurefireBooter.run(OsgiSurefireBooter.java:140)
2024-05-22T16:20:35.5241450Z 	at org.eclipse.tycho.surefire.osgibooter.HeadlessTestApplication.start(HeadlessTestApplication.java:29)
2024-05-22T16:20:35.5242200Z 	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
2024-05-22T16:20:35.5242980Z 	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
2024-05-22T16:20:35.5243890Z 	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
2024-05-22T16:20:35.5244590Z 	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
2024-05-22T16:20:35.5245200Z 	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
2024-05-22T16:20:35.5245820Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2024-05-22T16:20:35.5246520Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
2024-05-22T16:20:35.5247340Z 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2024-05-22T16:20:35.5247980Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
2024-05-22T16:20:35.5248490Z 	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
2024-05-22T16:20:35.5248970Z 	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
2024-05-22T16:20:35.5249400Z 	at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
2024-05-22T16:20:35.5251180Z 	at org.eclipse.equinox.launcher.Main.main(Main.java:1454)

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.

[azoitl]

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.

[ptziegler]

This takes about 4s. removing this 4s with the PR our editors are opening around 25% slower.

Out of curiosity, how long does it take with the "original" implementation? i.e. when canvas.update() is called?

[azoitl]

This takes about 4s. removing this 4s with the PR our editors are opening around 25% slower.

Out of curiosity, how long does it take with the "original" implementation? i.e. when canvas.update() is called?

As you have anticipated it takes much longer. Here we are at around 29 seconds.

[ptziegler]

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.

[ptziegler]

Too late for the 3.20, but I would've considered it too risky, anyway.