divVerent / s2tc

S2TC - a subset of a wellknown texture compression scheme (actually Color Cell Compression)
https://github.com/divVerent/s2tc/wiki
Other
43 stars 6 forks source link

Invalid color clamping #7

Open graphitemaster opened 9 years ago

graphitemaster commented 9 years ago

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 commented 9 years ago

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!

FabioPedretti commented 8 years ago

graphitemaster , any updates here?