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

Invalid color clamping #7

Open
graphitemaster opened this issue Oct 26, 2014 · 2 comments
Open

Invalid color clamping #7

graphitemaster opened this issue Oct 26, 2014 · 2 comments

Comments

@graphitemaster
Copy link

The following is incorrect

        inline operator color_t()
        {
            color_t out;
            out.r = r & 31;
            out.g = g & 63;
            out.b = b & 31;
            return out;
        }

This should be made into

        inline operator color_t()
        {
            color_t out;
            out.r = r % 31;
            out.g = g % 63;
            out.b = b % 31;
            return out;
        }

Or the following which isn't correct either as 31 and 63 are not powers of two, but probably what was intended when it was written.

        inline operator color_t()
        {
            color_t out;
            out.r = r & (31 - 1);
            out.g = g & (63 - 1);
            out.b = b & (31 - 1);
            return out;
        }
@divVerent
Copy link
Owner

Please attach an input file on which this code causes invalid output to be generated.

Both your change suggestions cause incorrect encoding of all-white blocks (#ffffff, causing r=31, g=63, b=31) as all-black.

Furthermore, the AND should actually in all practically relevant cases be the identity operation, and only serves to fulfill the output requirements; this is also why this code merely does AND and no clamping. If you can provide any input where this is relevant, I will check whether the calling code is wrong (and uses the color data wrong), or whether we really need clamping here.

Thanks!

@divVerent divVerent mentioned this issue Nov 10, 2015
@FabioPedretti
Copy link

graphitemaster , any updates here?

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

3 participants