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.

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

Quick-fix for #1673 #1676

Closed
wants to merge 1 commit into from
Closed

Quick-fix for #1673 #1676

wants to merge 1 commit into from

Conversation

seven-phases-max
Copy link
Member

…t or divide a color from a number' constraint
@Synchro
Copy link
Member

Synchro commented Nov 24, 2013

Looks good to me.

}

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)

@lukeapage
Copy link
Member

merged.

I think youre right that it is unlikely to break someones code.

The more I thought about it, the less useful I thought the colour adding stuff is - the various colour functions are far more useful - and the behaviour is strange anyway, e.g. why would I want to do rgb(128,128,128) + rgb(128, 128, 128) and get white out?

@lukeapage lukeapage closed this Dec 8, 2013
@seven-phases-max seven-phases-max deleted the color-ops-fixes branch December 8, 2013 18:05
@seven-phases-max
Copy link
Member Author

@lukeapage, thanks!

The more I thought about it, the less useful I thought the colour adding stuff is

Well... of course #888 + #888 does not make much sense... But still there're quite handy:

(@some-color + #111);     // lighten me a bit
(@some-color + #123);     // lighten and more blueish
(@some-color + #888) / 2; // i don't want mix(...) or average(...)

I think some people will prefer this way of colour manipulation (in many cases colour functions may be not so evident and readable). So I did not actually give up my idea of adding transparency to the ops in #1675. I hope I'll come up with some proposal before v2.0, I just need to do some more "research" of possible non-contradictory and not too-breaking ways to do that: + and - are now trivial to implement (they simply can reuse new colorBlend stuff), but * and / are yet puzzle.

@theMikeD
Copy link

theMikeD commented Dec 8, 2013

Yes, I use colour manipulation the way @seven-phases-max describes, for the same reasons too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mix() returns wrong result for added colors.
4 participants