py-pdf / fpdf2

Simple PDF generation for Python
https://py-pdf.github.io/fpdf2/
GNU Lesser General Public License v3.0
993 stars 227 forks source link

Add FPDF.bezier() method exposing PaintedPath.curve_to #1174

Closed awmc000 closed 1 month ago

awmc000 commented 1 month ago

Begins implementing #496

Checklist:

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 commented 1 month ago

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.

Lucas-C commented 1 month ago

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.

allcontributors[bot] commented 1 month ago

@Lucas-C

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

gmischler commented 1 month ago

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.

awmc000 commented 1 month ago

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 commented 1 month ago

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?).

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 commented 1 month ago

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.

gmischler commented 1 month ago

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 commented 1 month ago

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.

awmc000 commented 1 month ago

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

gmischler commented 1 month ago

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...

awmc000 commented 1 month ago

Thanks for catching that.

gmischler commented 1 month ago

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

gmischler commented 1 month ago

Thanks for this helpful feature addition, @awmc000 !

merging now.

Lucas-C commented 1 week ago

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 commented 1 week ago

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