-
Notifications
You must be signed in to change notification settings - Fork 25
Adds support for the extrusion capability to the CadDocument class.
#804
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
base: main
Are you sure you want to change the base?
Conversation
|
Integration tests report: appsharing.space |
|
Preview PR at appsharing.space |
arjxn-py
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work @asmith26, just have some minor comments otherwise it looks pretty good to me. Thanks :)
| self.set_visible(shape2, False) | ||
| return self.add_object(OBJECT_FACTORY.create_object(data, self)) | ||
|
|
||
| def extrude( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the name "extrude"
|
|
||
| def extrude( | ||
| self, | ||
| name: str = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find it ideal to have empty string for name as fallback - maybe something else like "extruded" or "extruded shape"? I admit this is the situation in other functions as well so maybe we can tackle this in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought, I was about to tackle this in a seperate PR, but it actually looks like there is a solution already: https://github.com/search?q=repo%3Ajupytercad%2FJupyterCAD%20_new_name&type=code
| length_fwd: float = 10, | ||
| length_rev: float = 0, | ||
| solid: bool = False, | ||
| color: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I see the correct color when you extrude in the video clip on this PR but still it'd be nicer to use "#808080" as fallback here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks good thought - there is a fallback here which Defaults to the base object's color if None (I copied this idea from one of the other methods in this class), and if I understand correctly I think every sketch object is assigned a default color:
| "default": "#808080" |
(Apologies if I'm misunderstand this though :) )
UPDATE: I've just realised that _get_color has a fallback to use "#808080":
| return "#808080" |
| def extrude( | ||
| self, | ||
| name: str = "", | ||
| shape: str | int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when you don't provide this shape arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks good thought, I actually copied this from one of the other methods in this class too, this line uses the _get_operand method to:
"If no objects are provided as input, the last created object will be used as operand."
Was trying to decide whether to call this method "extrude" vs "extrusion" - please feel free to rename if preferable. All other thoughts also very welcome.
extrude.mp4
Thanks!