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.

Sign up for GitHub

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

Jump to bottom

Rewritten printing logic and intelligent corner-filling #19

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Rewritten printing logic and intelligent corner-filling #19

wants to merge 4 commits into from

Conversation

AdamBajger
Copy link

I have rewritten the printing logic and few other things along the way:

  • I made the cache directory from a global variable (ew...) into an argument to each function that needs to use it.
  • I created abstract class for assembling the card grid and lines so that it is readable, simple to understand and flexible. I have then reimplemented the FPDF2 and Matplotlib printers by subclassing the abstract class.
  • I got rid of the matplotlib wrapper (why complicate stuff)
  • I have added a file with proper measures for the card sizes and paper sizes in different units. I tried my best to make the CLI unit agnostic as far as the support goes for the printing libraries. The units must be specified, though.
  • I have moved files a bit in test directory and added resources for offline tetsing of layouts
  • I have implemeted an inteligent method for filling in corners - code should be self explanatory if you know what gaussian blur and min/max filtering is (otherwise, read the docstring and comments)
  • I have introduced PDM and added configurations for it so that it automatically formats your code using all the fancy things you did, but using just Ruff (it supports it all).
  • I have changed the fixtures in tests and moved some of them into the conftest.py
  • I have also improved the deck-parsing code so that it does not break when user has "x" in the card numbers as per "4x Ajani Goldmane (M11) 1" which is quite frequent.

So, round and round, not much has changed - just a small rewrite to make it easier to add a new feature.

AdamBajger and others added 2 commits May 9, 2024 00:53
@AdamBajger
ref: rewrite for flexibility and readability (#1)
ccbdf5f 
* feat: restructure, update

- update from setup.py to pyproject.toml
- use pdm
- add support for "x" after the numbers in the decklists

* feat: add pre-set sizes for papers

- add a dedicated module for sizes and dimensions
- add nptyping for better typehints

* refactor: rewrite for flexibility and readability

- remove pdf case from matplotlib function, since PDF output is handled by a separate method
- add cache CLI argument for the following cache refactor
- fix methods being shadowed by their local variables
- replace Argparse with CLick
- rewrite dimensions for better accessibility
- decompose printing methods into classes for better extensibility

* refactor: refactor caching and add few tests

- modify cache location
- add cache argument to each function that uses it to remove cache definition through side effects (globals are 'ew'!)
- add some tests for units and dimensions

* fix: patch errors from cache refactor

- add conftest for fixtures
- fix some broken tests
-

* fix: patch errors from cache refactor and plotting

- add conftest for fixtures
- fix broken tests
- remove obsolete tests
- change str -> Path where applicable
- fix matplotlib plotting and FPDF2 printing issues, including issues rooted in the abstract base class.

* feat: update pre-commit hooks and add example images

- use the faster better RUFF instead of older things.
- add conventional commits hook to encourage readable reflogs
- add example card images for testing and offline development

* ref: update python-package.yml

* feat: use PDM in github action

* feat: use PDM in github action

- fix pre-commit dependency and stuff
- remove invalid import
- update README.md
- update deps in pyproject.toml
- remove prettier

* fix: install correct deps in github actions

* fix: use PDM to run tests in github actions

* fix: tests

- use correct fixtures

---------

Co-authored-by: Adam Bajger <[email protected]>
@AdamBajger
Feature to intelligently fill corners (#2)
0f77830 
* feat: Implement corner filling

- fill the corners of the cards using their own edges, allowing filled border for cards of any border color.
- make filled corners for borderless cards look good as well.
- make the border-filling finctions static
- rename CLI functions to generic names
- designate output destination for tests

* refactor: rewrite the corner-filling logic

- since for each corner, the code gets executed similarly, make the methods share code as much as possible
- add docstrings
- fix method name shadowing

* fix: wrong keyword
@AdamBajger AdamBajger marked this pull request as draft May 13, 2024 23:10
@AdamBajger
Copy link
Author

AdamBajger commented May 13, 2024 •
edited
Loading

Shiee, found a bug with cropping. I will need to fix it, but later.
It just turned out that I did not understand the whole "inner" border crop and instead I reimplemented it into an "outer" border crop because I though that was what it was doing and the code just looked too horrible for me to actually understand it, I only realized my misunderstanding now after reading about it in the issues. :D
I will need to go back, evaluate it again and add that feature back again probably.

AdamBajger added 2 commits May 16, 2024 23:33
@AdamBajger
fix: cropping and image type
95208b7 
- crop all sides the same
- change the Image object from numpy.ndarray to PIL.Image
- rework the image operations to use PIL
- add cache and outputs into .gitignore
- fix other minor typing issues
@AdamBajger
fix: add test cache to .gitignore
b804d28
@AdamBajger AdamBajger marked this pull request as ready for review May 16, 2024 21:51
@AdamBajger
Copy link
Author

I examined the "inner border" crop and decided not to attempt to match the behaviour 100%
IMHO, the crop I have implemented works more intuitively by just cropping the image from the outer edges and I don't see any reason to do it differently.

@DiddiZ
Copy link
Owner

DiddiZ commented Jun 21, 2024

Hello, I really appreciate your effort to add this feature. However this PR changes far too many things at once.
Each commit should be self-contained and clearly state what it changed.

Also, please avoid changing the overall structure and linter settings (ruff is amazing, though). These are kept identical to other projects.

@DiddiZ DiddiZ marked this pull request as draft June 21, 2024 11:47
@DiddiZ DiddiZ self-requested a review June 21, 2024 11:47
@AdamBajger
Copy link
Author

Hmm.
I may try to group it into different commits, but the essence of the changes is really a full rewrite of the printing classes.
Delete old functions and replace them with new ones that use inheritance. (and add the corner filling along the way)
There is a lot of changes that I feel were necessary (and benefitial)

With regard to the linter settings. Do you really believe that it needs to stay? I did not see any active projects that would seem to depend on linter settings etc.

@DiddiZ
Copy link
Owner

DiddiZ commented Jun 24, 2024

Yes, not all projects are public. Please leave the DevOps unchanged.

  • Refactoring printing is one commit
  • Corner filling is one commit
  • Paper sizes is one commit
  • Argsparse to click is a separate commit. What is the benefit, why is this necessary?
  • I don't see the point of the cache_dir parameter yet. That adds a lot of clutter and no clear benefit to me. For unittests, the environment variable can be patched.
  • Don't move the scryfall package. It's a candidate to be move to an own package.
  • Moving the examples is one commit
  • Adjusting the deck list parsing regex is one commit, etc.

Several of the changes are completely unrelated to each other and could be separate PRs.
Try to make the changes for each feature minimal and to only touch the relevant parts. That makes the review process much easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiddiZ DiddiZ Awaiting requested review from DiddiZ

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AdamBajger @DiddiZ