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

Additional constraints #975

Merged
merged 23 commits into from
Feb 15, 2022
Merged

Additional constraints #975

merged 23 commits into from
Feb 15, 2022

Conversation

adam-urbanczyk
Copy link
Member

@adam-urbanczyk adam-urbanczyk commented Feb 1, 2022

  • simplification of the assy solver
  • validation
  • PointOnLine
  • FixedPoint
  • FixedAxis
  • FixedRotation
  • docs

@adam-urbanczyk
Copy link
Member Author

I did not add the new constraints yet, but @marcus7070 @lorenzncode could you take a look to see if the reworked version is more readable?

@adam-urbanczyk adam-urbanczyk linked an issue Feb 1, 2022 that may be closed by this pull request
4 tasks
@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #975 (6fda0b8) into master (ed8e62c) will increase coverage by 0.08%.
The diff coverage is 96.39%.

❗ Current head 6fda0b8 differs from pull request most recent head 723ed4f. Consider uploading reports for the commit 723ed4f to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #975      +/-   ##
==========================================
+ Coverage   96.17%   96.26%   +0.08%     
==========================================
  Files          40       40              
  Lines        9292     9341      +49     
  Branches     1109     1103       -6     
==========================================
+ Hits         8937     8992      +55     
+ Misses        208      205       -3     
+ Partials      147      144       -3     
Impacted Files Coverage Δ
cadquery/occ_impl/solver.py 95.88% <94.49%> (+1.59%) ⬆️
cadquery/assembly.py 95.00% <97.36%> (+1.53%) ⬆️
tests/test_assembly.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed8e62c...723ed4f. Read the comment docs.

cadquery/occ_impl/solver.py Outdated Show resolved Hide resolved
cadquery/occ_impl/solver.py Show resolved Hide resolved
cadquery/occ_impl/solver.py Outdated Show resolved Hide resolved
tests/test_assembly.py Show resolved Hide resolved
Copy link
Member

@marcus7070 marcus7070 left a comment

Choose a reason for hiding this comment

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

This looks fantastic @adam-urbanczyk, thanks so much for doing it!

cadquery/assembly.py Outdated Show resolved Hide resolved
doc/assy.rst Outdated Show resolved Hide resolved
cadquery/occ_impl/solver.py Outdated Show resolved Hide resolved
cadquery/occ_impl/solver.py Show resolved Hide resolved
cadquery/assembly.py Outdated Show resolved Hide resolved
cadquery/assembly.py Outdated Show resolved Hide resolved
@adam-urbanczyk
Copy link
Member Author

Ok, I think that this is it @jmwright @lorenzncode @marcus7070 . The only thin I don't really like is the automatic fixed constraint handling. Maybe in the future (not in this PR) we should require an explicit Fixed constraint being specified.

Copy link
Member

@lorenzncode lorenzncode left a comment

Choose a reason for hiding this comment

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

I've added a few minor suggestions for docs, error messages.

cadquery/occ_impl/solver.py Outdated Show resolved Hide resolved
cadquery/occ_impl/solver.py Outdated Show resolved Hide resolved
doc/assy.rst Outdated Show resolved Hide resolved
doc/assy.rst Outdated Show resolved Hide resolved
Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for all the work @adam-urbanczyk

I'm relying on @marcus7070 and @lorenzncode quite a bit for this review since it seems that they understand the subtleties of the constraint system better than I do.

Co-authored-by: Lorenz <[email protected]>
Co-authored-by: Jeremy Wright <[email protected]>
@adam-urbanczyk
Copy link
Member Author

Great, I'll merge tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional constraints
4 participants