Fix mid-path transforms

[corranwebster]

This fixes #2206.

This is needed on backends where the new Context object is responsible for keeping track of the "current path", rather than having the underlying backend drawing library do it, specifically Android, Qt and Winforms.

The basic approach is that whenever a transform is applied, we need to apply the inverse transform to the points in the path (eg. if the origin is moved to the right, that effectively moves the existing points in the path to the left). In other words, the points of the path are always transformed to be in the current coordinate system.

This happens when:

• the context is rotated, scaled or translated
• the transformation is reset
• a saved state is restored

The last requirement means we need to track the difference between the current transform and the previous transform, which is easiest to do by adding a transform attribute to the State objects that we currently have which starts out as an identity transform, and gets modified with subsequent operations on the context. This is simpler than trying to track the total combined transform and calculate the difference between saved states.

The other significant change was to make the Winforms backend to use its Save and Restore methods (implemented in 130038a and s small fix in ffecac1. With that change all three platforms have a similar approach to fixing the bug, which will hopefully make review much easier.

The test image is based on the following javascript for the HTML Canvas:

  ctx.translate(100, 100);
  let c = 0;
  for (let i = 0; i < 12; i++) {
    ctx.rect(50, 0, 10, 10);
    ctx.scale(1.05, 1.0);
    ctx.rotate(Math.PI / 6);    
  }
  ctx.fill();

  ctx.beginPath();
  ctx.moveTo(25, 0);
  for (let i = 0; i < 12; i++) {
    ctx.lineTo(25, 0);
    ctx.rotate(Math.PI / 6);
  }
  ctx.fillStyle ="cornflowerblue";
  ctx.fill();

which produces:

Finally, because we need to use inverse matrices (indeed, any approach to this problem will need to use inverses of the transformation at some point) we run into difficulty with scaling by 0 in either x or y direction (or singular transformations more generally). Right now scaling by 0 will result in an exception when computing the inverse. There may be a point in trying to fail more gracefully, but comparison needs to be made with the HTML Canvas, and I think that that's more that I should try to fit in this PR.

To handle cases of scaling by 0, this PR adds a test for for the output we would like, and updates all the backends to make sure they pass. iOS and macOS pass without modifications, but the other backends needed a change to scale by a very small quantity instead of 0. The result should usually be visually imperceptible but under some conditions the result of the drawing may be different than what is expected (eg. scaling to 0 and then scaling by a very large factor.

To Do:

• Android
• Winforms (I think this might need a re-write of the state handling).
• Investigate what resetting the transform does in the HTML Canvas - it is local to the current state; approximately equivalent to applying the inverse of the current transform.
• What to do about non-invertible transforms (particularly scaling to 0). Solution for now: Error. Improved solution: where the backend can't handle scaling by 0, scale by 2**-24 instead.

Fixes #2206.
Fixes #4110.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

No worries about restructuring a given backend — the idea of #4057 was to establish the interface-to-backend contract and get all the current tests running, with the expectation that some of things would almost certainly have to change in one way or another to then get existing bugs out / add missing features.

That said, it's frustrating how much of the state it turns out we have to track on so many backends... I had envisioned most backends being able to let their platforms handle it. I'm starting to wonder if it is worth maintaining a representation of current state in Core.

One note, specifically: at least as currently implemented, there's no need to assign to self.state. It's a property that automatically returns the latest state in the list.

[corranwebster]

That said, it's frustrating how much of the state it turns out we have to track on so many backends... I had envisioned most backends being able to let their platforms handle it. I'm starting to wonder if it is worth maintaining a representation of current state in Core.

I think the windows backend can be a lot better - it's currently managing the entire state, but I know that the underlying API has save/restore analogues. My main obstacle to a re-write is not having easy access to a windows machine (I have an old laptop which should have a windows VM on it somewhere...)

I'm not yet at the point of throwing away the native state representation on all platforms - it's nice when it Just Works and is faster than what we could manage, I think.

[HalfWhitt]

I think the windows backend can be a lot better - it's currently managing the entire state, but I know that the underlying API has save/restore analogues.

As I understand it, GraphicsState.Save does save the transform matrix, as well as all other things that Windows considers a property of the GraphicsState — but the fill style and stroke properties aren't included in that; you still have to provide them every time you call the methods. (Same goes for text font, too.)

What's more, it does have an internal stack, but you still have to maintain your own: GraphicsState.Restore requires, as an argument, the GraphicsState to which you want to restore. If you call it with a state that's not on top of the stack, it'll pop until it gets to it — but you can't call it without an argument. This seems like such a bizarre design decision to me.

So as far as I know, using the native state-handling for Windows would still involve maintaining our own list of states, each of which holds onto a Brush, a Pen, and a GraphicsState (and, eventually, a font). At that point I figured, why bother with the GraphicsState... but I suppose it's possible it may have some performance advantage over tracking the Matrix itself.

I'm not yet at the point of throwing away the native state representation on all platforms - it's nice when it Just Works and is faster than what we could manage, I think.

I really don't want to... conceptually, I really like the idea of each backend exposing a clean interface that (basically) matches the HTML canvas's. But at least at present, most backends are able to leave transformation-tracking to the backend, while literally none of them are able to leave fill/stroke properties up to it. (Cocoa and iOS could... if it weren't for needing to apply them to text, too.)

Usually, if there's logic that we're implementing in every backend, that's a sign it shouldn't live there. The fact that most platforms do track transformation is definitely a complicating factor, though...

[corranwebster]

The iOS test failure looks to be independent of this PR (it passed on the second-to-last commit.

[HalfWhitt]

The iOS test failure looks to be independent of this PR (it passed on the second-to-last commit.

That's a frustratingly intermittent bug that's just shown up in the last few days. #4107 hopefully solved it — time will tell — so I'd recommend merging main to pick that up.

[HalfWhitt]

This is impressive. The concept of applying the inverse transformation to the existing path points makes great sense when you explain it, but if I came across this code without your PR description, I'm pretty sure it would take me some head-scratching to figure out. I think a brief comment, perhaps in each transformation method, could come in very handy.

Figuring out how best to handle non-invertible transformations is definitely going to be important, but I agree that it's fine to explore that after this PR.

This definitely solves a large portion of #2206 — how mid-path transforms affect the path itself — but it doesn't (as far as I can tell) handle the effect of transforms' effects on stroke width. That's probably an entirely different set of fixes than this, so a separate PR probably makes sense, but this one shouldn't close the issue.

My only real concerns are over untested changes, namely that the test doesn't have:

• A mid-path translate
• A mid-path reset_transform — and it especially is doing new tricks here.

Testing the latter may run into #4044 — since we know resetting is already broken on Cocoa and iOS — but those can be xfailed if need be. (I probably need to start #4045 over again, after the backend refactor...)

[corranwebster]

The image produced on winforms looks OK, so I'm going to adjust the threshold for winforms and clean up.

[johnzhou721]

@HalfWhitt

This definitely solves a large portion of #2206 — how mid-path transforms affect the path itself — but it doesn't (as far as I can tell) handle the effect of transforms' effects on stroke width. That's probably an entirely different set of fixes than this, so a separate PR probably makes sense, but this one shouldn't close the issue.

FYI, AFAICT, some restructuring in the Winforms backend seems to have fixed the line width issue -- we now let WinForms keep track of and draw transforms instead of keeping track of the transforms ourselves. The latest screenshot @corranwebster posted showcases this as well.

[HalfWhitt]

Well I'll be — yeah, I see that.

I'm about to be in the midst of a move (well, more than I already am), so I'm not sure I'll be able to give this a proper review for several days. My first thought, from a glance, is that it looks great, with the one note that I'd like to see a more stretched scaling that makes the correct stroke behavior even more obvious (and easy to see it missing, in case we ever regress somehow).

If this PR is blocking other work you're itching to move forward with, perhaps Russ or Malcolm can take a look. Otherwise, I look forward to reviewing this when I can.

[corranwebster]

Well I'll be — yeah, I see that.

I don't emphasise this, but since all drawing is in the correctly transformed space, it should stretch stroke widths appropriately.

I'm about to be in the midst of a move (well, more than I already am), so I'm not sure I'll be able to give this a proper review for several days. My first thought, from a glance, is that it looks great, with the one note that I'd like to see a more stretched scaling that makes the correct stroke behavior even more obvious (and easy to see it missing, in case we ever regress somehow).

I can stretch it a bit more without too much difficulty.

If this PR is blocking other work you're itching to move forward with, perhaps Russ or Malcolm can take a look. Otherwise, I look forward to reviewing this when I can.

This isn't particularly blocking anything right now.

[mhsmith]

I think throwing an error on a singular transform is too big a regression. See #4110 for discussion.

[corranwebster]

I think this is ready for (re)-review.

[HalfWhitt]

Thank you for taking this on, and staying with it through all the discussion. The substance of this all looks good to me — I just have a couple of hairs to split.

[corranwebster]

Hopefully these two issues are addressed, so this is ready for re-review.

[HalfWhitt]

Looks great, thank you!