Tokamak
Tokamak copied to clipboard
TokamakUI
Implementation of `CGAffineTransform `
Is the implementation of the non-apple CGAffineTransform correct?
Several things seem "off". For one, the implementation of inverted() simply negates the matrix's components. On other implementations, however, such as the Foundation and SwiftWin32 ones, it's much more complex — calculating a determinant, examining if inversion is possible, etc. Furthermore, rotated(by:) — despite claiming to rotate an existing transformation — ignores the translation components of the matrix (tx and ty) and existing scaling and rotation adjustments.
Perhaps, the current return value of rotated(by:) was intended to be appended to self before being returned.
If this is indeed incorrect, I'd be happy to submit a PR to fix it and add a test coverage — although tests in Tokamak seem to cover higher-level rendering behavior.
Could we just add a typealias CGAffineTransform = Foundation.AffineTransform?
I don't see why not? Ideally we want some tests that prove it would help though.
And tests that cover both low level stuff (the canonical meaning of "unit tests") and high level stuff are great. More tests on both sides is better 👍
To fully clarify, just making sure that CGAffineTransform and Foundation.AffineTransform are equivalent on all platforms both from API perspective (this would be confirmed by the plain build not failing on any platform) and functionally (some basic unit tests) should be enough.
High level snapshot tests would only be needed for transform modifiers, if we expect them to be impacted by this change.
It looks like Foundation.AffineTransform has a pretty different API compared to CGAffineTransform. So, I ended up using the same implementation but packaged in the CGAffineTransform API. Here's the PR.
Note: I suspect Foundation's implementation of inverted() is incorrect. So, the actual implementation is similar to Draw2d's Inverse() function.
Is there anything available on bugs.swift.org or a radar for inverted() incorrectness? Maybe we should also submit a PR to upstream Foundation?