Tokamak icon indicating copy to clipboard operation
Tokamak copied to clipboard

`CGAffineTransform` Implementation Fix

Open filip-sakel opened this issue 2 years ago • 2 comments

In this PR:

  1. The rotated(by:), scaledBy(x:y:), and translatedBy(x:y:) operators are made cumulative;
  2. The implementation of inverted() is corrected to return self when the matrix is not invertible;
  3. The implementation of concatenating(_:) is simplified; and
  4. The documentation of matrix entries is improved.

Further work still needs to be done; namely:

  1. The implementation of the aforementioned operators could be optimized (by not using concatenations) — of course, that should be properly documented;
  2. The optimizations in the implementation of inverted() should be explained;
  3. SIMD could optimize concatenations, inversions, and the other transformation operations; and,
  4. Unit tests should be added to help catch bugs and future regressions.

filip-sakel avatar Jul 19 '21 23:07 filip-sakel

Update

  1. The foundation patch has been corrected and now there's substantial test support for the non-NS AffineTransform; it is currently awaiting review.
  2. Tokamak now implements CGAffineTransform as a wrapper around Foundation.AffineTransform; it has its own tests, nonetheless. This means that until the Foundation patch is merged, tests will fail on this branch.
  3. Since we rely on Foundation's implementation, there's no need to document the operation — that's covered by the patch.
  4. The "optimized” SIMD versions were benchmarked (with Google Benchmarks), but found to be less efficient in some cases, especially in release builds, where they offer little —to no— performance boost.

filip-sakel avatar Sep 04 '21 15:09 filip-sakel

Your Foundation patch has landed in Swift 5.7, so I hope we'll be able to update the PR later this year when that Swift version and its corresponding SwiftWasm version are released.

MaxDesiatov avatar May 28 '22 19:05 MaxDesiatov