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

select0r hue term for cylindrical space not handled correctly(?) #87

Closed
Undoundoundo opened this issue Oct 14, 2019 · 2 comments
Closed

Comments

@Undoundoundo
Copy link
Contributor

Still verifying with original author, but it appears to me that the hue term in HCI cylindrical space distance calculations for the select0r plugin is not handled correctly.

The rgb2hci function appears to work correctly, returning a hue term in the range of (0.5,0.5] going from cyan (-0.5, exclusive) on the counter-clockwise side through blue (-1/3rd), magenta (-1/6th), red (0), yellow (1/6th), green (1/3rd), to cyan (0.5, inclusive).

In the cylindrical color space distance functions, however, the hue term, crossing across zero, is handled via these lines:

    ax=fabsf(hue-chue);
    if (ax>0.5) ax=ax-0.5;

Where hue and chue are the two colors' hue components, fabsf takes the absolute value, and ax is ostensibly the angular distance,

This causes an issue with e.g. comparing magenta (-0.1666) and cyan (0.5), nearly opposite colors on a hue wheel:

    ax = fabsf(0.5 - -0.1666) // -> 0.6666...
    if (ax > 0.5) { // true
      ax = ax - 0.5; // 0.6666 - 0.5 -> 0.16666
    }

Making the hue distance between them equivalent to, say, red and yellow, which are neighboring on a hue wheel.

If plotted, it becomes more readily apparent that instead of the distance function being smooth, it is discontinuous. imgur.com/XUpvf6f

There are many ways to handle this, two common ones employed will find favor depending on which ends up compiling to faster execution.
A branched approach:

ax = hue - chue;
if (ax > 0.5) { ax -= 1; }
else if (ax < -0.5) { ax += 1; }

A non-branched approach:

ax = 0.5 - fabsf(fabsf(hue - chue) - 0.5);

Either should result in an appropriate hue difference (angle difference) and a continuous function. imgur.com/6TNOJXW
( both assume the angles to be normalized in the -0.5 .. 0.5 range and should not be used as general purpose angle difference formula )

Note that proposed changes to this code will affect its results when using one of the cylindrical color spaces, and dependent projects may wish to add a compatibility flag or advise end-users of such a change.

@ddennedy
Copy link
Collaborator

Marko sent an e-mail to the developer mailing list asking someone to help you because he is having problems with git-ssh. He agrees with your finding, and I volunteered to help. Have you decided on a preference between your 2 changes? If so, can you make a pull request or at least run git diff and post it here? Thanks

Undoundoundo added a commit to Undoundoundo/frei0r that referenced this issue Oct 20, 2019
Corrects the discontinuous nature of the hue term distance function - which could lead to visibly different colors being considered close matches - into a continuous one.  This is a behavior-modifying change.  For more details, see dyne#87
@Undoundoundo
Copy link
Contributor Author

Bit rusty on my github routine myself, but the above PR should be it. I ended up choosing the non-branching version based solely on online benchmarks where it had a slight edge ( < 0.01% ). Exact build configuration may well cause the branching version to have the edge, but it's unlikely to affect performance enough to matter.

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

No branches or pull requests

2 participants