Ensure all backends implement `arc_to` and `quad_curve_to` #988

corranwebster · 2022-08-10T09:27:54Z

This consists of:

adding a simple implementation of arc_to for basecore2d.GraphicsContext
removing the implementations of arc_to (which uses cubic beziers) and quad_curve_to (which raises masking the superclass) from the PDF backend. It inherits from basecore2d.GraphicsContext so we just use the implementations from that.
adding tests for the curve methods to the drawing tester mixin.

Note that the PS and SVG backends won't draw eg. a rounded rectangle correctly until #987 is merged.

mdickinson

LGTM; some minor questions and comments.

mdickinson · 2022-08-11T08:13:41Z

kiva/basecore2d.py

+        Consider the common case of rounding a rectangle's upper left corner.
+        Let "r" be the radius of rounding. Let the current point be
+        ``(x_left + r, y_top)``. Then ``(x2, y2)`` would be
+        ``(x_left, y_top - radius)``, and ``(x1, y1)`` would be


Suggested change

``(x_left, y_top - radius)``, and ``(x1, y1)`` would be

``(x_left, y_top - r)``, and ``(x1, y1)`` would be

mdickinson · 2022-08-11T08:21:19Z

kiva/basecore2d.py

+        ``(x1, y1)`` is the imaginary intersection point of the two lines
+        tangent to the arc at the current point and at ``(x2, y2)``.
+
+        If the tangent point on the line from the current point to ``(x1, y1)``


What's the behaviour when all three points (i.e., the current point, (x1, y1) and (x2, y2)) are collinear? Do we simply get a line from the current point to (x1, y1)? Is this corner case worth documenting / testing?

mdickinson · 2022-08-11T08:25:36Z

kiva/basecore2d.py

+        tangent to the arc at the current point and at ``(x2, y2)``.
+
+        If the tangent point on the line from the current point to ``(x1, y1)``
+        is not equal to the current point, a line is drawn to it. Depending on


I assume this is true even in the case where that line is going "backwards"; i.e., in the direction from (x1, y1) towards the current point?

To clarify with an example: I'm thinking about cases like the one where the current point is (1, 0), (x1, y1) is (0, 0), (x2, y2) is (0, 1), and the radius is 2.

mdickinson · 2022-08-11T08:29:30Z

kiva/tests/test_svg_drawing.py

@@ -26,8 +26,11 @@ def draw_and_check(self):
        self.gc.save(filename)
        tree = ElementTree.parse(filename)
        elements = [element for element in tree.iter()]
-        if not len(elements) in [4, 7]:
-            self.fail("The expected number of elements was not found")
+        if not len(elements) in [4, 5, 7]:


Might as well change the unidiomatic if not ... in to the more idiomatic if ... not in while we're touching this code.

mdickinson · 2022-08-11T08:32:15Z

kiva/basecore2d.py

@@ -654,7 +683,7 @@ def _new_subpath(self):
            self.path.append(self.active_subpath)

    # ----------------------------------------------------------------
-    # Getting infomration on paths
+    # Getting information on paths


This typo seems to exist in a bunch of files ...

mdickinson · 2022-08-11T08:39:13Z

Design / naming observation: it's a little distressing that a method called arc_to doesn't necessarily go to either of the points described in its parameters. That seems inconsistent with expectations and with methods like line_to and move_to.

corranwebster · 2022-08-12T09:31:56Z

Exciting times - I decided to add a parameter sweep of arc_to to the benchmarks, and am getting a segfault in the OpenGL backend.

corranwebster · 2022-08-12T10:15:56Z

It looks like there are sufficiently many issues with the corner cases across all backends I've added an issue to investigate and fix: #995

corranwebster · 2022-08-12T10:17:43Z

it's a little distressing that a method called arc_to doesn't necessarily go to either of the points described in its parameters.

On some backends it does, and on others it doesn't

* Ensure all backends implement arc_to and quad_curve_to. * Fix 'infomration' typo in comments in multiple files. * Fixes suggested from PR review.

* Use the "sphinx-copybutton" extension in documentation (#948) * DOC: Use the sphinx-copybutton extension in documentation modified: ci/edmtool.py modified: docs/source/conf.py modified: enable/__init__.py * Update ci/edmtool.py * Get things working on wxPython again (#950) Addresses these warnigns/exceptions: - TypeError: BitmapFromImage() got an unexpected keyword argument 'depth' - AttributeError: 'PaintDC' object has no attribute 'BeginDrawing' - wxPyDeprecationWarning: Call to deprecated item EmptyImage. Use :class:`Image` instead. image = wx.EmptyImage(*sz) - wxPyDeprecationWarning: Call to deprecated item BitmapFromImage. Use :class:`wx.Bitmap` instead bmp = wx.BitmapFromImage(image, depth=-1) See https://forums.wxwidgets.org/viewtopic.php?t=39930 why the Begin/EndDrawing calls can simply be removed. Related issues: #798 and #531 * Add SWIG to pyproject.toml build dependencies (#954) * Experiment with installing SWIG from PyPI * Update the EDM-based workflow * Remove restriction on SWIG version * Add Cython back as a development dependency; remove verify_swig_install * Remove generated C++ file. (#958) * Fix Kiva Quartz backend string encoding (#966) * Add regression test for Enable #964. * Properly decode strings in Kiva Quartz font code. * Properly guard against running Quartz tests on non-Mac systems. * Ensure Wx Quartz test terminates. * Make test success externally visible. * Fix tests, use UTF-8. * Add a github workflow that publishes to sdists to PyPI on release (#967) * Add a github workflow that publishes to sdists to PyPI on release. * Use build to build the sdist. * Take into account all font features when drawing SVG text. (#980) * Fix Quartz font rendering (#978) * Use font family in quartz when no font name given. * Render quartz text with fill color rather than stroke color. * Fix `SCRIPT` font family in Kiva (#975) * Add a test for #971 This should fail CI. * Fix bug and clean-up style. * Correct first point and orientation of Kiva QPainter arcs (#970) * Correct first point and orientation of Kiva QPainter arcs. This fixes #962 but not #960. * Update kiva/qpainter.py * Render font families better in QPainter backend (#973) * Fall back to kiva.fonttools if Qt can't come up with a font name. * Add a comment about setFamilies() * Fix WEIGHT_EXTRAHEAVY on Qt6 (#990) Some changes to tests to make sure eveything is tested correctly. * Fix the rendering of polygons in basecore2d-based backends (#987) * Fix the rendering of polygons in basecore2d-based backends This: - consistently uses Nx2 numpy arrays to store path points - doesn't create a new subpath after a LINES function A driveby fix gives a default implementation for show_text_at_point(). * Clean-up comments. * Fix some unused imports. * Some of the unused imports are in fact used. * Improve the situation of line_state_equal a bit. * Ensure all subpath arguments are numpy arrays. * Ensure all backends implement `arc_to` and `quad_curve_to` (#988) * Ensure all backends implement arc_to and quad_curve_to. * Fix 'infomration' typo in comments in multiple files. * Fixes suggested from PR review. Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com> Co-authored-by: Brecht Machiels <brecht@mos6581.org> Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

Ensure all backends implement arc_to and quad_curve_to.

5208a55

corranwebster mentioned this pull request Aug 10, 2022

Make sure all kiva backends implement AbstractGraphicsContext faithfully #648

Open

12 tasks

mdickinson approved these changes Aug 11, 2022

View reviewed changes

corranwebster added 2 commits August 12, 2022 10:13

Merge branch 'main' into fix/curve-rendering

48b1438

Fix 'infomration' typo in comments in multiple files.

e8721d5

Fixes suggested from PR review.

91219c3

corranwebster merged commit 51a05c3 into main Aug 12, 2022

corranwebster deleted the fix/curve-rendering branch August 12, 2022 10:21

corranwebster added a commit that referenced this pull request Aug 12, 2022

Ensure all backends implement arc_to and quad_curve_to (#988)

aee98e3

* Ensure all backends implement arc_to and quad_curve_to. * Fix 'infomration' typo in comments in multiple files. * Fixes suggested from PR review.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all backends implement `arc_to` and `quad_curve_to` #988

Ensure all backends implement `arc_to` and `quad_curve_to` #988

corranwebster commented Aug 10, 2022

mdickinson left a comment

mdickinson Aug 11, 2022

mdickinson Aug 11, 2022

mdickinson Aug 11, 2022

mdickinson Aug 11, 2022

mdickinson Aug 11, 2022

mdickinson Aug 11, 2022

mdickinson commented Aug 11, 2022

corranwebster commented Aug 12, 2022

corranwebster commented Aug 12, 2022

corranwebster commented Aug 12, 2022

	``(x_left, y_top - radius)``, and ``(x1, y1)`` would be
	``(x_left, y_top - r)``, and ``(x1, y1)`` would be

Ensure all backends implement arc_to and quad_curve_to #988

Ensure all backends implement arc_to and quad_curve_to #988

Conversation

corranwebster commented Aug 10, 2022

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson Aug 11, 2022

Choose a reason for hiding this comment

mdickinson commented Aug 11, 2022

corranwebster commented Aug 12, 2022

corranwebster commented Aug 12, 2022

corranwebster commented Aug 12, 2022

Ensure all backends implement `arc_to` and `quad_curve_to` #988

Ensure all backends implement `arc_to` and `quad_curve_to` #988