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

Implement Path.addArc(tangent1End:...). #526

Closed
wants to merge 0 commits into from

Conversation

filip-sakel
Copy link
Contributor

The addArc(tangent1End:tangent2End:radius:transform:) method was left unimplemented and added nothing to the path components. This implementation uses some vector arithmetic on CGPoint to create an arc. There are probably better approaches out there, but I think this a good starting point.

@filip-sakel
Copy link
Contributor Author

Swiftlint is complaining about using a = a + b, but there’s no reason to implement += just for these operations. I don't know how I can disable this check for the places where this is used.

Copy link
Member

@carson-katri carson-katri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a snapshot test?

Sources/TokamakCore/Shapes/Path/PathMutations.swift Outdated Show resolved Hide resolved
@filip-sakel
Copy link
Contributor Author

filip-sakel commented Nov 7, 2022

I saw how other snapshot tests are written and think I understand how to write one, but I'm not sure how I can generate a snapshot to compare against.

I'll add the snapshot tests later in the week.

@carson-katri carson-katri added the enhancement New feature or request label Nov 7, 2022
@carson-katri
Copy link
Member

I think you could just run the test and copy the image it writes in, then compare against that going forward. Layout tests compare directly against a rendered SwiftUI view.

@filip-sakel
Copy link
Contributor Author

I realized that my code doesn't really choose the start-end/clockwise combinations correctly. This means that the start of the arc may be on the second tangent and the arc's end will be on the first tangent.

So, I'll keep working on this to get everything exactly like the native implementations.

@filip-sakel
Copy link
Contributor Author

I fixed the issue and added some more tests. However, I was unable to get the project to build so I could run the snapshot tests. I run the following according to the contribution guide to no avail:

swift build --product TokamakPackageTests && `xcrun --find xctest` .build/debug/TokamakPackageTests.xctest

I’m not sure why, but there seems to be a problem with core-graphics types, such as CGLineJoin and CGPoint. @carson-katri could you add the snapshot tests?

@@ -65,6 +65,123 @@ final class ShapeRenderingTests: XCTestCase {
timeout: defaultSnapshotTimeout
)
}

func testTangentArc() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running this test, the call stack overflows in getArc where it gets the second chunk:


It seems to make sense, as the chunk1 angle is created by adding .pi / 2 to the startAngle, which the call to getArc then checks and sees the value is greater than .pi / 2.

Personally I'm not sure what the solution to this is, I'm hoping you have some ideas 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants