treeform / pixie

Full-featured 2d graphics library for Nim.
MIT License
743 stars 28 forks source link

Add PPM format support #363

Closed nnsee closed 2 years ago

nnsee commented 2 years ago

This is a PR to add Netbpm's PPM format to Pixie. See issue #359 for more information on the format.

List of things to do:

treeform commented 2 years ago

Looks pretty good, thank you for the PR. can't wait for it to be finished.

nnsee commented 2 years ago

Thanks.

This is the first time for me working on a "real" project in Nim, so any criticism is highly welcome.

treeform commented 2 years ago

Does PPM support alpha? I think when writing out you need a convert image to straight alpha first?

nnsee commented 2 years ago

Good point, PPM does not support transparency or the alpha channel. I incorrectly did .rgba(), but I've now removed that so transparency is blended with black instead.

treeform commented 2 years ago

Do you think its ready to go? Or are you still working on it?

nnsee commented 2 years ago

Functionality wise it's good to go, but I was wondering whether the code could be improved, but if you think it looks fine then it could be merged.

nnsee commented 2 years ago

Actually, scratch that, I've just found a bug in my implementation.

nnsee commented 2 years ago

Okay, I've fixed the bug and added a test case for it.

When running tests, all of the produced images should be identical. I usually check manually by running md5sum tests/fileformats/ppm/!(*.master.ppm) in bash (needs extglob set), but should I instead be assert-ing this in the test file itself?

treeform commented 2 years ago

I think you should assert. PPM has no compression so you can just have a master and check that the file bytes match. Then there is not need for manual check.