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

Improve shadow rendering on Android, fix shadow clipping on iOS #26789

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Dec 23, 2024

Android Shadow Rendering

Description of Change

  • move the shadow computation to android level with an improved algorithm
    • use Glide's LruBitmapPool to reduce the memory pressure on Java
    • prepare Paint only when shadow changes
  • when we know we're going to render a solid shape we can use Canvas API to speedup the shadow rendering performance by a lot
  • remove unneeded saveLayer call from platform interop: it's a very expensive operation done for no reasons here
  • fix shadow rendering on Android which was still broken (despite Shadows not rendering as expected on Android and iOS #24414) under some conditions (like negative offsets)

Benchmark

Here I've executed the testing host app with return new ShadowBenchmark(); as main page and scrolling the CV once.

Before

image

After (with transparency)

You can see this is a bit faster, and for sure it is creating less GC/allocation work due to the bitmap pool.
Still timings are not good enough.

image

After (with solid background)

This time no need to create bitmaps and render descendants, we can simply draw the shadow.
And this is exactly the speed we want.

image

Notes

To collect those speedscopes I've temporary re-added:

protected override void DrawShadow(Canvas canvas, int viewWidth, int viewHeight)
{
	base.DrawShadow(canvas, viewWidth, viewHeight);
}

iOS Shadow Rendering

  • Removes unneeded shadow sublayer from wrapper view => we can simply apply the shadow to the WrapperView's main Layer considering it's the parent of the clipped content.
  • Make clear that the layer where we apply clipping is the content's main Layer
  • Ensure that when the clip is removed, we also clear the content's Layer's mask

Issues Fixed

Fixes #27156
Fixes #23630
Fixes #20518
Fixes #18202
Fixes #17886
Fixes #13981
Fixes #9539
Fixes #4106

Relates to #17881
Relates to #17257
Relates to #10401

@albyrock87 albyrock87 requested a review from a team as a code owner December 23, 2024 16:46
@albyrock87 albyrock87 marked this pull request as draft December 23, 2024 16:46
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Dec 23, 2024
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added this to the .NET 9 SR4 milestone Dec 23, 2024
@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87 albyrock87 force-pushed the android-shadow branch 5 times, most recently from 2f0b86f to eccf37e Compare December 24, 2024 13:45
@AmrAlSayed0
Copy link
Contributor

I'm late to the party but an attempt to improve shadow's performance was made before here #10523. Moving the code to the Java side might help but I think what would benefit the most is a shadow cache. All views that have the same size and the same shadow properties (very common scenario in CollectionView items for example) should use the same bitmap. This will eliminate a huge number of calls including C# to Java ones. There is an implementation of the shadow cache in the PR.

@albyrock87
Copy link
Contributor Author

@AmrAlSayed0 thanks for sharing, I'll check it out.
This is still a very early WIP.
I also intend using Glide's bitmap pool for this, which should improve the performance by avoiding continuous allocations of the bitmaps.

@albyrock87 albyrock87 force-pushed the android-shadow branch 11 times, most recently from d40a883 to 5cb0404 Compare December 26, 2024 11:35
@albyrock87
Copy link
Contributor Author

This PR depends on #27531 to fix MacCatalyst screenshot taking.

@PureWeen
Copy link
Member

PureWeen commented Feb 4, 2025

/rebase

@PureWeen
Copy link
Member

PureWeen commented Feb 4, 2025

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87
Copy link
Contributor Author

Failing tests look unrelated.

@jsuarezruiz jsuarezruiz self-assigned this Feb 4, 2025
[InlineData("#FF0000")]
[InlineData("#00FF00")]
[InlineData("#0000FF")]
public void SolidPaintTest(string hexColor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some Device Tests to validate the IsSolid extension method.

@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Feb 5, 2025

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot Feb 5, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Feb 5, 2025
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz self-requested a review February 6, 2025 15:28
jsuarezruiz
jsuarezruiz previously approved these changes Feb 6, 2025
@jsuarezruiz
Copy link
Contributor

Rebased and updated macOS snapshots after merge #27531

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz self-requested a review February 7, 2025 12:29
@PureWeen PureWeen merged commit a27ddfd into dotnet:main Feb 10, 2025
123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing community ✨ Community Contribution
Projects
Status: Done
5 participants