canismarko / dungeon-sheets

A tool to create character sheets and GM session notes for Dungeons and Dragons fifth edition (D&D 5e).
https://dungeon-sheets.readthedocs.io/en/latest/
GNU General Public License v3.0
161 stars 66 forks source link

Improved image generation #152

Closed Valentin-Metz closed 8 months ago

Valentin-Metz commented 8 months ago

Hi, I've done some work to improve the character image generation.

I have made the following improvements:

To achieve this I've done the following:

  1. I reworked the image generation:
    • portrait and symbol in the user_character.py file can now be set to a filepath (str)
    • The images generated are centered around an anchor-point where they are allowed to expand up to a maximum value while keeping their aspect ratio
    • Custom images can be inserted with arbitrary location and dimensions at an arbitrary page. For this the user can set the images list to a list of (path_to_image_file, page_index, x_center_coordinate, y_center_coordinate, max_width, max_height)
  2. I've upgraded the tool dependencies:
    • Instead of pdfrw (which has been deprecated since 2017), we now use its successor pypdf (although I have only exchanged actual usage in places where I made relevant changes)
    • This integrates abilities like "merge" and "stamp" with python. No more subprocess calls to external programs.
    • requirements.txt has been modified accordingly

I provide a usage example in examples/bard1.py.

As per the contribution guidelines I've formatted the files with black (as they were not formatted with black before, I've separated the formatting itself into a different commit, so that you can more easily inspect the relevant code changes.

The files I have created fully pass flake8. The files I have modified pass flake8 in all modified lines.

canismarko commented 8 months ago

Thanks for the PR. Since this changes how images are specified, so you expect that existing character .py files with images will still work, or is this a breaking change? I'm not opposed either way, just want to know what to expect.

canismarko commented 8 months ago

CI failure is unrelated to this PR. Let me fix that first and I'll run the tests again.

canismarko commented 8 months ago

@Valentin-Metz Would you please merge in the recent changes from master? The CI is fixed now, so hopefully this PR passes now (or at least fails for real reasons). Thanks.

Valentin-Metz commented 8 months ago

Thanks for the PR. Since this changes how images are specified, so you expect that existing character .py files with images will still work, or is this a breaking change? I'm not opposed either way, just want to know what to expect.

Currently this is a breaking change (instead of the bool, we now expect an str). Python would theoretically allow us to turn this into a "soft-deprecation" by checking for the type of a variable, but that's a very hacky solution. If you want I can implement that though.

canismarko commented 8 months ago

Currently this is a breaking change (instead of the bool, we now expect an str). Python would theoretically allow us to turn this into a "soft-deprecation" by checking for the type of a variable, but that's a very hacky solution. If you want I can implement that though.

No, I don't think that's necessary. I am thinking of making this part of a v1.0 release anyway, which means breaking changes would be expected.

canismarko commented 8 months ago

I think the overall design is fine, so I'm happy to merge once the tests are working.

There was another CI problem, but I'll fix that later, so I just ran the tests and example locally. I had to make two small changes to get the tests to pass: https://gist.github.com/canismarko/40a69b1f2e7d9f74da1cdee6a119e72b

In character.py, it raises an error if "source_file_location" is not in the character attributes. If I make it so that "" is the default, then it's fine.

In make_sheets.py, it cannot import from pdf_image_insert import insert_image_into_pdf unless you're in the source directory. It works if I change it to import from from dungeonsheets.pdf_image_insert.

Are you willing to add these changes to the PR?

Valentin-Metz commented 8 months ago

Done

canismarko commented 8 months ago

I'd like to restore support for older versions of python. Is there a specific feature that this PR depends on that is only available in python >= 3.10?

canismarko commented 8 months ago

Thanks for the PR, @Valentin-Metz.

Valentin-Metz commented 8 months ago

I'd like to restore support for older versions of python. Is there a specific feature that this PR depends on that is only available in python >= 3.10?

Probably just the new (native) type hint system for 3.9. Other than that some of the dependencies don't provide wheels for python versions even older than that.

Instead of maintaining backwards compatibility, it might be a better idea to provide a drop-in Docker image of this repository with all dependencies provided. GitHub supports building and deploying these to an integrated registry. I have some experience with this and will see if I can provide a reference implementation.