Replace all external programs/libraries with Python extensions #87
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With usable prebuilt bindings to ooz decompression at https://pypi.org/project/pyooz/ we can unify decompression across all platforms, removing the need for manual adding of `ooz.exe` or `libooz.dll` to the execution environment. This should make PyPoE more self-contained and has the benefit of being able to decompress without intermediary files on non-Windows platforms.
With the porting of `process-image` from poe-utils to a Python extension at https://pypi.org/project/pydds/ we can now remove all subprocess invocations of both ImageMagick and `process-image`. This change still writes a number of intermediary files to disk and could be optimized to keep them in memory in some circumstances.
zao
changed the title
pm5k
reviewed
Jun 1, 2022
Comment on lines
+1549
to
+1550
| with open(out_path, 'wb') as f: | ||
| f.write(self.file_system.extract_dds(data)) |
There was a problem hiding this comment.
You could import Path from pathlib to make use of Path's built-in context manager to avoid this.
Instead the whole thing could be written as:
Path(out_path).write_text | .write_bytes (self.file_system.extract_dds(data))
Comment on lines
+4128
to
+4129
| img = Image.open(ico) | ||
| img.save(ico) |
There was a problem hiding this comment.
Not familiar with this. Why is the image saved after being just opened?
There was a problem hiding this comment.
The save here is indeed superfluous, I was probably trying to keep the same disk I/O as the original version as I don't know which files are temporary and which ones are expected to be there for further editor work or upload.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Abstract
PyPoE currently has several implicit external dependencies for tasks:
ooz.exe/libooz.dllfor decompressing game data;process-image.exefrom poe-utils for converting and slicing BC7 and other modern DDS pixel formats to PNGs;convert.exe/magick.exefrom ImageMagick for DDS decoding and compositing/coloring of map items and atlas sigils.These must be manually installed and sometimes even built by the end user before some parts of the software are operable and it's not immediately obvious where this is documented.
Action Taken
ooz has been replaced with a Python extension (https://pypi.org/project/pyooz/) built on the same codebase.
poe-utils has been replaced with a Python extension (https://pypi.org/project/pydds/) ported from the same codebase leveraging GLI for DDS scaffolding and Compressonator's
cmp_corefor decompressing BCn formats.ImageMagick has been replaced with Pillow package (https://pypi.org/project/Pillow/) for and the pydds extension.
Caveats
As the map compositing was approximate I took the opportunity to improve the coloring and blending of map sigils onto the map base item, producing an outcome closer to what is seen in game. Map item icons on the wiki should probably be regenerated to be consistent and prettier.
As this adds new packages both as base install requirements and as extras requirements for
cli, a development installation of PyPoE may need to be reinstalled to apply these changes in requirements.pyddsandpyoozwere authored and uploaded by @zao to PyPI as a source dist and binary wheels. While there's a full set of wheels for macOS, Windows and Linux there might be a need to build more wheels in the future when new Python versions are released. As a fallback the source dists ought to build out of the box but requires the person installing them to have C++ build tools installed for their Python.Despite being core tech unrelated to PoE releases this PR is made against
patchesas it touches code that has had improvements made inpatchessince the lastdevmerge.FAO
@angelic-knight @pm5k