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

Add slot and box shape primitives for sketching #105

Merged
merged 24 commits into from
Sep 26, 2022
Merged

Add slot and box shape primitives for sketching #105

merged 24 commits into from
Sep 26, 2022

Conversation

chadqueen
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the enhancement New features or code improvements label Sep 19, 2022
@chadqueen
Copy link
Contributor Author

These two shapes are combinations of other shapes. Could use some thoughts about how to handle the conversion to gRPC messages. For instance, should BaseShape have a method to "GetPrimitives" where something like Circle returns self and something like Box returns 4 segments and Pill returns 2 segments and 2 arcs? Both of these are convenience methods for users as they could be constructed from the existing primitives. The value is that it groups the primitives within the new shapes for actions like translation and resizing. All feedback welcome, and if anyone would like to contribute/finish, feel free.

@RobPasMue
Copy link
Member

Hmm I like this PR! :) Let me try and work on it throughout the day! :)

@RobPasMue
Copy link
Member

Let's go with get_simple_shapes()

@RobPasMue RobPasMue self-requested a review September 20, 2022 13:27
@chadqueen chadqueen changed the title Add pill and box shape primitives for sketching Add slot and box shape primitives for sketching Sep 21, 2022
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 22, 2022
@chadqueen chadqueen marked this pull request as ready for review September 22, 2022 19:54
src/ansys/geometry/core/shapes/arc.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/circle.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/circle.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/arc.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/ellipse.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/line.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/line.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/line.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/polygon.py Show resolved Hide resolved
@RobPasMue
Copy link
Member

BTW... for creating a box, we should also be able to provide a min/max point... not only width/height

@chadqueen
Copy link
Contributor Author

BTW... for creating a box, we should also be able to provide a min/max point... not only width/height

Totally. Once we are handling 2D better within the sketch we can do this. For now I mimicked the other shapes to provide a center and parameters.

@RobPasMue
Copy link
Member

Unlinking issue to avoid its closure. Thanks @chadqueen! I'll review it first thing Monday morning

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM! Apart from my comments. I'll work on those personally. Good job @chadqueen.

src/ansys/geometry/core/shapes/arc.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/arc.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/arc.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/box.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/box.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/box.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/box.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/ellipse.py Show resolved Hide resolved
src/ansys/geometry/core/shapes/line.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/shapes/slot.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Revathyvenugopal162 Revathyvenugopal162 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MaxJPRey MaxJPRey left a comment

Choose a reason for hiding this comment

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

Looks great @chadqueen .

@RobPasMue RobPasMue enabled auto-merge (squash) September 26, 2022 09:59
@RobPasMue RobPasMue merged commit e27eaf9 into main Sep 26, 2022
@RobPasMue RobPasMue deleted the feat/box branch September 26, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New features or code improvements
Projects
None yet
4 participants