Skip to content

Add PPM format support#363

Merged
treeform merged 9 commits intotreeform:masterfrom
nnsee:ppm
Jan 14, 2022
Merged

Add PPM format support#363
treeform merged 9 commits intotreeform:masterfrom
nnsee:ppm

Conversation

@nnsee
Copy link
Copy Markdown
Contributor

@nnsee nnsee commented Jan 6, 2022

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:

  • CLI front end support
  • PPM encoder
  • PPM decoder
    • P3 (plain) format
    • P6 format
  • Tests
  • Add to documentation

@nnsee nnsee changed the title WIP: add PPM encoder WIP: add PPM format support Jan 6, 2022
@treeform
Copy link
Copy Markdown
Owner

treeform commented Jan 6, 2022

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

@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Jan 7, 2022

Thanks.

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

@treeform
Copy link
Copy Markdown
Owner

treeform commented Jan 7, 2022

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

@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Jan 9, 2022

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
Copy link
Copy Markdown
Owner

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

@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Jan 10, 2022

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 nnsee changed the title WIP: add PPM format support Add PPM format support Jan 10, 2022
@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Jan 10, 2022

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

@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Jan 10, 2022

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
Copy link
Copy Markdown
Owner

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.

@treeform treeform merged commit cc5c699 into treeform:master Jan 14, 2022
@nnsee nnsee deleted the ppm branch January 30, 2022 11:43
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.

2 participants