[Windows] Fix performance issue using Shadows #18337
Conversation
jsuarezruiz
added
t/bug
|
I see a lot of code here that we are writing and maintaining. How close is this to https://learn.microsoft.com/en-us/windows/communitytoolkit/helpers/attachedshadows? Can we borrow some of that code and rather sync it instead of us creating the same code and having to fix all the same bugs that they probably already fixed? We could copy the required files and change to internal. |
Initially the Toolkit had |
|
/rebase |
#29681 was merged. Perhaps if |
|
@jfversluis this pr needs a rebase and probably then to adjust the artifacts |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a performance issue with shadows on Windows by optimizing the shadow rendering logic. The fix prevents unnecessary expensive operations when elements have clipping applied, which was causing severe performance degradation.
- Adds a clipping check to avoid expensive alpha mask operations on clipped elements
- Introduces special handling for ContentPanel elements
- Includes comprehensive test coverage and benchmarking tools for shadow performance
Reviewed Changes
Copilot reviewed 7 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ShadowExtensions.cs | Core fix - adds clipping detection and optimizes alpha mask generation logic |
| Issue18172.cs (test) | UI test implementation for validating shadow rendering behavior |
| Issue18172.cs (hostapp) | Test page demonstrating the shadow performance issue scenario |
| ShadowPage.cs | Adds navigation to new shadow testing pages |
| ShadowMaskPage.xaml/.cs | Sample page for testing shadow mask functionality |
| ShadowBenchmark.cs | Performance benchmarking tool for shadow rendering |
| else if (element is FrameworkElement frameworkElement) | ||
| if (!isClipped && element is ContentPanel contentPanel) | ||
| { | ||
| return contentPanel.BorderPath?.GetAlphaMask(); |
There was a problem hiding this comment.
This method returns a CompositionBrush but the call to GetAlphaMask() could return null due to the null-conditional operator. This will cause a type mismatch since the method signature expects a non-null CompositionBrush return value.
| return contentPanel.BorderPath?.GetAlphaMask(); | |
| mask = contentPanel.BorderPath?.GetAlphaMask(); | |
| break; |
Copilot uses AI. Check for mistakes.
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue18172.cs
Outdated
Show resolved
Hide resolved
…172.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
There are two tests failing 
and it looks like they might be genuine test failures 
|
|
@jfversluis Could you please add the images: snapshots-diff.zip (I downloaded the drop.zip file and extracted relevant files) ? |
There was a problem hiding this comment.
@jsuarezruiz I know this was wrong even before, but there should be no shadows here given that the code does
case 5:
border.Shadow = null;
break;
|
Looking at 24414 screenshots, I can definitely say that there are regressions in shadow drawing. |
That test's snapshot is not yet updated: #18337 (comment) (if you look only at github). I can't paste here atm from phone. |
|
Adding do-not-merge, setting it to draft, while everything builds, the snapshots seem to indicate a change in how shadows are rendered after this change. |
|
The PR shoud be rebased to the inflight branch and not main. |
|
Now that #31488 and #31452 are merged in main, is it possible to re-run the pipeline please? @jfversluis |
|
Looks like there are conflicts to resolve now first @jsuarezruiz |
I did it here: #31713 |
12 participants
Description of Change
Fix performance issue using Shadows on Windows.
Issues Fixed
Fixes #18205