Create an independent renderer for draw_marker_at_points #724

jwiggins · 2021-03-16T16:23:38Z

Closes #708

This was surprisingly straightforward. Basically, since marker rendering is a much more primitive form of rendering, it's not too bad to just copy the subset of AGG which implements it.

As was the case with #392, there's a bunch of code from AGG here which is just copied in. Well, copied and given a new agg24markers namespace so that there's no danger of symbol collisions. (also LF line endings and stripped whitespace) You can basically ignore it for the purpose of reviewing and concentrate on the Python and Cython code.

I've added draw_marker_at_points to the celiagg backend to show what an implementation might look like.

aaronayres35

LGTM

I ran the benchmark and the celiagg draw_marker_at_points results look correct.
I did not look through most of the changes line by line as you suggested (skipped bulk of the .h files). I looked through celiagg implementation, test, etc. and everything made sense. I walked through the cython code and it seemed to all make sense, but given my limited exposure it might be good to get another pair of eyes on those changes specifically

kiva/_marker_renderer.pxd

jwiggins · 2021-03-17T08:35:25Z

kiva/markers/marker_renderer.h

+// (C) Copyright 2005-2021 Enthought, Inc., Austin, TX
+// All rights reserved.
+//
+// This software is provided without warranty under the terms of the BSD
+// license included in LICENSE.txt and may be redistributed only under
+// the conditions described in the aforementioned license. The license
+// is also available online at http://www.enthought.com/licenses/BSD.txt
+//
+// Thanks for using Enthought open source!
+#ifndef KIVA_MARKER_RENDERER_H
+#define KIVA_MARKER_RENDERER_H


This file is new code. Only the code in kiva/markers/agg is copied.

jwiggins

There are several additional markers methods in AGG that we could use to speed up various code paths in chaco's scatterplot (size and color variations). With the marker rendering existing independently of the kiva backend, we can even entertain such an idea...

jwiggins · 2021-03-17T08:37:30Z

kiva/markers/agg/agg_renderer_markers.h

+        template<class T>
+        void markers(int n, const T* x, const T* y, const T* r, marker_e type)


Using this method would give us a draw_marker_at_points where size could be an array (it's r in the signature).

jwiggins · 2021-03-17T08:39:00Z

kiva/markers/agg/agg_renderer_markers.h

+        template<class T>
+        void markers(int n, const T* x, const T* y, const T* r, const color_type* fc, marker_e type)


And this version would give per-marker fill color selection (in addition to per-marker sizes).

jwiggins · 2021-03-17T08:39:21Z

kiva/markers/agg/agg_renderer_markers.h

+        template<class T>
+        void markers(int n, const T* x, const T* y, const T* r, const color_type* fc, const color_type* lc, marker_e type)


Variable fill and stroke colors.

corranwebster · 2021-03-17T08:50:48Z

Absolute +1 for the ability to provide arrays of sizes, markers shapes and colors. That would permit huge simplification of scatterplot/cmap_scatterplot code and potentially speed things up as well, since currently differences in color/marker/size mean rendering calls in Python loops.

kiva/_marker_renderer.pyx

jwiggins · 2021-03-17T08:47:36Z

kiva/_marker_renderer.pyx

+cdef class MarkerRendererABGR32(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_abgr32_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )
+
+cdef class MarkerRendererARGB32(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_argb32_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )
+
+cdef class MarkerRendererBGRA32(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_bgra32_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )
+
+cdef class MarkerRendererRGBA32(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_rgba32_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )
+
+cdef class MarkerRendererBGR24(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_bgr24_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )
+
+cdef class MarkerRendererRGB24(MarkerRendererBase):
+    def __cinit__(self, uint8_t[:,:,::1] image, bottom_up=True):
+        self.base_init(image)
+        self._this = <renderer_base_t*> new renderer_rgb24_t(
+            &image[0][0][0], image.shape[1], image.shape[0], image.strides[0], bottom_up
+        )


You think there's a nicer way to do these specializations?

has no understanding whatsoever about templates/specializations so cannot provide constructive comments Should we ask internally if anyone knows better?

There are options: https://martinralbrecht.wordpress.com/2017/07/23/adventures-in-cython-templating/

jwiggins · 2021-03-17T08:50:00Z

kiva/celiagg.py

+        # Grab the fill and stroke colors (where possible)
+        fill = (0.0, 0.0, 0.0, 0.0)
+        stroke = (0.0, 0.0, 0.0, 1.0)
+        if isinstance(self.fill_paint, agg.SolidPaint):
+            fp = self.fill_paint
+            fill = (fp.r, fp.g, fp.b, fp.a)
+        if isinstance(self.stroke_paint, agg.SolidPaint):
+            sp = self.stroke_paint
+            stroke = (sp.r, sp.g, sp.b, sp.a)


Maybe it would be better to add set_stroke_color and set_fill_color methods to MarkerRenderer* so that we can just pass through colors from GraphicsContext.set_*_color methods? This particular block of code is just here in case a gradient fill color was set.

i'm not sure i fully understand what you're recommending here.

Add set_*_color methods to MarkerRenderer, then in the set_*_color methods of GraphicsContext call self.marker_gc.set_*_color(color) so that the marker renderer stays in sync with the simple fill and stroke colors. If you set a gradient on the celiagg backend and then call draw_marker_at_points, the fill color of the markers will be transparent. One could argue that it's a bug in the code doing the drawing, but it's a silent failure.

I won't doing anything about it in this PR

kiva/tests/test_marker_rendering.py

jwiggins · 2021-03-17T08:54:06Z

Something I left out here: I didn't commit the 30k lines of Cython-generated C++ code like what was done for kiva._cython_speedups. We don't do that for kiva.quartz either, but I know there's been discussion about that in the past.

kiva/celiagg.py

corranwebster · 2021-03-17T09:19:56Z

With the code copied from Agg, was this from the version in Kiva, or from the updated non-GPL Agg repo?

jwiggins · 2021-03-17T09:31:31Z

was this from the version in Kiva, or from the updated non-GPL Agg repo?

From Kiva, but we updated that version from the upstream SVN repo in #288

corranwebster · 2021-03-17T09:42:10Z

QPainter prefers premultiplied ARGB32 for its internal buffer format, I think - is that going to make using this for the QPainter backend difficult? Don't have the insight into Agg's internal buffer formats to know if there is a mismatch here.

jwiggins · 2021-03-17T09:54:48Z

is that going to make using this for the QPainter backend difficult?

AGG has pixel formats for everything, basically:

enable/kiva/markers/agg/agg_pixfmt_rgba.h

Lines 2750 to 2753 in cd06311

    
           typedef pixfmt_alpha_blend_rgba<blender_rgba32_pre, rendering_buffer> pixfmt_rgba32_pre; 
        
           typedef pixfmt_alpha_blend_rgba<blender_argb32_pre, rendering_buffer> pixfmt_argb32_pre; 
        
           typedef pixfmt_alpha_blend_rgba<blender_abgr32_pre, rendering_buffer> pixfmt_abgr32_pre; 
        
           typedef pixfmt_alpha_blend_rgba<blender_bgra32_pre, rendering_buffer> pixfmt_bgra32_pre;

We can make more versions of MarkerRenderer as needed. I believe Quartz uses premultiplied alpha as well.

rahulporuri

Can you add kiva/_marker_renderer.cpp to the git ignore list given that we don't want to include it in the git repo?

rahulporuri

LGTM with a couple of comments/questions (some nitpicky). I too ran the benchmark and things looked good on windows.

kiva/_marker_renderer.pxd

kiva/_marker_renderer.pyx

kiva/celiagg.py

setup.py

rahulporuri · 2021-03-18T08:10:03Z

Also, I don't think we want to implement draw_marker_at_points for any of the other backends - but it'll probably be useful for aaron (or someone new) to implement the method for new backend to get comfortable with the code. We definitely need an issue to track this work though.

jwiggins · 2021-03-18T08:41:08Z

Also, I don't think we want to implement draw_marker_at_points for any of the other backends

You mean in this PR, right? No complaints from my side there.

Adding this to an AGG backend is fairly trivial because we're already in control of the backing store there. It might be a bit more challenging for the Quartz and QPainter backends.

rahulporuri · 2021-03-18T09:09:18Z

I'm testing this branch (and enable main branch) against an internal application - and there's one problem i'm running into when I use the celiagg backend.

The internal application uses the MarkerTrait -

enable/enable/markers.py

Lines 486 to 489 in 2ce5387

    
           # A mapped trait that allows string naming of marker classes. 
        
           MarkerTrait = Trait( 
        
               "square", MarkerNameDict, editor=EnumEditor(values=marker_names) 
        
           )

- and when I try to use the following markers, i don't see anything in the plot - left_triangle, right_triangle, pentagon, hexagon, hexagon2, star, cross_plus.

rahulporuri · 2021-03-18T09:11:47Z

Also, there are a ton of log messages now from fontTools, most of them of the form -

2021-03-18 14:38:11,681 DEBUG    [fontTools.ttLib.ttFont:381] Reading 'name' table from disk
2021-03-18 14:38:11,681 DEBUG    [fontTools.ttLib.ttFont:390] Decompiling 'name' table

corranwebster · 2021-03-18T09:20:31Z

when I try to use the following markers, i don't see anything in the plot - left_triangle, right_triangle, pentagon, hexagon, hexagon2, star, cross_plus.

These markers don't have native agg versions, and so are rendered by paths only; so draw_marker_at_points doesn't work with them (in their definition you will see that they have a kiva_marker value of NO_MARKER)

rahulporuri · 2021-03-18T09:23:16Z

These markers don't have native agg versions, and so are rendered by paths only; so draw_marker_at_points doesn't work with them (in their definition you will see that they have a kiva_marker value of NO_MARKER)

But this worked fine with the agg backend, which is why im slightly concerned

corranwebster · 2021-03-18T09:32:34Z

corranwebster · 2021-03-18T10:17:46Z

For the record here, issue was that Chaco code expects draw_marker_at_points to return False or 0 when it can't draw because it was passed NO_MARKER.

jwiggins · 2021-03-18T10:49:55Z

Thanks for all the great feedback and testing. This should be good to go once CI is happy and y'all have taken a look at my new changes.

kiva/api.py

kiva/_marker_renderer.pyx

kiva/markers/marker_renderer.h

corranwebster · 2021-03-18T13:55:22Z

Not going to have enough time to do this myself today, but if we haven't we should make sure that the various Chaco scatterplot types work with this as expected. They are the primary users.

For fun it might be also worth comparing how fast rendering large numbers of "pixel" markers is on agg and celiagg. Based on really old numbers I would expect ~100k to 1m points to be reasonably performant.

rahulporuri

Create an independent renderer for draw_marker_at_points

cd06311

jwiggins requested review from rahulporuri, corranwebster and aaronayres35 and removed request for rahulporuri and corranwebster March 16, 2021 16:23

aaronayres35 approved these changes Mar 16, 2021

View reviewed changes

kiva/_marker_renderer.pxd Show resolved Hide resolved

jwiggins commented Mar 17, 2021

View reviewed changes

corranwebster reviewed Mar 17, 2021

View reviewed changes

kiva/celiagg.py Outdated Show resolved Hide resolved

rahulporuri reviewed Mar 17, 2021

View reviewed changes

rahulporuri approved these changes Mar 18, 2021

View reviewed changes

kiva/_marker_renderer.pxd Outdated Show resolved Hide resolved

kiva/_marker_renderer.pyx Show resolved Hide resolved

kiva/_marker_renderer.pyx Outdated Show resolved Hide resolved

kiva/celiagg.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

PR feedback

50060ba

jwiggins requested review from rahulporuri and corranwebster March 18, 2021 10:48

jwiggins commented Mar 18, 2021

View reviewed changes

kiva/api.py Outdated Show resolved Hide resolved

rahulporuri reviewed Mar 18, 2021

View reviewed changes

kiva/_marker_renderer.pyx Outdated Show resolved Hide resolved

kiva/_marker_renderer.pyx Outdated Show resolved Hide resolved

kiva/_marker_renderer.pyx Outdated Show resolved Hide resolved

kiva/markers/marker_renderer.h Outdated Show resolved Hide resolved

More PR feedback. transform() arguments

ef66a0a

rahulporuri approved these changes Mar 18, 2021

View reviewed changes

jwiggins merged commit 9f201e2 into master Mar 18, 2021

jwiggins deleted the feature/independent-marks branch March 18, 2021 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an independent renderer for draw_marker_at_points #724

Create an independent renderer for draw_marker_at_points #724

jwiggins commented Mar 16, 2021

aaronayres35 left a comment

jwiggins Mar 17, 2021

jwiggins left a comment

jwiggins Mar 17, 2021

jwiggins Mar 17, 2021

jwiggins Mar 17, 2021

corranwebster commented Mar 17, 2021

jwiggins Mar 17, 2021

rahulporuri Mar 18, 2021

jwiggins Mar 18, 2021

jwiggins Mar 17, 2021

rahulporuri Mar 18, 2021

jwiggins Mar 18, 2021

jwiggins Mar 18, 2021

jwiggins commented Mar 17, 2021

corranwebster commented Mar 17, 2021

jwiggins commented Mar 17, 2021

corranwebster commented Mar 17, 2021

jwiggins commented Mar 17, 2021 •

edited

Loading

rahulporuri left a comment

rahulporuri left a comment •

edited

Loading

rahulporuri commented Mar 18, 2021

jwiggins commented Mar 18, 2021

rahulporuri commented Mar 18, 2021

rahulporuri commented Mar 18, 2021

corranwebster commented Mar 18, 2021 •

edited

Loading

rahulporuri commented Mar 18, 2021

corranwebster commented Mar 18, 2021 •

edited

Loading

corranwebster commented Mar 18, 2021

jwiggins commented Mar 18, 2021

corranwebster commented Mar 18, 2021

rahulporuri left a comment

		template<class T>
		void markers(int n, const T* x, const T* y, const T* r, marker_e type)

Create an independent renderer for draw_marker_at_points #724

Create an independent renderer for draw_marker_at_points #724

Conversation

jwiggins commented Mar 16, 2021

aaronayres35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwiggins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corranwebster commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwiggins commented Mar 17, 2021

corranwebster commented Mar 17, 2021

jwiggins commented Mar 17, 2021

corranwebster commented Mar 17, 2021

jwiggins commented Mar 17, 2021 • edited Loading

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri left a comment • edited Loading

Choose a reason for hiding this comment

rahulporuri commented Mar 18, 2021

jwiggins commented Mar 18, 2021

rahulporuri commented Mar 18, 2021

rahulporuri commented Mar 18, 2021

corranwebster commented Mar 18, 2021 • edited Loading

rahulporuri commented Mar 18, 2021

corranwebster commented Mar 18, 2021 • edited Loading

corranwebster commented Mar 18, 2021

jwiggins commented Mar 18, 2021

corranwebster commented Mar 18, 2021

rahulporuri left a comment

Choose a reason for hiding this comment

jwiggins commented Mar 17, 2021 •

edited

Loading

rahulporuri left a comment •

edited

Loading

corranwebster commented Mar 18, 2021 •

edited

Loading

corranwebster commented Mar 18, 2021 •

edited

Loading