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

KCL: Polygon function #4261

Open
adamchalmers opened this issue Oct 22, 2024 · 8 comments · May be fixed by #4300
Open

KCL: Polygon function #4261

adamchalmers opened this issue Oct 22, 2024 · 8 comments · May be fixed by #4300
Labels
kcl Language and compiler features kcl-stdlib

Comments

@adamchalmers
Copy link
Collaborator

Add a polygon function to the stdlib. It should take these parameters:

  1. An object with fields {sideLength: f64, numSides: u64, center: [f64; 2]}
  2. A sketchgroup

Define this in std/shapes.rs

@nrc nrc added kcl Language and compiler features kcl-stdlib labels Oct 23, 2024
@guptaarnav
Copy link
Collaborator

guptaarnav commented Oct 23, 2024

Was working on implementing this and am getting an unexpected engine error. I am creating the polygon by computing the vertex points and extending a path to each of the vertices. My implementation of the polygon sketch works with no shown errors:
Image

There seems to be a BadRequest API error stating "The given edge has no opposite edge" when I try to extrude on this polygon sketch, see below. The polygon still successfully shows the extrusion as expected, but this error comes up and this same error is also causing cargo nextest tests to fail for my polygon test cases. I wasn't able to find any documentation or discussion in github about this error except for #3830, which vaguely alluded to the problem being in the engine in that instance.
Image. My first guess is that this error is saying that the sketch is not closed, but it appears that it is. Am I missing something?

@jtran
Copy link
Collaborator

jtran commented Oct 23, 2024

@guptaarnav, have you tried adding |> close(%) on line 7 before the extrude if it's what you said about not being closed? This just came up in #4271. But I'd expect the polygon function to close it for you.

@jtran
Copy link
Collaborator

jtran commented Oct 23, 2024

  1. An object with fields {sideLength: f64, numSides: u64, center: [f64; 2]}

What's the reason for using side length instead of radius? I guess I'm imagining wanting to keep a constant radius and change the number of sides. On the other hand, a side length parameter would need to be recomputed to keep the same overall size. But maybe that's not how people would actually use it?

@guptaarnav
Copy link
Collaborator

@guptaarnav, have you tried adding |> close(%) on line 7 before the extrude if it's what you said about not being closed? This just came up in #4271. But I'd expect the polygon function to close it for you.

@jtran I already close it in the polygon function following what circle does to close the sketch.

But I tried anyways to throw an explicit close(%) before the extrude in the kcl code and close complained ("No such object exists of type Scene::Model::Brep::Path"). Is that confirmation that the polygon sketch is closed already?

@guptaarnav
Copy link
Collaborator

guptaarnav commented Oct 23, 2024

  1. An object with fields {sideLength: f64, numSides: u64, center: [f64; 2]}

What's the reason for using side length instead of radius? I guess I'm imagining wanting to keep a constant radius and change the number of sides. On the other hand, a side length parameter would need to be recomputed to keep the same overall size. But maybe that's not how people would actually use it?

@jtran, I've usually seen two polygon buttons in CAD software, circumscribed polygon and inscribed polygon. Both use radius and numSides rather than sideLength and numSides. I agree that radius makes more sense. (also it might be weird for a user if they're messing with polygon numSides and the shape grows in correlation with the number of sides).

I think we should go with two polygon std lib functions in that case: circumscribedPolygon and inscribedPolygon ({radius: f64, numSides: u64, center: [f64; 2]})?

@adamchalmers, would you use a sideLength parameterized variant of polygon as you noted in the original Issue description?

@jtran
Copy link
Collaborator

jtran commented Oct 23, 2024

I already close it in the polygon function following what circle does to close the sketch.

But I tried anyways to throw an explicit close(%) before the extrude in the kcl code and close complained ("No such object exists of type Scene::Model::Brep::Path"). Is that confirmation that the polygon sketch is closed already?

I think so, but not sure.

I think we should go with two polygon std lib functions in that case: circumscribedPolygon and inscribedPolygon ({radius: f64, numSides: u64, center: [f64; 2]})?

What about ...

polygon({ radius: f64, numSides: u64, center: [f64; 2], inscribed: bool })

? In Rust, I'd suggest an enum over a bool.

@adamchalmers
Copy link
Collaborator Author

"No such object exists of type Scene::Model::Brep::Path" is the unfortunate error that gets returned when the object is already closed yeah.

@guptaarnav
Copy link
Collaborator

figured out why I was getting the "The given edge has no opposite edge" error from the API, it was because I was not setting unique id's on the lines. Not running into error anymore!

@guptaarnav guptaarnav linked a pull request Oct 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kcl Language and compiler features kcl-stdlib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants