Ogeon / palette

A Rust library for linear color calculations and conversion
Apache License 2.0
749 stars 60 forks source link

Fix potential `NaN` from converting to `Okhsl` when the input is white or black #369

Closed Ogeon closed 7 months ago

Ogeon commented 7 months ago

This turned out to be an error in the reference implementation as well, as I understand it. This line in the get_Cs function produces NaN when the lightness is 1.0 or 0.0, by dividing 0.0 by 0.0:

let k = C_max/Math.min((L*ST_max[0]), (1-L)*ST_max[1]);

The reference seem to be missing the necessary safeguards for this. I looked around for other implementations to see if they had the same issue, but didn't find anything so far. Regardless, it breaks down if it gets an Oklab value with lightness 1 or 0. Some inaccuracies or rounding made the chroma != 0.0, which bypassed the already added check for that and let the NaNs out.

Closed Issues

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (508f644) 80.62% compared to head (503f34c) 81.46%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #369 +/- ## ========================================== + Coverage 80.62% 81.46% +0.84% ========================================== Files 113 113 Lines 18502 18438 -64 Branches 18502 18438 -64 ========================================== + Hits 14917 15021 +104 + Misses 3297 3220 -77 + Partials 288 197 -91 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codspeed-hq[bot] commented 7 months ago

CodSpeed Performance Report

Merging #369 will improve performances by ×3.1

Comparing fix_issue_368 (503f34c) with master (508f644)

Summary

⚡ 7 improvements ✅ 38 untouched benchmarks

Benchmarks breakdown

Benchmark master fix_issue_368 Change
yxy to xyz 20 µs 13.2 µs +50.98%
matrix_inverse 838.9 ns 271.1 ns ×3.1
xyz to yxy 18.4 µs 11.2 µs +64.26%
multiply_3x3 427.8 ns 372.2 ns +14.93%
hsv to hsl 29 µs 16 µs +81.27%
hsl to hsv 20.5 µs 13.9 µs +46.74%
hwb to hsv 18.3 µs 12.1 µs +51.57%
Ogeon commented 7 months ago

bors r+

Ogeon commented 7 months ago

Oh, looks like bors has finally been disabled. :disappointed: