fpdf2 icon indicating copy to clipboard operation
fpdf2 copied to clipboard

Add FPDF.bezier() method exposing PaintedPath.curve_to

Open awmc000 opened this issue 1 year ago • 1 comments

Begins implementing #496

Checklist:

  • [x] The GitHub pipeline is OK (green), meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • [x] A unit test is covering the code added / modified by this PR

  • [x] This PR is ready to be merged

  • [x] In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • [x] A mention of the change is present in CHANGELOG.md

I do think this could be added as is, but since it only supports cubic Bezier curves in its current state, maybe others would disagree and say this should support any number of control points? That is my next goal.

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

awmc000 avatar May 17 '24 21:05 awmc000

Thank you for the helpful feedback. I'll sort out the issues you mentioned and extend it to support point lists of 3 or 4 points, closed/open paths, and either type of Bezier curves.

awmc000 avatar May 21 '24 01:05 awmc000

Thank you for the helpful feedback. I'll sort out the issues you mentioned and extend it to support point lists of 3 or 4 points, closed/open paths, and either type of Bezier curves.

Thank YOU for taking the time to contribute to fpdf2! 🙂

@allcontributors please add @awmc000 for code.

Lucas-C avatar May 24 '24 08:05 Lucas-C

@Lucas-C

I've put up a pull request to add @awmc000! :tada:

allcontributors[bot] avatar May 24 '24 08:05 allcontributors[bot]

Please check the pylint output and fix any issues it brings up. You'll also need to rebase your branch, and resolve any conflicts that brings up.

And then I have another wishlist item... The other shape methods all have a style parameter, which accepts an enum.RenderStyle (or a string), and controls whether to draw the outline of the shape, its area fill, or both. In the drawing module, this seems to translate into the paint_rule field of the GraphicsStyle class, which gets populated with an enum.PathPaintRule value. I'm not sure if we have an example yet on how this translation is best done. This detail is (hopefully) the last one that is necessary to make your new feature actually useable.

gmischler avatar May 28 '24 07:05 gmischler

OK, got it, so I will figure out how to convert a RenderStyle to a PathPaintRule, so that the latter represents the same settings as the former does? And thanks for reminding me about Pylint.

awmc000 avatar May 29 '24 00:05 awmc000

It looks like the values for RenderStyle and PathPaintRule correspond but just have different names, and that PathPaintRule supports some behaviour RenderStyle doesn't, namely FILL_EVENODD, STROKE_FILL_EVENODD, DONT_PAINT (and AUTO checks PaintedPath.style?).

  • DRAW corresponds to STROKE = "S"
  • FILL to FILL_NONZERO = "F"
  • DRAW_FILL to STROKE_FILL_NONZERO = "B"

So it looks what I need to do is select the PathPaintRule enum value STROKE if the style is D, FILL_NONZERO if the style is F and STROKE_FILL_NONZERO if the style is DF. Does this all sound right?

awmc000 avatar May 29 '24 00:05 awmc000

I tested that and it seems to work for a basic test in the shell.

>>> from fpdf import FPDF
>>> pdf = FPDF()
>>> pdf.add_page()
>>> pdf.bezier([(20, 20), (20, 10), (30, 30)], style="FD")
>>> pdf.set_fill_color((200, 5, 5))
>>> pdf.bezier([(20, 120), (20, 110), (30, 130)], style="F")
>>> pdf.bezier([(20, 220), (20, 210), (30, 230)], style="D")
>>> pdf.output("stylescr2.pdf")

I get a curve with a fill and stroke, one with just a fill, and one with just the stroke.

awmc000 avatar May 29 '24 00:05 awmc000

I get a curve with a fill and stroke, one with just a fill, and one with just the stroke.

Looks great! This appears to be pretty much complete now.

There's yet another thing you might want to test, though. I think that PaintedPath() automatically picks up the current settings of FPDF.line_width and FPDF.dash_pattern (just like it does with draw_color and fill_color). But it would be good to verify that with an explicit test, so that we know for certain that the new method behaves exactly like the other shapes in all relevant aspects. This will also help prevent regressions during future changes to the code.

Oh, and you should remove any generate=True from your tests before checking in. It currently prevents the tests from running through in the pipeline.

gmischler avatar May 29 '24 06:05 gmischler

Two last little nitpicks:

you need to run black on your code, it blocks the testing pipeline.

And I just noticed that you still have a debug_stream=None parameter in the code. While it may be helpful during development that the drawing module offers this option, I don't think that it should be exposed in the public API.

gmischler avatar May 29 '24 20:05 gmischler

Should I be getting a debug_stream from somewhere else? Or just remove it? (Removed for now.)

awmc000 avatar May 29 '24 21:05 awmc000

Should I be getting a debug_stream from somewhere else? Or just remove it? (Removed for now.)

Removing it is fine, as long as you do it in the docstring as well...

gmischler avatar May 29 '24 21:05 gmischler

Thanks for catching that.

awmc000 avatar May 29 '24 22:05 awmc000

There's still an unresolved conflict in CHANGELOG.md (even if github doesn't seem to detect it).

gmischler avatar May 29 '24 22:05 gmischler

Thanks for this helpful feature addition, @awmc000 !

merging now.

gmischler avatar May 30 '24 04:05 gmischler

Hi 🙂

Thank you for this contribution @awmc000 !👍

I have just been testing this new feature today, and I realized that there is minor discrepancy between the code and the docstring: https://py-pdf.github.io/fpdf2/fpdf/fpdf.html#fpdf.fpdf.FPDF.bezier

style (fpdf.enums.RenderStyle, str): Optional style of rendering. Allowed values are:
* `D` or None: draw border. This is the default value.

Currently the actual default behaviour seems to be "F", when no style is provided.

I think we should stick with "D" being the default bejaviour, to be consistent with the other methods.

What do you think @gmischler & @awmc000?

Lucas-C avatar Jun 18 '24 07:06 Lucas-C

I used the opportunity of PR https://github.com/py-pdf/fpdf2/pull/1210 to fix this 🙂

Lucas-C avatar Jun 18 '24 17:06 Lucas-C