-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Introduce RenderOptions.RequiresFullOpacityHandling #12572
Introduce RenderOptions.RequiresFullOpacityHandling #12572
Conversation
@Gillibald I guess there are issues that would be solved? e.g.: #9227 |
Yes, it gives control over layered opacity handling per visual so the performance impact isn't that high anymore. It still applies to the whole sub-scene so if you define this on the TopLevel it behaves the same as if you enable it globally via the UserOpacitySaveLayer option. |
You can test this PR using the following package version. |
@@ -189,7 +189,10 @@ public void DrawBitmap(IBitmapImpl source, double opacity, Rect sourceRect, Rect | |||
var d = destRect.ToSKRect(); | |||
|
|||
var paint = SKPaintCache.Shared.Get(); | |||
paint.Color = new SKColor(255, 255, 255, (byte)(255 * opacity * (_useOpacitySaveLayer ? 1 : _currentOpacity))); | |||
|
|||
var useOpacitySaveLayer = _useOpacitySaveLayer || RenderOptions.RequiresFullOpacityHandling == true; |
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.
We need to push the _currentOpacity
value and set it to 1. Otherwise rendering will be broken with the following visual tree structure:
- Border №1 - opacity: 0.8
- Border №2 - opacity: 0.8, requires full opacity: true
- Border №3 - opacity: 0.8, requires full opacity: true
In this case Border №3 would get rendered with 0.64 opacity into a layer created by Border №2, while it should be rendered with 0.8.
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.
Another problem I think we have is the first two borders: we don't take the accumulated _currentOpacity from Border №1 when rendering the layer from Border №2.
if(_useOpacitySaveLayer) | ||
var useOpacitySaveLayer = _useOpacitySaveLayer || RenderOptions.RequiresFullOpacityHandling == true; | ||
|
||
if (useOpacitySaveLayer) | ||
{ | ||
if (bounds.HasValue) |
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.
So in this branch we still need to use the opacity stack, but instead of _currentOpacity *= opacity;
we need _currentOpacity = 1
Just curious, once this attached property gets added, would |
Correct. With the small difference that the Skia option applies globally. |
That's great, it will be much nicer to have this render option available to only affect small subtrees where needed. |
4b998b1
to
4b5c680
Compare
You can test this PR using the following package version. |
What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues