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

Add and remove the compatibility APIs #2789

Merged
merged 9 commits into from
Apr 8, 2024
Merged

Add and remove the compatibility APIs #2789

merged 9 commits into from
Apr 8, 2024

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented Mar 7, 2024

Description of Change

In SkiaSharp 3.0 we are focusing on performance and making use of new APIs and structures.

However, we can add a few compatibility things back to make upgrades much simple/easier.

@maxkatz6
Copy link
Contributor

maxkatz6 commented Mar 8, 2024

SKPath.Transform(SKMatrix) can be added as well.

@mattleibow mattleibow force-pushed the dev/compat-apis branch 2 times, most recently from 67d6abc to a4c12fa Compare March 27, 2024 14:32
@mattleibow
Copy link
Contributor Author

I am trying to add back magic here, but I am wondering if this is actually useful. For example, we added some overloads - but what about all the other things that are changed - like switching from array to a span? Are we just making things better for 1% of apps?

@maxkatz6
Copy link
Contributor

@mattleibow testing on Avalonia, I didn't have any incompatibilities with span/array methods. Possibly because we didn't use these. The only issues I had were CreateBlur/DrowShadow methods, as well as SKPath.Transform and SKCanvas.SetMatrix. Not sure if it covers 1% or more.

I would expect apps that heavily use SkiaSharp API would have to upgrade either way.
Also, it seems the biggest breaking change PRs weren't merged (yet?), like other Span related PRs.

@mattleibow
Copy link
Contributor Author

Doing some more testing and I think this is working nicely. We have some new tolling that you can use to help detect things. Check out this issue description for more information: #2790

Add comments on that issue if there are any general upgrade issues or issues with the tools.

@maxkatz6
Copy link
Contributor

maxkatz6 commented Apr 6, 2024

Wow, didn't know about this tool. Quite useful. I will test 2.88.8-preview1 on Avalonia, thank you!

@maxkatz6
Copy link
Contributor

maxkatz6 commented Apr 6, 2024

@mattleibow for some reason, I see some false positives with this tool.

That all the compat issues it found (comparing with the latest nuget preview):

<member fullname="System.Void SkiaSharp.SKCanvas::SetMatrix(SkiaSharp.SKMatrix)" />
<member fullname="System.Void SkiaSharp.SKPath::Transform(SkiaSharp.SKMatrix)" />
<member fullname="SkiaSharp.SKPoint[] SkiaSharp.SKRoundRect::get_Radii()" />
<member fullname="System.Void SkiaSharp.SKMatrix::Concat(SkiaSharp.SKMatrix&amp;,SkiaSharp.SKMatrix,SkiaSharp.SKMatrix)" />
<member fullname="System.Boolean SkiaSharp.SKPathMeasure::GetPosition(System.Single,SkiaSharp.SKPoint&amp;)" />
<member fullname="System.Boolean SkiaSharp.SKPathMeasure::GetPositionAndTangent(System.Single,SkiaSharp.SKPoint&amp;,SkiaSharp.SKPoint&amp;)" />
<member fullname="System.Boolean SkiaSharp.SKRegion/RectIterator::Next(SkiaSharp.SKRectI&amp;)" />

SetMatrix and Transform make sense, as these changes weren't yet pushed.

But all other APIs: SKRoundRect::get_Radii, SKMatrix::Concat, SKPathMeasure::GetPosition, SKPathMeasure::GetPositionAndTangent, SKRegion/RectIterator::Next were not really changed, as I can see, right?

Like, before and after for RectIterator.

@maxkatz6
Copy link
Contributor

maxkatz6 commented Apr 6, 2024

Enabled render tests for SkiaSharp 3.0, some outputs are different, most likely we need to somehow use SKSamplingOptions apis with SKBitmap now (see for details). But at least compilation works well, and all the tests are passing. I think it will be enough for us, until we move to SkiaSharp 3.0 as a minimum version somewhere with 12.0 (no eta, but probably early 2025).

Will remove SKCanvas/SKPath hacks later as well, when this PR is shipped.
Thank you!

@mattleibow
Copy link
Contributor Author

for some reason, I see some false positives with this tool.

@maxkatz6 thanks for letting me know. I had a bug in the tool that was not reporting ref/out params, but I fixed that and pushed an update to nuget.

some outputs are different

Do you have some examples? There has been over a year of changes to skia, and before the sampling options the filter quality was whatever the implementation felt like. Now it is a bit more predictable. Not sure how one would handle cases where they did a pixel rounding adjustment or a slightly different algorithm for a path or something. Technically neither will be "wrong" just different based on your old baselines. If there are major differences, then maybe there is a bug in v2 or v3.

@mattleibow mattleibow merged commit 647403d into main Apr 8, 2024
74 checks passed
@mattleibow mattleibow deleted the dev/compat-apis branch April 8, 2024 19:09
@maxkatz6
Copy link
Contributor

maxkatz6 commented Apr 25, 2024

@mattleibow created AvaloniaUI/Avalonia#15503 tracking issue, what I should have done earlier.
Thanks to these PRs, Avalonia now works with SkiaSharp 3.0 without any reflection hacks which is great.
Only difference, what I mentioned before, is that paint.FilterQuality is a no-op now in native Skia code, effectively disabling rendering interpolation for bitmaps in Avalonia. But that's what I would call a "best effort" support, sufficient until 12.0.

@mattleibow
Copy link
Contributor Author

I see we do store the filter quality on the paint, but I think it was like that for some time.

Do you have an example of where it is not working properly? Maybe I am using some wrong api and not passing the quality from paint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants