Split out the GL backend

[jwiggins]

This PR aims to separate the Kiva OpenGL backend completely from the agg backend. This has several benefits:

• OpenGL can be avoided on headless installations with no GL libraries installed
• macOS builds on Catalina and later can skip OpenGL without losing agg
• The agg backend becomes a little bit simpler and (possibly) easier to replace with Celiagg

To that end, I've copied all of the GL-specific code from kiva.agg into a new kiva.gl subpackage. Included in the copy is a very small subset of agg 2.4 which is needed for affine transform, color, and path representations.

This will begin life as a draft PR, because there are a couple of important steps remaining:

• Removal of the OpenGL code from the kiva.agg package
• Testing!

[corranwebster]

Had a quick look at this - the approach seems generally valid, but it's a bit hard to review because it's hard to see what has been moved, what has been copied, and what is new. I'll need to set aside some time to map that out.

[jwiggins]

the approach seems generally valid, but it's a bit hard to review because it's hard to see what has been moved, what has been copied, and what is new.

• Everything in kiva/gl/src/agg is copied from the Agg source in kiva/agg/agg-24
• Everything else in kiva/gl/src was either moved or copied from kiva/add/src (the core C++ bits of the SWIG wrapper)
• kiva/gl/src/swig is also mostly just copies from kiva/agg/src. I would try diffing them to see the differences (if there are any)

The name of the game here is basically: copy all the code from the kiva.agg extension which was shared with the OpenGL code before inside kiva.agg. Then remove all the GL code from kiva.agg

[corranwebster]

OK, I think I have this running correctly. For concreteness, I am using this branch merged into the #514 branch. Running KivaExplorer with ETS_TOOLKIT=qt4.gl I get an error with font discovery:

'NoneType' object has no attribute 'findfont'

and a weird red background to the image:

This is compared to what I get with ETS_TOOLKIT=qt4:

[corranwebster]

I am also getting failures for Quartz backend:

Traceback (most recent call last):
  File "/Users/cwebster/.edm/envs/enable-test-3.6-pyqt5/lib/python3.6/site-packages/enable/qt4/base_window.py", line 212, in paintEvent
    self.handler.paintEvent(event)
  File "/Users/cwebster/.edm/envs/enable-test-3.6-pyqt5/lib/python3.6/site-packages/enable/qt4/base_window.py", line 64, in paintEvent
    self._enable_window._paint(event)
  File "/Users/cwebster/.edm/envs/enable-test-3.6-pyqt5/lib/python3.6/site-packages/enable/abstract_window.py", line 457, in _paint
    self._gc = self._create_gc(size)
  File "/Users/cwebster/.edm/envs/enable-test-3.6-pyqt5/lib/python3.6/site-packages/enable/qt4/quartz.py", line 86, in _create_gc
    self.dc = get_mac_context(self.control.winId())
ValueError: get_mac_context() requires a pointer to an NSView.
Abort trap: 6

but I have to check if that is independent of this.

[jwiggins]

Looks like #488 caused some breakage in the GL code... With an ugly workaround, I can get it to render:

I'll see how hard it is to fix in a less ugly way.

[jwiggins]

I am also getting failures for Quartz backend

Open an issue. It's not related here.

[corranwebster]

I am also getting failures for Quartz backend

Open an issue. It's not related here.

Done.

[jwiggins]

OK. Try again now that I've fixed the font issue.

[corranwebster]

OK - it is working better. No font rotation which implies something isn't right with the affine transform code. I'll play with some examples to see if it is just fonts.

I still see the red sometimes: this morning I was not plugged into my external monitor and was working on my retina laptop display. On my external monitor I see the image above, on my laptop monitor I see this instead:

This feels like it may be an issue with the Enable qt backend GL window class rather than this PR.

I am also seeing a lot of flicker when I resize the window like it is painting white then painting the widget.

[jwiggins]

I can basically guarantee that the GL backend already had these drawing issues.

It looks like CI is segfaulting consistently when using PyQt5. We should probably disable the GL tests there and split out a new issue.

[jwiggins]

I can basically guarantee that the GL backend already had these drawing issues

GL is broken in the master branch. If you apply my FakePygletContext fix from this branch it works again. The flicker is not present, but everything else is.

[corranwebster]

Yep - it looks as if it is just text which isn't following the rotation of the transform.

I'm guessing by the blocky nature of the blue square that it doesn't support drawing with line joins.

So I think this is OK in that it is no worse than what we currently have; which is perfectly fine for a refactor.

I agree that we should disable the pyqt5 tests for GL and open issues. There is a bigger question about if there are so many missing features with the GL backend is it worth keeping, or should we drop it?

It is fast, though.

[corranwebster]

Same flickering issue for Pyside2 - even more feels like an issue with the Enable Qt GL window class.

[corranwebster]

Much smoother on pyqt4; still a little flickering.

[jwiggins]

OK. The only way I've been able to get rid of the test segfaults is by skipping the QPainter tests when running on Linux with Qt5. That's not great, and I'm currently a bit puzzled how the new GL code should be causing problems here.

Looking into the drawing issues with Qt5 on macOS, I see a bit of confusion about HiDPI displays. In fact, the GL drawing tests fail there because the context is being resized for the higher res screen, but it appears to be happening in the Pyglet code somewhere. I'll open an issue to track.

All of that said, this PR is ready for reviews with an eye to actually merging the code.

[jwiggins]

Just shouting into the void here, but here's what I can get out of gdb when looking at the PyQt5-related segfault in the unit tests:

#0  0x0000000500000001 in ?? ()
#1  0x00007f7f4413651b in hb_font_set_funcs ()
   from /usr/lib/x86_64-linux-gnu/libharfbuzz.so.0
#2  0x00007f7f43c76951 in hb_font_create ()
   from /home/john/.edm/envs/enable-test-3.6-pyqt5/bin/../lib/../lib/libharfbuzz.so
#3  0x00007f7f3e4aad87 in ?? ()
   from /home/john/.edm/envs/enable-test-3.6-pyqt5/lib/python3.6/site-packages/PyQt5/../../../../lib/libQt5Gui.so.5
#4  0x000000000358c180 in ?? ()
#5  0x00000000035e90c0 in ?? ()
#6  0x0000000000000000 in ?? ()

Tested on Ubuntu 18.04

[kitchoi]

You are not shouting to the void :) I am trying to map out the ten thousand line of diffs here and I found some unexplained changes. I think I should batch them instead of spamming the thread though. I am still working on it.

[jwiggins]

In kiva_gl_graphics_state.h, the font_type is not defined in the kiva_gl namespace. The SWIG code for kiva.gl.font_type still refers to the kiva::font_type.

Good catch. I've corrected this and am also checking to see if it fixes the QPainter segfault.

Why is the numpy.i added?

This is basically the same file as kiva/agg/src/numeric.i but with the Numeric compatibility stripped out. Numpy has been around for 15 years, so this should be OK. 😉

Should GraphicsContext.save raise an error or log a warning instead of doing nothing silently?

Yes, it would be nice if it did.

[jwiggins]

checking to see if it fixes the QPainter segfault.

It did not.

[kitchoi]

Thank you @jwiggins for the update, and the latest commits (they look like the original commits from the beginning). The changes make sense. The only lingering issue is the segfault on the two text related tests for QPainter on Linux. I don't really know enough about the objectives of those QPainter tests to tell if the failures represent genuine issues we should worry about and what kind of impact downstream it would be. Apart from that, the rest looks good to me.

[rahulporuri]

FTR, looks like chaco is happy with the changes introduced in this PR - enthought/chaco#561. I verified in the CI job logs that enable was in fact installed from source (enable-4.8.1.dev145).

Testing against another, larger, internal project next. Note that the changes in chaco were not tested on OSX though. UPDATE : The internal project is also happy with the changes in this PR.

[rahulporuri]

LGTM.

To be frank, I couldn't follow most (if not all) of the code changes - but in terms of functionality, i'm happy that the chaco testsuite and an internal project testsuite are both happy with the changes introduced in this PR. Given that, i'm okay with this being merged.

@jwiggins I'm not sure if there are improvements that can be made to the gl/agg backends beyond this PR - or if there are issues that weren't resolved in this PR which we want to address later on (like the QPainter tests segfaulting). Please consider opening additional issues as necessary to track any work that is needed in the future.

Finally, given the weight of changes made in this PR, it is probably best if we make a release candidate i.e. 5.0.0.rc1 (which should be shipped via pypi and edm) and then give our users a week or two to raise concerns before making the final 5.0.0 release.

[jwiggins]

Big thanks to @corranwebster, @kitchoi, and @rahulporuri for the reviews and feedback here. Let's keep moving.