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

Added an "approximate_rgb" function. #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

exhuma
Copy link

@exhuma exhuma commented Aug 9, 2013

Also covered 256 colour codes with test. Not 100% sure if those test are
any good though.

@fwenzel
Copy link
Collaborator

fwenzel commented Apr 29, 2014

This would fix #14, I presume.

@exhuma
Copy link
Author

exhuma commented Apr 30, 2014

Hmm... just saw the failed tests. Seems it's working on Python 2.7 but not on 2.5 and 2.6. I will need to look into that.

@exhuma
Copy link
Author

exhuma commented Apr 30, 2014

Tried to install Python 2.5 and 2.6 from source, but I am having trouble making virtualenv work with them... I am giving up for now due to time-constraints :(

If I don't forget it, I will look it up later again.

@erikrose
Copy link
Owner

erikrose commented May 1, 2014

I'm fully in favor of a nearest-possible-color fallback in case of limited-color terminals, in case it helps.

@jquast
Copy link
Collaborator

jquast commented Mar 1, 2015

I designed a draft proposal in issues #67, please review. It would likely make some use of the given code.

I am going to provide some criticism of this PR, please consider it constructive!

  • the comments do not explain the numerical adjustments made.
  • the only seemingly useful comment is really just a complaint. https://docs.python.org/devguide/documenting.html#affirmative-tone
  • People will study Blessings' internals to re-write analogous libraries in other languages. Blessings should continue to not only be a useful API, but a depth of code that others may learn and implement from. This particular function is very unforgiving in describing the implementation.
  • as such, i don't think anybody is qualified to "sign off" on this kind of PR. Speaking for myself, I certainly wouldn't have confidence that it is correct.
  • the tests do not provide any assurance that the color space was mapped correctly, to clarify: this could just as easily been created as "I ran it, i got these values, so I reversed it into a test".

So,

The rules of why and how these values are clamped/divided/reduced, then matched along a row/col/face, then some magic about indexes should be declared as a code comment in the implementation: so there is no need to express why such values are matched in the test: it should be desk-checkable by the given description of the implementation.

I think I see three functions to be compoundly tested:

  • reducing the 8x8x8-bit colorspace to 6x6x6-bit.
  • finding its face/col/row.
  • given this face/col/row, finding its range in 0-256.

Currently this is all done in a single function. My proposal in #67 also hopes to make use of variants of these 3 functions to map to a 24-bit colorspace as well, which would require conditionally excluding some of these components.

@exhuma
Copy link
Author

exhuma commented Mar 1, 2015

I fully agree with your comments. It has been a while since I looked into this code, I don't even remember writing this, and it does not even look like something I had written. Not sure where even it comes from. Right now, I don't get too much time to do some research in this, but I will try to get back to that code as soon as I can and clean things up. I hope I will get to it soon.

@ssbarnea
Copy link

ssbarnea commented Sep 3, 2019

Do we have any change of making this a reality?

@jquast
Copy link
Collaborator

jquast commented Jan 12, 2020

Also covered 256 colour codes with test. Not 100% sure if those test are
any good though.
@exhuma
Copy link
Author

exhuma commented Oct 6, 2021

I've stumbled across this issue again. It dropped off my radar again. I will deal with the conflicts and will change the code commend to something objective.

@exhuma exhuma force-pushed the 256-colour-support branch from 024fffd to 2979a15 Compare October 6, 2021 06:35
@exhuma
Copy link
Author

exhuma commented Oct 6, 2021

Done. Should be better now.

@exhuma
Copy link
Author

exhuma commented Oct 6, 2021

I've also hunted down the original implementation and added a link (for attribution).

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.

5 participants