-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Clip inner content from AttachedCardShadow using CompositionMaskBrush #4404
Clip inner content from AttachedCardShadow using CompositionMaskBrush #4404
Conversation
Thanks Ryken100 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
|
@Ryken100 looks like this is based off your other nine grid PR, mind rebasing with just the relevant commits for this new feature (to keep history cleaner)? Also, can we pull someone else in your team to help review too, thanks! @Arlodotexe @XAML-Knight feel free for one of you to grab this on your plate to review as well. |
91108b4
to
0809907
Compare
Fix DependencyProperty registration
I've rebased it :) |
Remove extra OnPropertyChanged call
Microsoft.Toolkit.Uwp.UI/Shadows/AttachedShadowElementContext.cs
Outdated
Show resolved
Hide resolved
@michael-hawker Looks like something happened to this page, changing the options only affects one of the images. Easy to repro in the live sample app, so this PR isn't the cause. Might need to file a new issue. |
That's the current expected behavior, they're only tied to one element in the XAML here: Lines 41 to 57 in 00e9790
We'll be re-working these all for 8.0 anyway with the new sample infrastructure from Labs, so don't think we need an issue/tracking for now. |
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.
Some minor nits, but otherwise LGTM! 🚀
Microsoft.Toolkit.Uwp.UI/Shadows/AttachedShadowElementContext.cs
Outdated
Show resolved
Hide resolved
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
@Sergio0694 looks good to you now? I think @Ryken100 has addressed your comments? |
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.
LGTM, ship it!
Fixes #4410
This PR adds the option to clip inner content from shadows using
CompositionMaskBrush
, which may improve performance over the defaultCompositionGeometricClip
method.PR Type
What kind of change does this PR introduce?
What is the current behavior?
AttachedCardShadow
only clips its inner content usingCompositionGeometricClip
, which can have poor framerates when several shadows are on screen at a time.What is the new behavior?
Added the option to clip inner shadow content using
CompositionMaskBrush
, which improves performance overCompositionGeometricClip
when several shadows are on screen at a time, but looks slightly worse as the boundary of the inner clipped are lacks anti aliasing.An
InnerContentClipMode
enum was added, as well as anAttachedCardShadow.InnerContentClipMode
property, making it possible to choose betweenCompositionGeometricClip
andCompositionMaskBrush
clipping modes.When
CompositionMaskBrush
is selected, aCompositionMaskBrush
is created, with aCompositionSurfaceBrush
rendering a hollow rounded rectangle set as itsMask
and aCompositionSurfaceBrush
rendering theSpriteVisual
with theDropShadow
set as itsSource
. Then an additionalSpriteVisual
is created which renders theCompositionMaskBrush
.The
CompositionMaskBrush
has slightly worse visuals thanCompositionGeometricClip
, as the corners of the inner clipped area lack anti aliasing. This is difficult to notice when the shadow has low opacity, but can stand out with high opacity shadows. For that reason, this PR keepsCompositionGeometricClip
as the default method, whileCompositionMaskBrush
can be selected manually.PR Checklist
Please check if your PR fulfills the following requirements: