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

CGAffineTransform Implementation Fix #432

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

filip-sakel
Copy link
Contributor

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.

@MaxDesiatov MaxDesiatov added bug Something isn't working documentation Technical writing, references, tutorials etc labels Jul 25, 2021
@filip-sakel
Copy link
Contributor Author

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.

@MaxDesiatov
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Technical writing, references, tutorials etc
Development

Successfully merging this pull request may close these issues.

2 participants