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

new sketch and addition to LoftedShape #50

Merged
merged 3 commits into from
Aug 3, 2024

Conversation

LasseHolch
Copy link
Contributor

shape.py:
Possibility to create spline from multiple mid_sketches added.

spline_round.py:
Two new sketches added

Possibility to create spline from multiple mid_sketches added.

spline_round.py:
Two new sketches added
@FranzBangar
Copy link
Member

Wow, that was unexpected! Great!
Just a few thoughts before merge:

  1. You're setting u_1 and other properties in __init__(). When a sketch is transformed, will these properties update or remain as in the beginning?
  2. QuarterSplineRound and QuarterSplineRing have a lot of duplicated code. How much could be reused?
  3. Also you're adding 11 and more attributes to your new classes. Not that I'm sticking blindly to the 'max-7-attributes-rule' but I guess some could be grouped into a separate class and reused in both of the above classes?
  4. Can you add an example that demonstrates the usage of the two classes? I can update the documentation that shows blocking of these sketches.

Anyway, I'm really glad that someone has the will and eagerness to contribute! Thanks!

LasseHolch and others added 2 commits July 30, 2024 10:49
@FranzBangar
Copy link
Member

Funny, while reviewing your contributions, I found out my own hollow shapes (rings) don't work with transforms. Please stand by while I untangle all this.

@FranzBangar
Copy link
Member

Sorry, false alarm, everything seems to be in order, I mixed up traumas from the past.

Anyway, I managed to sort out most of the typing conundrums (except for the spline side edges). I replaced property setters (which generate tons of boilerplate code) and conformed to creation pattern in other sketches. Most of your code (including maths) remains the same. Please take look and let me know if you think that's okay.

Here's the commit: 0c6a59f

I'll solve the other issues next, then I'll merge it into master.
Thanks!

@FranzBangar FranzBangar merged commit 937c81f into damogranlabs:master Aug 3, 2024
@FranzBangar
Copy link
Member

Here it is. And finally, you're on the list of contributors!

@LasseHolch
Copy link
Contributor Author

Thank you so much.

@LasseHolch LasseHolch deleted the roundedRectangle branch August 5, 2024 03:57
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.

None yet

2 participants