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

update polygon doc #395

Closed
wants to merge 2 commits into from
Closed

update polygon doc #395

wants to merge 2 commits into from

Conversation

MatthiasJ1
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b18af27) 95.84% compared to head (fe86f5c) 95.84%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #395   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files          24       24           
  Lines        7275     7275           
=======================================
  Hits         6973     6973           
  Misses        302      302           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gumyr
Copy link
Owner

gumyr commented Nov 27, 2023

There is no restriction to the order of the points, here is a clockwise example:

p = Polygon((0, 0), (0, 1), (1, 1), (1, 0), (0, 0), align=None)

image
Is the concern that the object is up-side-down?

@MatthiasJ1
Copy link
Contributor Author

Yes. I clarified the docs.

@gumyr
Copy link
Owner

gumyr commented Nov 30, 2023

I don't understand why you're requesting this change. Both CW and CCW orders generate polygons and the code does not attempt to enforce any order. Is there something specific that is causing a problem here?

@MatthiasJ1
Copy link
Contributor Author

MatthiasJ1 commented Nov 30, 2023

Yes, that is why I specified it is the winding order which only determines the normal, not the shape.
The code does enforce an order when it comes to the polygon's normal which is what I am documenting.

@gumyr
Copy link
Owner

gumyr commented Nov 30, 2023

How about this instead?

    Add polygon(s) defined by given sequence of points to sketch. 

    Note that the order of the points define the normal of the Face that is created in Algebra mode, 
    where counter clockwise order creates Faces with their normal being up while a clockwise order 
    will have a normal that is down.  In Builder mode, all Faces are up.

with no change to the description of pts.

@MatthiasJ1
Copy link
Contributor Author

Sounds good to me. I had to figure out the winding order by testing when lofting in algebra mode so this would address that.

gumyr added a commit that referenced this pull request Dec 1, 2023
@gumyr
Copy link
Owner

gumyr commented Dec 1, 2023

Fixed with commit f622815

@gumyr gumyr closed this Dec 1, 2023
MatthiasJ1 pushed a commit to MatthiasJ1/build123d that referenced this pull request Dec 22, 2023
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.

2 participants