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

Fix JernArc not always co-planar with input plane #644

Merged
merged 10 commits into from
Jun 23, 2024

Conversation

jdegenstein
Copy link
Collaborator

This PR fixes #635 in which I determined that the localization of the tangent vector was resulting in an incorrect tangent direction. Ultimately it was discovered that this problem went to a fairly low level within build123d and is related to how affine transformations in OCCT do not affect gp_Vec, gp_Pnt, and gp_Dir in the same ways. In particular a tangent direction is analogous to a gp_Dir that should not be affected by translation or scaling, and only by the rotations. The existing Vector.transform method was extended to include an affine transformation option for gp_Dir. JernArc was reworked to depend on this newly created functionality in Vector.transform.

I also added additional tests to cover the newly added features and more tests for JernArc itself.

I am looking for input on whether this change makes architectural sense and is not overly simplistic and so would lead to problems in the future.

@jdegenstein jdegenstein changed the title Fix JernArc not always co-planar with input plane (issue https://github.com/gumyr/build123d/issues/635) Fix JernArc not always co-planar with input plane Jun 23, 2024
@gumyr
Copy link
Owner

gumyr commented Jun 23, 2024

I find the vec_dir parameter confusing. How about naming it is_direction: Bool = False?

There are many ways to fix this like introducing a Direction(Vector) class or adding a Matrix.is_direction_transformation() method that would zero out the translation and scaling elements but this solves the immediate problem so let's go with this (with the naming change if you agree).

@jdegenstein
Copy link
Collaborator Author

Thanks @gumyr for your comments! I agree with the is_direction naming change and implemented it in this PR. I agree that if this problem keeps coming up then larger solutions like class Direction(Vector) might become the better approach.

@jdegenstein jdegenstein merged commit 2cd6365 into gumyr:dev Jun 23, 2024
11 checks passed
@jdegenstein jdegenstein deleted the jernArcFix1 branch June 23, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JernArc is not always co-planar with input plane
2 participants