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.

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

Discrepancies in image colour space interpretation. #7

Closed
AnonymouX47 opened this issue Oct 3, 2021 · 5 comments · Fixed by #8
Closed

Discrepancies in image colour space interpretation. #7

AnonymouX47 opened this issue Oct 3, 2021 · 5 comments · Fixed by #8
Assignees
Labels
bug Something isn't working

Comments

@AnonymouX47
Copy link
Contributor

AnonymouX47 commented Oct 3, 2021

Here:
https://github.com/pranavbaburaj/img/blob/1309c087b7b7aa5ade183c2800a0789d1529b6ab/image.py#L73
conversion to RGB is not enabled by default but here:
https://github.com/pranavbaburaj/img/blob/1309c087b7b7aa5ade183c2800a0789d1529b6ab/image.py#L93
RGB values are expected.

I actually ran into this bug while trying to print a PNG image with a single (alpha) chanel. Here: https://github.com/pranavbaburaj/img/blob/1309c087b7b7aa5ade183c2800a0789d1529b6ab/image.py#L83
pixel_values was just a tuple of integers (0s and 1s), representing the opacity of each pixel.

So, here:
https://github.com/pranavbaburaj/img/blob/1309c087b7b7aa5ade183c2800a0789d1529b6ab/image.py#L90-L92
continue was always executed on every iteration.

Therefore, there was no output at all!
As expected, the image could only be displayed when I set convert_to_rgb = True when calling draw_image().

I think conversion to RGB should be made the default and then support for other colour spaces can be added later on.

@AnonymouX47 AnonymouX47 added the bug Something isn't working label Oct 3, 2021
@AnonymouX47
Copy link
Contributor Author

I can fix it but I want to know your opinion concerning it before opening a PR.

@lainq
Copy link
Owner

lainq commented Oct 3, 2021

It would be nice if u can work on this issue. I feel like you are right, rgb conversion should be true by default to avoid the hassle of of type checking. Thank you for pointing out the issue :)

@lainq
Copy link
Owner

lainq commented Oct 3, 2021

I would like to hear if you would be interested in fixing the issue :)

@AnonymouX47
Copy link
Contributor Author

I would like to hear if you would be interested in fixing the issue :)

Sure, I am. It doesn't require that much changes, so I'll open a PR as regards this very soon.

Also, I realized there'll be no need for an option to specify the colour space of an image... since the 24-bit ANSI codes already use RGB, it makes sense to just convert all images in other colour spaces to RGB.

@lainq
Copy link
Owner

lainq commented Oct 3, 2021

ah yes.

AnonymouX47 added a commit to AnonymouX47/img that referenced this issue Oct 4, 2021
@lainq lainq closed this as completed in #8 Oct 4, 2021
lainq added a commit that referenced this issue Oct 4, 2021
Convert all color spaces to RGB (fixed #7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants