fix: don't double-weight first color's hue in average - #391
Open
chatman-media wants to merge 1 commit into
Open
Conversation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In
chroma.average(colors, mode, weights), whenmodehas a hue channel (hsl,hsv,lch,hcl,hcg, …), the first color's hue gets its weight applied twice.In the "initial color" loop,
xyz[i]is multiplied byweights[0]before the hue angle is computed:The angle
Ashould be derived from the raw hue, with the weight applied only to thecos/sincontributions — which is exactly what the loop over the remaining colors does. Because the first color's angle is scaled, its direction on the color wheel is corrupted wheneverweights[0] !== 1.Closes #252. This is also why averaging hues produces order-dependent / wrong results as reported in #282.
Why existing tests didn't catch it
Every weighted-hue test had a first color whose hue is
0(red), and0 * weight == 0, so the corruption was invisible. The bug only shows up when the first color's hue is non-zero and its weight is not 1.Reproduction
A weighted circular mean must be independent of the order the colors are listed in. Before the fix it isn't:
Red (hue 0°) carries 4× the weight of cyan (hue 180°), so the result must sit at ~0° regardless of order. Same for
lch:Reference value
Computing the weighted circular mean directly —
atan2(Σ wⱼ·sin(hⱼ), Σ wⱼ·cos(hⱼ))— for#ff0000(lch hue 40.85°, weight 3) and#00ff00(lch hue 136.02°, weight 1) gives 51.68°, matching colorjs.io / the standard mean-of-circular-quantities formula. After the fix both orderings return 51.68° and the same color#ff6300.Fix
Mirror the logic already used for the remaining colors: in the initial-color loop, compute the hue angle from the raw value and don't fold the weight into
xyz[i]for the hue channel (the final loop overwrites it with the resultant angle anyway). Non-hue channels are unchanged.Tests
Added two tests asserting order-independence of the weighted hue average (hsl + lch) plus the reference value. Both fail on
mainand pass with the fix. Full suite:2521 passed.