Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
May 30, 2024

Conversation

awmc000
Copy link

@awmc000 awmc000 commented May 17, 2024

Begins implementing #496

Checklist:

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

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

  • This PR is ready to be merged

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

  • 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 awmc000 requested a review from gmischler as a code owner May 17, 2024 21:27
@awmc000 awmc000 marked this pull request as draft May 18, 2024 00:31
Copy link
Collaborator

@gmischler gmischler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool new feature, thanks @awmc000!

But as always: We want more! 😁

You seem to have noticed the naming confusion yourself:
The method is called bezier(), internally it creates a quadratic bezier, while the test file is called cubic_bezier_curve.pdf

It would actually be nice if the method could create both bezier types. After all, as far as I can tell, the only API change would be to allow point lists with three or four elements, which you can then use to select the appropriate drawing method (and check if the user has supplied a valid number of points at the same time).

The other surprise is that your code only seems to allow closed paths.
Unfortunately, a lot of the functionality in the drawing module is not explicitly documented. However, if you look at the function open_path_drawing() in test/drawing/test_drawing.py, you'll find that you can create open paths with path.style.auto_close = False.
You could expose that option with a closed=False parameter (or default it to True, if you prefer).

@awmc000
Copy link
Author

awmc000 commented May 21, 2024

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
Copy link
Member

Lucas-C commented May 24, 2024

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.

Copy link

allcontributors bot commented May 24, 2024

@Lucas-C

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

@awmc000 awmc000 marked this pull request as ready for review May 24, 2024 22:57
@gmischler
Copy link
Collaborator

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
Copy link
Author

awmc000 commented May 29, 2024

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
Copy link
Author

awmc000 commented May 29, 2024

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
Copy link
Author

awmc000 commented May 29, 2024

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
Copy link
Collaborator

gmischler commented May 29, 2024

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.

…emove generate=True from Bezier tests; clean up point drawing function for bezier tests.
@gmischler
Copy link
Collaborator

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
Copy link
Author

awmc000 commented May 29, 2024

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

@gmischler
Copy link
Collaborator

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
Copy link
Author

awmc000 commented May 29, 2024

Thanks for catching that.

@gmischler
Copy link
Collaborator

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

@gmischler
Copy link
Collaborator

Thanks for this helpful feature addition, @awmc000 !

merging now.

@gmischler gmischler merged commit f0bd468 into py-pdf:master May 30, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants