Initial commit of extrusions plugin for code review #25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martinbudden
changed the title
|
Changed to use |
|
@martinbudden I assume that by "incomplete initial submission" you mean that it doesn't include an exhaustive list of profiles that it can generate. This is ready for review as-is, correct? |
|
@jmwright , sorry, should have been more clear. Yes this is ready for review. When I said incomplete, I was mainly referring to the documentation - that's still not fully complete, but it's almost there. As for the types of extrusions, I don't plan to add any fore for this submission, though I will probably add some more in future. I'm particularly keen for you to review the style, and see if it fits in with cadquery style. I'm also not really a python programmer, so if there are any python-related issues it would be good if those were pointed out. |
jmwright
reviewed
Sep 2, 2021
jmwright
reviewed
Sep 2, 2021
plugins/extrusions/README.md
Outdated
| # Extrusions | ||
|
|
||
| Aluminium extrusions in a variety of sizes in both standard and v-slot profiles. | ||
| Currently e2020, e2040, e2080, e4040, v2020, v2040, and v4040 extrusions are available. |
There was a problem hiding this comment.
V stands for V-slot I assume. What would e stand for? Is it some "standardized" extrusion type. For sure there are more, but I know of Bosch-type and Item-type extrusions.
There was a problem hiding this comment.
E just stands for extrusion, meaning a generic type. It's the same abbreviation I used when I implemented extrusions in NopSCADlib, see https://github.com/nophead/NopSCADlib#Extrusions .
Someone might implement the B and I types in future, if the exact profile was important for their project, but for most projects a generic form is perfectly acceptable (and I have found variations in the profile form different suppliers, especially the size of the center hole and the channel width).
There was a problem hiding this comment.
Maybe good to state somewhere what is the source of this "generic" extrusion. I assume you did not just imagine the dimensions yourself.
There was a problem hiding this comment.
OK, I'll state the original source.
jmwright
reviewed
Sep 2, 2021
jmwright
reviewed
Sep 3, 2021
marcus7070
approved these changes
Sep 11, 2021
There was a problem hiding this comment.
Looks good! I have some suggestions but they are optional.
I also note that if this is pip installed without -e then it does not install the data directory containing the profiles. Which isn't a problem, I just can't understand what an editable install has to do with the package data. 😠
| @@ -0,0 +1,66 @@ | |||
| from plugins.extrusions import extrusions | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
You might want to do some stuff with BoundingBox here to check that the length and centering arguments are doing what you expect.
plugins/extrusions/extrusions.py
Outdated
Comment on lines
11
to
147
| extrusion = profile.toPending()._extrude(length).move(cq.Location(offset)) | ||
| return cq.Workplane("XY").newObject([extrusion]) | ||
|
|
||
|
|
||
| def e2020(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 2020 extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([20, 20], "e2020.dxf", length, centered) | ||
|
|
||
|
|
||
| def e2040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 2040 extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([40, 20], "e2040.dxf", length, centered) | ||
|
|
||
|
|
||
| def e2080(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 2080 extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([80, 20], "e2080.dxf", length, centered) | ||
|
|
||
|
|
||
| def e4040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 4040 extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([40, 40], "e4040.dxf", length, centered) | ||
|
|
||
|
|
||
| def v2020(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 2020 v-slot extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([20, 20], "v2020.dxf", length, centered) | ||
|
|
||
|
|
||
| def v2040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 2040 v-slot extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([40, 20], "v2040.dxf", length, centered) | ||
|
|
||
|
|
||
| def v4040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | ||
| """ | ||
| Return a 4040 v-slot extrusion with the specified length | ||
|
|
||
| :param length: length of the extrusion | ||
| :type length: float > 0 | ||
| :param centered: If True, the extrusion will be centered around the reference point. | ||
| If False, the corner of the extrusion will be on the reference point and it will | ||
| extend in the positive x, y and z directions. Can also use a 3-tuple to | ||
| specify centering along each axis. | ||
| """ | ||
| return _extrusion([40, 40], "v4040.dxf", length, centered) |
There was a problem hiding this comment.
I had an idea for how to reduce the repetition here. From a user perspective this doesn't change anything, it's just a refactoring.
If you find this too complicated just to reduce some minor repetition that's completely justified and you are welcome to ignore this suggestion.
Suggested change
| def _extrusion( | |
| size: Tuple[float, float], | |
| profile, | |
| length: float, | |
| centered: Union[bool, Tuple[bool, bool, bool]], | |
| ) -> T: | |
| """ | |
| Return an extrusion with the specified profile and length | |
| :param size: size of the extrusion, used for centering | |
| :param profile: filename of the extrusion's profile | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| # importlib.resources is favored over pkg_resources, but not yet available in cadquery, | |
| # see https://setuptools.readthedocs.io/en/latest/pkg_resources.html | |
| # file = ( | |
| # importlib.resources.files("plugins.extrusions") | |
| # .joinpath("profiles/") | |
| # .joinpath(profile) | |
| # ) | |
| file = pkg_resources.resource_filename(__name__, "profiles/" + profile) | |
| profile = importers.importDXF(file).wires() | |
| if isinstance(centered, bool): | |
| centered = (centered, centered, centered) | |
| offset = cq.Vector( | |
| 0 if centered[0] else size[0] / 2, | |
| 0 if centered[1] else size[0] / 2, | |
| -length / 2 if centered[2] else 0, | |
| ) | |
| extrusion = profile.toPending()._extrude(length).move(cq.Location(offset)) | |
| return cq.Workplane("XY").newObject([extrusion]) | |
| def e2020(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 2020 extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([20, 20], "e2020.dxf", length, centered) | |
| def e2040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 2040 extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([40, 20], "e2040.dxf", length, centered) | |
| def e2080(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 2080 extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([80, 20], "e2080.dxf", length, centered) | |
| def e4040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 4040 extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([40, 40], "e4040.dxf", length, centered) | |
| def v2020(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 2020 v-slot extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([20, 20], "v2020.dxf", length, centered) | |
| def v2040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 2040 v-slot extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([40, 20], "v2040.dxf", length, centered) | |
| def v4040(length: float, centered: Union[bool, Tuple[bool, bool, bool]] = True) -> T: | |
| """ | |
| Return a 4040 v-slot extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference point. | |
| If False, the corner of the extrusion will be on the reference point and it will | |
| extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| return _extrusion([40, 40], "v4040.dxf", length, centered) | |
| def _extrusion( | |
| size: Tuple[float, float], | |
| filename: str, | |
| doc_name: str, | |
| ) -> Callable: | |
| """ | |
| Return a function that will in turn generate the standard extrusion. | |
| :param size: size of the extrusion, used for centering | |
| :param filename: filename of the extrusion's profile | |
| :param doc_name: a string that will be inserted into a docstring template and assigned | |
| to __doc__ on the returned function | |
| """ | |
| file = pkg_resources.resource_filename(__name__, "profiles/" + filename) | |
| profile = importers.importDXF(file).wires() | |
| docstring = f""" | |
| Return a {doc_name} extrusion with the specified length | |
| :param length: length of the extrusion | |
| :type length: float > 0 | |
| :param centered: If True, the extrusion will be centered around the reference | |
| point. If False, the corner of the extrusion will be on the reference point and | |
| it will extend in the positive x, y and z directions. Can also use a 3-tuple to | |
| specify centering along each axis. | |
| """ | |
| def rv(length: float, centered: Union[bool, Tuple[bool, bool, bool]]) -> T: | |
| if isinstance(centered, bool): | |
| centered = (centered, centered, centered) | |
| offset = Vector( | |
| 0 if centered[0] else size[0] / 2, | |
| 0 if centered[1] else size[0] / 2, | |
| -length / 2 if centered[2] else 0, | |
| ) | |
| extrusion = profile.toPending()._extrude(length).move(Location(offset)) | |
| return cq.Workplane("XY").newObject([extrusion]) | |
| rv.__doc__ = docstring | |
| return rv | |
| e2020 = _extrusion([20, 20], "e2020.dxf", "2020") | |
| e2040 = _extrusion([40, 20], "e2040.dxf", "2040") | |
| e2080 = _extrusion([80, 20], "e2080.dxf", "2080") | |
| e4040 = _extrusion([40, 40], "e4040.dxf", "4040") | |
| v2020 = _extrusion([20, 20], "v2020.dxf", "2020 v-slot") | |
| v2040 = _extrusion([40, 20], "v2040.dxf", "2040 v-slot") | |
| v4040 = _extrusion([40, 40], "v4040.dxf", "4040 v-slot") |
Note that you can't use evaluated strings in a function docstring, ie.
s = "a string"
def foo():
f"""
a function relating to {s}.
"""
passWill actually result in foo.__doc__ being None. So instead I modify the docstring after creating the function.
There was a problem hiding this comment.
OK, I'll look into making that change.
|
Thanks @martinbudden Sorry it took so long for me to review this. A couple of PRs got missed in this repo. |
|
Sorry, I got overzealous on merging this one. I see there is still outstanding discussion items.I have reverted the merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an incomplete initial submission for code review.
It addresses issue #3 [Plugin Request] Aluminum Profile Generator