[Enhancement] clarify tranform relationship between CanvasState and ICanvas

[vpenades]

From my previous PR, I've been digging into how the CanvasState and its derived classes behave, in relationship with ICanvas. It's possible that I got something wrong, and I would like to be corrected if that's the case, but so far, my understanding is this:

• (so far) CanvasState represents the transform state of an ICanvas derived class.
• CanvasState derived classes mirror the transform to the native transform implementation.

The issue I noticed is that several CanvasState implementations implement methods called NativeRotatate, NativeTranslate and so on, that are being called directly from the ICanvas Rotate, Translate and Scale methods. So when that happens, it bypasses the implict transform of the CanvasState base class.

Also, ICanvas has two paths to set a transform: via the ICanvas state CanvasState.Transform, where the CanvasState transform can be set directly (but right now would have no effect), and through the ICanvas transform methods, which are directed to the native transform, but don't change the CanvasState.Transform.

I think this is inconsistent and a potential source of conflicts, because there's two different ways of setting the transform, and it's not clear which is the right one, or if it even has an effect.

my proposal:

In my PR I've introduced a "TransformChanged" virtual method to the CanvasState base class, this method can be overridden by derived CanvasState classes, so whenever the base class Transform changes, they can mirror the transform change to the native transform. This would allow CanvasState.Transform to be the one and only transform API entry point.

Once CanvasState derived classes implement the TransformChanged, they would no longer need the Native* transform methods, and also, the ICanvas derived classes would reroute the rotate, translate and scale, directly to the base class

there's two advantages of this approach:

• a more consistent API design.
• CanvasState derived classes could be greatly simplified and streamlined.

[jonlipsky]

@vpenades The original intent of CanvasState is so that we can store values (sometimes differing on a per-platform basis) so that we can make sure that SaveState() and RestoreState() behave consistently across platforms. Additionally, in the original version of this library, there are quite a few of additional higher level graphics features added on that I didn't bring down to Maui.Graphics, which relied on the data in CanvasState.

To be clear though, CanvasState is intended only for internal use by the core library, and any implementation for a specific graphics platform. An end user should never use it.

Since it's now needed for less, I agree we can simplify what it's doing.

[vpenades]

@jonlipsky If CanvasState is intended to be used internally, then I believe ICanvas should not expose a property pointing to it, just for the sake of not misleading developers.

Regarding the simplification, the issue I have with is with the Rotate,Scale,Translate and ConcatenateTransform.

First of all, I think they all should be named Concatenate* (as in ConcatenateRotation, ConcatenateTranslation, etc), just to clarify the opperation is concatenated and does not override the current transform state.

Then, the other issue is with the path followed by these calls, which I believe is:

ICanvas.Rotate => Native.Canvas.Rotate => Native.CanvasState.Rotate

So Both NativeCanvas and NativeCanvasState need to mirror all the transform operations.

Also, following that path, after you've set the initial state of CanvasState's transform, it is not updated anymore because all the changes happen in the native implementation and not the base class.

What I was proposing was soemthing along these lines:

ICanvas.Rotate => Base.Canvas.Rotate => Base.CanvasState.SetTransform => NativeCanvasState.TransformChanged

So the CanvasState class would have more control, all the ICanvas transform changes would go through the underlaying CanvasState, and propagated to the derived class (the native canvasState) using the TransformChanged method.

Also, the RestoreState would have to set the CanvasState's transform state back.

So I believe it would make sense that CanvasState base class to have virtual SaveState and RestoreState methods, overriden by the derived classes.

I think this would have these advantages:

• The whole API would be more consistent and easier to maintian.
• Implementations of CanvasState could be simplified because they would only need to override the TransformChanged method. No need to support scale/rotate/concatenate stuff because that would be provided by the base class.

[jonlipsky]

I agree, those are all good changes and make for a more general purpose library. For my use cases, which are 100% in the minority, where I need that level of granularity for a few performance optimizations, I can wrap ICanvas.

Again, I'm really thankful for the collaboration with you!

[vpenades]

@jonlipsky Then, I could open a new PR with those changes:

Align ICanvas methods:

• ConcatenateScale
• ConcatenateRotation
• ConcatenateTranslation
• ConcatenateTransform

Add virtual SaveState and RestoreState methods to CanvasState

And reroute some of the derived functionality throgh the base class (regarding the transform) to simplify derived classes.

[jonlipsky]

@vpenades Yes! That would be greatly appreciated.

[vpenades]

@jonlipsky I've noticed that only GDICanvasState have Save/RestoreState methods, and they seem to be called from nowhere, and the actual "state" is handled on AbstractCanvas.... so the ones of GDICanvasState are leftovers, am I right?

[vpenades]

Okey... what I tried, and failed to do:

SRTX = Scale, Rotation, Translation, Transform

The idea I had was to move the abstract void NativeSRTX methods defined in AbstractCanvas directly into CanvasState , and if I had succeeded, the path would have changed from:

ICanvas.Rotate => AbstractCanvas.Rotate =>  GDICanvas.NativeRotate => GDICanvasState.NativeRotate

to:

ICanvas.Rotate => AbstractCanvas.Rotate => CanvasState.Rotate => GDICanvasState.NativeRotate

The main difference is that AbstractCanvas derived classes would not need to implement redirect method calls for NativeSRTX , and it would enforce derived classes of CanvasState to implement NativeSRTX methods consistently. Because right now it's messy: in some places the methods are called Native* in others Xaml* , etc

The problem I've found is that in some cases, the native transform is set in the CanvasState derived class (GDICanvasState), and in others, it is set in the AbstractCanvas derived class (SKiACanvas)

These implementation differences between backends would require a huge refactoring before even attempting improving the API

Personally, I would get rid of all the concatente Rotate/Scale/Translate, and at low level, that is, AbstractCanvas, CanvasState and so on, I would implement a single method: SetNativeTransform( Matrix );

As I demonstrated in CanvasState, the scale can be precisely derived from any matrix, so there's no need to drag the scaling multiplication around, as it can be derived from the current Transform. But in order for this to work, the CanvasState transform must be kept up to date with all those Native* calls.

Not sure if all this makes sense... right now I see a lot of needless complexity, although I might be wrong due to my lack of knowledge of the library