-
Notifications
You must be signed in to change notification settings - Fork 667
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
[css-color-5] Adjustments to edge cases and set notation for hue interpolation types #5278
Conversation
(This is following up to the changes made in 4e88f2c.) |
css-color-5/Overview.bs
Outdated
θ₂ += 360; | ||
} | ||
</pre> | ||
|
||
: ''longer'' | ||
:: Angles are adjusted so that θ₂ - θ₁ ∈ [180, 360). In pseudo-Javascript: | ||
:: Angles are adjusted so that θ₂ - θ₁ ∈ {(-360, -180], 0, (180, 360)}. In pseudo-Javascript: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this rather hard to read, can we prioritize readability over conciseness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By just changing it to an English sentence rather than set notation? Or some other way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is not needed if we make the sets closed, and we can combine [-360, -180] and [180, 360] by using |θ₂ - θ₁| ∈ [180, 360]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still pretty hesitant to write a rule that says that interpolation of identical hues goes around the circle -- both because we'd have to choose which way, and because I think it might be problematic if we applied such rules to transitions and animations, which I think may depend in some ways on interpolation of identical values not going through other values.
I'll make the other changes shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, yeah.
css-color-5/Overview.bs
Outdated
<pre> | ||
if (0 < θ₂ - θ₁ < 180) { | ||
if (0 < θ₂ - θ₁ <= 180) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Currently on the equal case, you just don't add 360. It's still well-defined. Is there a reason why we want to enter this clause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for wanting to do this is to follow the principle from @svgeesus that angles that differ by 180 should go in the decreasing
direction. Without this change, the direction they go would depend on what angles they were, since it would be determined by the constraining to [0, 360)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it some more, I think if θ₂ - θ₁ == 180 (so θ₂ == θ₁ + 180) then interpolation should be the same as increasing
, whereas when θ₂ - θ₁ == -180 (so θ₂ == θ₁ - 180), interpolation should be the same as decreasing. This seems to be less surprise-inducing than going in the same direction regardless of whether it's θ₂ < θ₁ or θ₂ > θ₁.
I'll think of how to put that in these algorithms tomorrow (or if you want to give it a shot, fine by me!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @LeaVerou convinced me that basing the direction at exactly 180 on the sign of the angle difference made more sense.
…rpolation types. This makes a few adjustments to the definitions of hue interpolation types to follow up on the discussions in w3c#4735. First, it adjusts the pseudo-code so that for 'shorter' and 'longer', angles that differ by 180 degrees will always go in the decreasing direction, as described in w3c#4735 (comment) Second, it adjusts the set notation to match the pseudo-code. (They were not matching even prior to this change.) The set notation previously seemed to assume that there were absolute-value functions present that were not actually there. However, given the asymmetry of always going in the decreasing direction, it's more correct to write it without absolute values.
… and fix inconsistent indent.
a2bc0e0
to
09236c1
Compare
LGTM, and even if we tweak further eventually, the changes bring it to a much better state than the current one, so merging. |
This makes a few adjustments to the definitions of hue interpolation
types to follow up on the discussions in #4735.
First, it adjusts the pseudo-code so that for 'shorter' and 'longer',
angles that differ by 180 degrees will always go in the decreasing
direction, as described in
#4735 (comment)
Second, it adjusts the set notation to match the pseudo-code. (They
were not matching even prior to this change.) The set notation
previously seemed to assume that there were absolute-value functions
present that were not actually there. However, given the asymmetry of
always going in the decreasing direction, it's more correct to write it
without absolute values.