Add alpha blending to Travertino #3140
Conversation
|
The MapView testbed failure on windows is unrelated. |
travertino/src/travertino/colors.py
Outdated
| self.g, | ||
| self.b, | ||
| 1, # alpha will be 1 for a rgb color. | ||
| ) |
There was a problem hiding this comment.
Is this necessary? Shouldn't it be inheriting the definition from the parent?
There was a problem hiding this comment.
Yes, it is inheriting it, so I have removed it.
travertino/src/travertino/colors.py
Outdated
| def blend_over(self, back_color: rgba) -> rgba: | ||
| """Returns an alpha blended color, by performing the "over" straight alpha | ||
| blending operation, compositing the front color over the back color.""" | ||
| return straight_alpha_blending_over( |
There was a problem hiding this comment.
As a matter of API consistency, I'm not sure about having a straight_alpha_blending_over primitive, with a blend_over member method.
You've made a clear (and well documented) distinction between straight and pre-multiplied blending; is there any practical reason why we would ever implement a premultiplied_alpha_blending_over()? Or, if we did need it, use a boolean/enum argument (i.e., alpha_blend_over(..., straight=True) or alpha_blend_over(... , method=STRAIGHT))?
I'm not saying we need to implement the other blending method - more highlighting the opportunity for a naming cleanup right now that doesn't preclude adding the alternative later if it were required.
So - why not use alpha_blend_over() and alpha_unblend_over()?
There was a problem hiding this comment.
Since, CSS's rgba() only does straight alpha blending, so I doubt we are going to be needing premultiplied alpha blending in the near future. Therefore, I have changed up the names to alpha_blend_over() and alpha_unblend_over().
Should I convert the alpha_blend_over() and alpha_unblend_over() functions, into methods of the Color class? or keep them both in the form of functions, as well as methods of Color class?
There was a problem hiding this comment.
I think it would make sense for Color to have 2 methods (blend_over() and unblend_over()); I don't think there's a need for standalone versions, or for alpha to be in the name.
The one detail that you need to watch for: the instance, or the background being blended with, could be RGB, RGBA, HSL, or HSLA - so:
- the type declaration needs to refer to
Color, notrgba; - the implementation needs to do the rgba conversion on the value provided, and
- the tests should include validation of every combination of rgb/hsl etc as base and background color.
|
The MapView testbed failure on windows is unrelated. |
| ): | ||
| try: | ||
| if {type(actual), type(expected)} == {rgba}: | ||
| assert abs(actual.rgba.r - expected.rgba.r) < tolerance |
There was a problem hiding this comment.
This is what pytest.approx() is for. It also works on tuples, not just single values.
| :param round_to_nearest_int: Should the rgb values of the blended color be | ||
| rounded to the nearest int? If the blended color will be later deblended | ||
| to get the original front color, then keeping the decimal precision will | ||
| give a more accurate value of the original front color. |
There was a problem hiding this comment.
This needs to be indented after the first line.
| :param front_color: The foreground color in the form of :any:`rgba()`. | ||
| :param back_color: The background color in the form of :any:`rgba()`. |
There was a problem hiding this comment.
There's no need to provide type details here (and the type detail is wrong anyway... and still includes the front color as an argument)
| else: | ||
| final_blended_color = blended_color | ||
| # Return in the same color format as the current color class. | ||
| return getattr(final_blended_color, current_color_class_type) |
There was a problem hiding this comment.
A nice thought, but I'm not sure it works in the general case. rgb(...).blend_over(hsla(...)) will drop all the alpha components from the resulting color. If you're blending colors, I don't know that the preservation of type is important; I'd be fine with this always returning rgba as long as it can accept any input types.
| front_color_alpha, | ||
| ) | ||
| # Return in the same color format as the current color class. | ||
| return getattr(front_color, type(self).__name__) |
There was a problem hiding this comment.
As with blend_over - this is a nice thought, but I don't think it works in the general case.
| return rgb(self.r, self.g, self.b) | ||
|
|
||
| @property | ||
| def hsla(self) -> hsla: |
There was a problem hiding this comment.
This is a nice utility, but it's only tested implicitly. Type conversions like this should have explicit tests.
| return hsla( | ||
| min(360, max(0, round(hue, 4))), | ||
| # Considering the CSS specs, specify that the saturation and lightness can | ||
| # be a percentage, so rounding to 4 decimal places would preserve precision. |
There was a problem hiding this comment.
I don't follow the logic here. How does rounding to 4dp preserve precision? Isn't it losing precision by definition?
| # The derived front color from the blended color, will be equal to the | ||
| # original front color, within the given tolerance range. | ||
| assert_equal_color( | ||
| calculated_front_color, front_color, alpha_unblending_operation=True |
There was a problem hiding this comment.
As a matter of style - long parameter names aren't always a virtue. unblend=True would mean this doesn't need to be split over 3 lines.
Related PR: #2484
This PR adds alpha blending operation capabilities to Travertino. The following APIs are added to Travertino:
Color.blend_over()straight_alpha_blending_over()reverse_straight_alpha_blending_over()PR Checklist: