Fix `AffineTransform.rotate(byRadians:)` implementation. #3012

filip-sakel · 2021-07-23T05:55:47Z

This PR makes rotate(byRadians:) cumulative. The implementation is corrected; however, test coverage hasn't been added.

millenomi · 2021-08-20T19:24:58Z

@filip-sakel are you interested in adding a test for this so I can merge?

filip-sakel · 2021-08-24T19:51:01Z

I'll probably be done by the day after tomorrow because I'm also updating some legacy code.

spevans · 2021-08-27T23:17:11Z

@swift-ci test

Tests/Foundation/Tests/TestAffineTransform.swift

spevans · 2021-08-29T07:58:16Z

I ran the tests against BigSur 11.5's Foundation using the DarwinCompatibilityTests project and there was a few test failures (unless these work in Monterrey or Darwin is also buggy)?

filip-sakel · 2021-08-29T12:13:38Z

OK, I looked at the failing tests and these seem to be the issues:

Point/Size transform mismatch
I erroneously assumed that since both are vectors their transformations should be equivalent — whereas Size shouldn’t be translated; I’ll fix that.
Combination Tests
I think the tests are correct here; the issue seems to be that Darwin Foundation prepends new transformations rather than appending them. For example, inverting the order of operations in either check in testTranslationScaling() yields only the point/size error.
Hashing (EDIT)
The tests iterate over an array of unique samples testing if the hash value between the same sample matches and if it differs from the other samples; the latter seems to be failing on Darwin.

filip-sakel · 2021-08-31T11:43:44Z

I fixed the size transformation; let me know if there's any other issue.

MaxDesiatov · 2021-09-05T07:50:16Z

@swift-ci please test

filip-sakel · 2021-09-06T07:19:32Z

There seems to be a Swift test failure on linux. I fetched apple:main, so the problem should be fixed.

@MaxDesiatov could you please try another CI validation?

MaxDesiatov · 2021-09-06T08:16:08Z

@swift-ci please test

MaxDesiatov · 2021-09-06T11:36:45Z

The error is caused by TestAffineTransform.swift:52:17: error: cannot find type 'CGVector' in scope

filip-sakel · 2021-09-07T10:32:59Z

Thanks, Max; I didn't notice this error on the first CI build.

I've updated the code to use a new Vector type instead of the Darwin-only CGVector.

MaxDesiatov · 2021-09-07T10:33:42Z

@swift-ci please test

AtariDreams · 2022-01-25T18:28:43Z

Any update on this?

millenomi · 2022-01-25T19:06:13Z

@swift-ci please test and merge

millenomi · 2022-01-25T19:06:22Z

It fell through the cracks, but it was good.

MaxDesiatov · 2022-01-26T09:10:35Z

@swift-ci please test macOS platform

MaxDesiatov · 2022-01-26T15:25:53Z

The macOS CI failure is caused by the this assertion in the compiler:

1.	Swift version 5.7-dev (LLVM 5ea1e5e020828d8, Swift ac875d00f04126d)
2.	Compiling with the current language version
3.	While evaluating request ExecuteSILPipelineRequest(Run pipelines { PrepareOptimizationPasses, EarlyModulePasses, HighLevel,Function+EarlyLoopOpt, HighLevel,Module+StackPromote } on SIL for SwiftFoundation)
4.	While running pass #2144137 SILModuleTransform "SerializeSILPass".
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  swift-frontend           0x0000000109236cf7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39
1  swift-frontend           0x0000000109235c18 llvm::sys::RunSignalHandlers() + 248
2  swift-frontend           0x0000000109237340 SignalHandler(int) + 288
3  libsystem_platform.dylib 0x00007fff20447d7d _sigtramp + 29
4  swift-frontend           0x000000010bcd4000 llvm::CSR_Altivec_SaveList + 319024
5  libsystem_c.dylib        0x00007fff20357411 abort + 120
6  libsystem_c.dylib        0x00007fff203567e8 err + 0
7  swift-frontend           0x00000001097bcb33 swift::ModuleDecl::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const (.cold.1) + 35
8  swift-frontend           0x00000001054ae652 swift::ModuleDecl::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 882
9  swift-frontend           0x00000001046bcbd4 swift::ModuleFile::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) + 36
10 swift-frontend           0x00000001054ae34f swift::ModuleDecl::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 111
11 swift-frontend           0x00000001054ae5fa swift::ModuleDecl::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 794
12 swift-frontend           0x0000000104415af0 swift::getTopLevelDeclsForDisplay(swift::ModuleDecl*, llvm::SmallVectorImpl<swift::Decl*>&) + 32
13 swift-frontend           0x00000001041d4e56 swift::ide::api::SwiftDeclCollector::lookupVisibleDecls(llvm::ArrayRef<swift::ModuleDecl*>) + 134
14 swift-frontend           0x00000001041d8886 swift::ide::api::dumpModuleContent(swift::ModuleDecl*, llvm::StringRef, bool) + 230
15 swift-frontend           0x00000001046e304c swift::serialize(llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::SerializationOptions const&, swift::symbolgraphgen::SymbolGraphOptions const&, swift::SILModule const*, swift::fine_grained_dependencies::SourceFileDepGraph const*) + 620
16 swift-frontend           0x00000001041e7ff9 bool llvm::function_ref<bool (swift::fine_grained_dependencies::SourceFileDepGraph&&)>::callback_fn<performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*)::$_26::operator()() const::'lambda'(swift::fine_grained_dependencies::SourceFileDepGraph&&)>(long, swift::fine_grained_dependencies::SourceFileDepGraph&&) + 41
17 swift-frontend           0x0000000105451c87 swift::fine_grained_dependencies::withReferenceDependencies(llvm::PointerUnion<swift::ModuleDecl const*, swift::SourceFile const*>, swift::DependencyTracker const&, llvm::StringRef, bool, llvm::function_ref<bool (swift::fine_grained_dependencies::SourceFileDepGraph&&)>) + 151
18 swift-frontend           0x00000001041e7f7d std::__1::__function::__func<performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*)::$_26, std::__1::allocator<performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*)::$_26>, void ()>::operator()() + 285
19 swift-frontend           0x00000001042ef2e4 swift::SILModule::serialize() + 36
20 swift-frontend           0x0000000104c11c71 SerializeSILPass::run() + 145
21 swift-frontend           0x0000000104ab3fe1 swift::SILPassManager::runModulePass(unsigned int) + 689
22 swift-frontend           0x0000000104ab912a swift::SILPassManager::execute() + 634
23 swift-frontend           0x0000000104ab0d58 swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 72
24 swift-frontend           0x0000000104ab0cf3 swift::ExecuteSILPipelineRequest::evaluate(swift::Evaluator&, swift::SILPipelineExecutionDescriptor) const + 51
25 swift-frontend           0x0000000104ad201d swift::SimpleRequest<swift::ExecuteSILPipelineRequest, std::__1::tuple<> (swift::SILPipelineExecutionDescriptor), (swift::RequestFlags)1>::evaluateRequest(swift::ExecuteSILPipelineRequest const&, swift::Evaluator&) + 29
26 swift-frontend           0x0000000104abbee1 llvm::Expected<swift::ExecuteSILPipelineRequest::OutputType> swift::Evaluator::getResultUncached<swift::ExecuteSILPipelineRequest>(swift::ExecuteSILPipelineRequest const&) + 241
27 swift-frontend           0x0000000104ab0f84 swift::executePassPipelinePlan(swift::SILModule*, swift::SILPassPipelinePlan const&, bool, swift::irgen::IRGenModule*) + 68
28 swift-frontend           0x0000000104abf0f7 swift::runSILOptimizationPasses(swift::SILModule&) + 151
29 swift-frontend           0x00000001042492c0 swift::CompilerInstance::performSILProcessing(swift::SILModule*) + 688
30 swift-frontend           0x00000001041e42dd performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*) + 637
31 swift-frontend           0x00000001041e3eea swift::performCompileStepsPostSema(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 2218
32 swift-frontend           0x00000001041e5b99 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3641
33 swift-frontend           0x0000000104108047 swift::mainEntry(int, char const**) + 503
34 libdyld.dylib            0x00007fff2041df3d start + 1
error: Abort trap: 6 (in target 'SwiftFoundation' from project 'Foundation')

MaxDesiatov · 2022-02-01T20:16:30Z

Please test with following PRs:
swiftlang/swift#41115

@swift-ci please test macOS platform

MaxDesiatov · 2022-02-02T20:48:43Z

@millenomi would you mind if this is merged?

millenomi · 2022-02-16T19:56:11Z

Only after swiftlang/swift#41115 is merged.

millenomi · 2022-02-16T20:19:39Z

Per discussion: other patches have gone in, do not block on 41115. Merging.

millenomi · 2022-02-16T20:20:07Z

(The checks on the last round weren't done, but checks prior to that were green and there were no patch changes.)

Fix rotate(byRadians:) implementation.

6e8f8e5

filip-sakel added 4 commits August 28, 2021 01:22

Restructure AffineTransform.

0a3112a

Expand and refine AffineTransform tests.

5a80d80

Update AffineTransform implementation status.

3722028

[AffineTransform] Add Foundation testable import.

b965b14

filip-sakel marked this pull request as ready for review August 27, 2021 22:38

spevans reviewed Aug 29, 2021

View reviewed changes

Tests/Foundation/Tests/TestAffineTransform.swift Show resolved Hide resolved

spevans reviewed Aug 29, 2021

View reviewed changes

Tests/Foundation/Tests/TestAffineTransform.swift Outdated Show resolved Hide resolved

spevans reviewed Aug 29, 2021

View reviewed changes

Tests/Foundation/Tests/TestAffineTransform.swift Outdated Show resolved Hide resolved

spevans reviewed Aug 29, 2021

View reviewed changes

Tests/Foundation/Tests/TestAffineTransform.swift Outdated Show resolved Hide resolved

Apply code-review fixes.

4ea13f4

filip-sakel added 3 commits August 29, 2021 15:19

Remove debug code from testTranslationScaling().

db47073

Fix ns-testable-import #if-directive code.

127f75f

[AffineTransform] Fix size transformation.

3a31d34

[AffineTransform] Explain the operation calculations.

cfed8d0

filip-sakel mentioned this pull request Sep 4, 2021

CGAffineTransform Implementation Fix TokamakUI/Tokamak#432

Draft

Merge branch 'apple:main' into main

b7f0e55

filip-sakel added 2 commits September 7, 2021 13:25

[AffineTransform Tests] Add platform-independent Vector type.

af044a2

Merge remote-tracking branch 'refs/remotes/origin/main'

5fbc650

MaxDesiatov requested review from spevans and millenomi September 10, 2021 09:37

millenomi merged commit cc90ac5 into apple:main Feb 16, 2022

filip-sakel mentioned this pull request Apr 12, 2022

[SR-3941] AffineTransform.rotate(byRadians:) can be wrong #4105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `AffineTransform.rotate(byRadians:)` implementation. #3012

Fix `AffineTransform.rotate(byRadians:)` implementation. #3012

filip-sakel commented Jul 23, 2021

millenomi commented Aug 20, 2021

filip-sakel commented Aug 24, 2021

spevans commented Aug 27, 2021

spevans commented Aug 29, 2021 •

edited

Loading

filip-sakel commented Aug 29, 2021 •

edited

Loading

filip-sakel commented Aug 31, 2021

MaxDesiatov commented Sep 5, 2021

filip-sakel commented Sep 6, 2021

MaxDesiatov commented Sep 6, 2021

MaxDesiatov commented Sep 6, 2021

filip-sakel commented Sep 7, 2021

MaxDesiatov commented Sep 7, 2021

AtariDreams commented Jan 25, 2022

millenomi commented Jan 25, 2022

millenomi commented Jan 25, 2022

MaxDesiatov commented Jan 26, 2022

MaxDesiatov commented Jan 26, 2022

MaxDesiatov commented Feb 1, 2022

MaxDesiatov commented Feb 2, 2022

millenomi commented Feb 16, 2022

millenomi commented Feb 16, 2022

millenomi commented Feb 16, 2022

Fix AffineTransform.rotate(byRadians:) implementation. #3012

Fix AffineTransform.rotate(byRadians:) implementation. #3012

Conversation

filip-sakel commented Jul 23, 2021

millenomi commented Aug 20, 2021

filip-sakel commented Aug 24, 2021

spevans commented Aug 27, 2021

spevans commented Aug 29, 2021 • edited Loading

filip-sakel commented Aug 29, 2021 • edited Loading

filip-sakel commented Aug 31, 2021

MaxDesiatov commented Sep 5, 2021

filip-sakel commented Sep 6, 2021

MaxDesiatov commented Sep 6, 2021

MaxDesiatov commented Sep 6, 2021

filip-sakel commented Sep 7, 2021

MaxDesiatov commented Sep 7, 2021

AtariDreams commented Jan 25, 2022

millenomi commented Jan 25, 2022

millenomi commented Jan 25, 2022

MaxDesiatov commented Jan 26, 2022

MaxDesiatov commented Jan 26, 2022

MaxDesiatov commented Feb 1, 2022

MaxDesiatov commented Feb 2, 2022

millenomi commented Feb 16, 2022

millenomi commented Feb 16, 2022

millenomi commented Feb 16, 2022

Fix `AffineTransform.rotate(byRadians:)` implementation. #3012

Fix `AffineTransform.rotate(byRadians:)` implementation. #3012

spevans commented Aug 29, 2021 •

edited

Loading

filip-sakel commented Aug 29, 2021 •

edited

Loading