Implement SkCanvas::SaveLayerRec

[ahmed605]

Description of Change

Implement SkCanvas::SaveLayerRec

Bugs Fixed

• Fixes [FEATURE] Expose SkCanvas::SaveLayerRec API #2773

API Changes

Added:

class SKCanvas {
    public int SaveLayer (SKCanvasSaveLayerRec rec)
}

public enum SKCanvasSaveLayerRecFlags {
    None = 0,
    PreserveLcdText = 2,
    InitializeWithPrevious = 4,
    F16ColorType = 16,
}

public unsafe class SKCanvasSaveLayerRec {
    public SKRect? Bounds { get; set; }
    public SKPaint? Paint { get; set; }
    public SKImageFilter? Backdrop { get; set; }
    public SKCanvasSaveLayerRecFlags Flags { get; set; }
}

Behavioral Changes

None.

Required skia PR

mono/skia#130

PR Checklist

• Has tests (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Merged related skia PRs
• Changes adhere to coding standard
• Updated documentation

[mattleibow]

Not sure my magic with CI is working with the auto detection of the skia branch (probably because I am not doing it right for forks) so maybe just update the submodule here and we can merge in one go later once the skia PR is merged.

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[ramezgerges]

@mattleibow Hello, I'm an Uno Platform dev and I'm following up on this PR (and the accompanying mono/skia PR). I haven't actually tested the new APIs yet, but here are my thoughts.

• The SaveLayer signature that takes an SKCanvasSaveLayerRec (instead of individual components) seems to be identical to the C++ API, so it makes sense.
• Shouldn't we make SKCanvasSaveLayerRec a struct instead? It seems to be something you create, immediately pass to SaveLayer, and then throw away. The example in google/skia (https://github.com/google/skia/blob/d2ac2b0d6467fdca614bd85b49bf01363fe94604/docs/examples/Canvas_SaveLayerRec.cpp#L15) confirms this.

[mattleibow]

Thanks for the review @ramezgerges, this makes sense today with the small struct. But, if you look at the latest skia, does your thought still seem valid? The struct is getting fairly large: https://github.com/google/skia/blob/main/include/core/SkCanvas.h#L689 There are new properties to be added - color space, image filters as well as a collection/array of image filters.

Not sure either way here, but at what point to we think a class is better? Will we ever re-use this object for multiple calls?

I was also thinking that due to the large-ish size and potentially fairly complex nature, it should be a class: https://learn.microsoft.com/dotnet/standard/design-guidelines/choosing-between-class-and-struct

However, rules in .NET are just best ideas for most cases, not the law. So, maybe we can break it. But, a struct may still be the best. Not sure if any of this changes anything? Let me know if you still think a struct is better.

[ramezgerges]

Thanks for getting back to me, @mattleibow. Just to be clear, the struct-vs-class point is merely a suggestion/observation, and I'm okay with the PR either way.

• I didn't notice the new properties that were added. However, I think the argument is still the same: assuming that 99% of the usescases follow the "Create a SaveLayerRec -> Immediately give it to SaveLayer -> forget about it" pattern, then allocating a new object every time seems wasteful. If we're worried about the cost of copying, then can't we just make SaveLayer take a ref (or a ref readonly) parameter? i.e.
  public int SaveLayer (ref readonly SKCanvasSaveLayerRec rec)

• If we keep it as a class, can we instead add a Reset method to reset the SaveLayerRec to its initial state? We've had to deal with excessive SKPath/SKPaint allocations in our main loop before and the workaround I did was to use an ObjectPool + a Reset() call to get a "fresh" SKPath/SKPaint.

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[mattleibow]

@ramezgerges Sorry for the super long wait. I think your comments are the way to go, so I have updated the rec type to be a struct and also use the new in keywork so that we don't make a copy of the struct either.

Let me know if this will work for you and if there are any other changes that you think need to be made.

[ramezgerges]

@mattleibow No worries. I tested the CI packages today and things are working great!

[mattleibow]

Thanks for creating this PR and sticking with it for a year :)

I have merged and it should go out into the preview feed shortly. If there are any issues or adjustments, feel free to open a PR.

Thanks again!

[jeromelaban]

Many thanks @mattleibow!