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.

Sign up for GitHub

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

Jump to bottom

Quick-fix for #1673 #1676

Closed
wants to merge 1 commit into from
Closed

Quick-fix for #1673 #1676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions lib/less/tree/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,12 @@ tree.Color.prototype = {
// we create a new Color node to hold the result.
//
operate: function (env, op, other) {
var result = [];

if (! (other instanceof tree.Color)) {
other = other.toColor();
}

var rgb = [];
var alpha = this.alpha * (1 - other.alpha) + other.alpha;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conside 0.1 and 0,9 - used to give 1, now would give

0.1 * (1-0.9) + 0.9
or
0.9 * (1-0.1) + 0.1

e.g.
0.91

why this instead of the old result, e.g. why shouldn't it be Math.min(1, this.alpha + other.alpha)

otherwise this looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.1 and 0,9 - used to give 1

And that is exactly what was wrong. Real-world example: Consider two half-transparent pieces of glass, when you put them together you can never get a completely non-transparent glass packet. The new formula is a common standard of handling alpha channel within color operations, basically it's just this one also known as this one (the same alpha compositing algorithm is used in svg, html.canvas and it was used in most of graphics editors and libraries since 80's :)

Math.min(1, this.alpha + other.alpha)

Nope, this is incorrect method. Alpha channel is like a color multiplier - so (c1 * c1alpha) op (c2 * c2alpha) cannot be implemented as just (c1 op c2) * clamp(c1alpha + c2alpha).

Although this patch only fixes the overflowed alpha channel (i.e. #1673), but the operations themselves remain incorrect (so it is actually possible to use Math.min(1, this.alpha + other.alpha) for backward compatibility since it only makes either way incorrect transaparent-color ops to be just a bit more incorrect :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I see that, but is it correct to handle alpha like this but not the other components? this is the reason we added the extra blend functions, because standard add and mix handle things in a very standard way.

I like the change.. as long as no-one is relying on rgba(1,2,3,0.5) + rgb(1,2,3,0.5) to have alpha 1 like it is at the moment.. I'm just cautious about breaking changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I see that, but is it correct to handle alpha like this but not the other components?

Nope (though it still gives just a bit more "fair" result than clamp(c1a + c2a)), but in fact I used this formula only because it's simple and gives correct result for non-transparent colors passed (e.g. for alpha 1 + 1). So I absolutely do not mind if you change it to Math.min(1, this.alpha + other.alpha).

I'm just cautious about breaking changes.

I understand. Though I'd say the earlier a breaking change comes the less code it breaks :) I doubt if there's any code out there that really does something like rgba(1,2,3,0.5) + rgba(1,2,3,0.5)

for (var c = 0; c < 3; c++) {
result[c] = tree.operate(env, op, this.rgb[c], other.rgb[c]);
rgb[c] = tree.operate(env, op, this.rgb[c], other.rgb[c]);
}
return new(tree.Color)(result, this.alpha + other.alpha);
return new(tree.Color)(rgb, alpha);
},

toRGB: function () {
Expand Down
13 changes: 5 additions & 8 deletions lib/less/tree/operation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,14 @@ tree.Operation.prototype = {
},
eval: function (env) {
var a = this.operands[0].eval(env),
b = this.operands[1].eval(env),
temp;
b = this.operands[1].eval(env);

if (env.isMathOn()) {
if (a instanceof tree.Dimension && b instanceof tree.Color) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about this too.

If I see 1 + black I would almost expect black to be converted to a colour where as if I see black + 1 it looks more like black, adding 1 to each colour component. In the same way 3 / red doesn't make as much sense to me as red / 3

what was your thinking in now allowing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was your thinking in now allowing this?

Let's see, w/o this patch it's:

black + 1; // OK
1 + black; // OK
black - 1; // OK
1 - black; // Error

Does it make sense that the addition is commutative (for type) and the substraction is not?
(E.g. (255 - red) looks fine as a way to get to cyan.) And the div was made the same just to follow others.

If I see 1 + black I would almost expect black to be converted to a colour

Or did you mean to convert black to a number? Maybe. But it's a common practice to convert operands to a "bigger" operand type regardless of which operand comes first, e.g. javascript:

console.log(typeof(1 * true)); // -> number
console.log(typeof(true * 1)); // -> number
console.log(typeof(1 / true)); // -> number
console.log(typeof(true / 1)); // -> number

Same for many other languages (at least those that allow math ops on different types).
So my main intention was just to make code more simple by removing a constrain that I saw as a sort of artificial. Btw.:

255 / @some-color * #963;

... strange but not absolutely meaningless.

if (this.op === '*' || this.op === '+') {
temp = b, b = a, a = temp;
} else {
throw { type: "Operation",
message: "Can't substract or divide a color from a number" };
}
a = a.toColor();
}
if (b instanceof tree.Dimension && a instanceof tree.Color) {
b = b.toColor();
}
if (!a.operate) {
throw { type: "Operation",
Expand Down
3 changes: 0 additions & 3 deletions test/less/errors/color-operation-error.less
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions test/less/errors/color-operation-error.txt
View file
Open in desktop

This file was deleted.