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

Fix artefact on line junctions when calling arc() #492

Merged
merged 4 commits into from
Aug 17, 2022

Conversation

Jmillan-Dev
Copy link

@Jmillan-Dev Jmillan-Dev commented Aug 17, 2022

Fixes #488

By using the operator h when both start_from_center and end_at_center are True fixes the rendering issue.

This closes the path of the shape instead of leaving it open like l did before.

Tests have been added to check this new behavior.

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

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

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

Use operator `h` when both `start_from_center` and `end_at_center` are
True otherwise use the previous operator (`l`).
@Lucas-C
Copy link
Member

Lucas-C commented Aug 17, 2022

@all-contributors please add @Jmillan-Dev for code

@allcontributors
Copy link

@Lucas-C

I've put up a pull request to add @Jmillan-Dev! 🎉

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Looks great!
Thank you for contributing 😊

CHANGELOG.md Outdated Show resolved Hide resolved
fpdf/fpdf.py Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Aug 17, 2022

Seems like some unit tests are now failing:

FAILED test/shapes/test_arc.py::test_arc_start_from_center_and_end_at_center
FAILED test/shapes/test_solid_arc.py::test_solid_arc_not_circle
FAILED test/shapes/test_solid_arc.py::test_solid_arc_style
FAILED test/shapes/test_solid_arc.py::test_solid_arc_line_width
FAILED test/shapes/test_solid_arc.py::test_solid_arc_draw_color
FAILED test/shapes/test_solid_arc.py::test_solid_arc_fill_color
FAILED test/shapes/test_solid_arc.py::test_solid_arc_inclination
FAILED test/shapes/test_solid_arc.py::test_arc_clockwise

Can you reproduce this on your computer with pytest -vv test/shapes/test_solid_arc.py?

I you check the source code of the solid_arc() method you'll see that it always calls arc() with start_from_center=True and end_at_center=True. Hence you will have to regenerate all reference PDF files of test functions in test/shapes/test_solid_arc.py by passing generate=True once to all calls to assert_pdf_equal(), and then include the newly generated PDF files to this PR.

If my explanations are not very clear to you, feel free to ask any questions and I'll try to make things clearer 😊

@Jmillan-Dev
Copy link
Author

Okay, I forget to change the new test PDF file for test_arc_start_from_center_and_end_at_center when I did a last minute change when I read that the operator h did not required any arguments.

And I now have updated all of the solid_arc() test files since they use arc(start_from_center=True, end_at_center=True)

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #492 (3f4f379) into master (7d98f98) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
- Coverage   92.11%   91.96%   -0.16%     
==========================================
  Files          23       23              
  Lines        6682     6555     -127     
  Branches     1365     1333      -32     
==========================================
- Hits         6155     6028     -127     
  Misses        299      299              
  Partials      228      228              
Impacted Files Coverage Δ
fpdf/fpdf.py 89.87% <100.00%> (ø)
fpdf/prefs.py 90.90% <0.00%> (-1.95%) ⬇️
fpdf/drawing.py 92.73% <0.00%> (-0.28%) ⬇️
fpdf/syntax.py 95.37% <0.00%> (-0.05%) ⬇️
fpdf/sign.py 100.00% <0.00%> (ø)
fpdf/enums.py 100.00% <0.00%> (ø)
fpdf/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Lucas-C
Copy link
Member

Lucas-C commented Aug 17, 2022

This looks great! Merging now

@Lucas-C Lucas-C merged commit 658fcb3 into py-pdf:master Aug 17, 2022
@Jmillan-Dev Jmillan-Dev deleted the fix-arcs-artefacts branch August 17, 2022 13:09
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.

Improve arc rendering
2 participants