KittyCAD / modeling-app

The KittyCAD modeling app.
https://kittycad.io/modeling-app/download
MIT License
412 stars 35 forks source link

KCL: Polygon function #4261

Closed adamchalmers closed 1 week ago

adamchalmers commented 2 weeks ago

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

guptaarnav commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago
  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 commented 1 week ago

@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 commented 1 week ago
  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 commented 1 week ago

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 commented 1 week ago

"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 commented 1 week ago

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!

lf94 commented 1 week ago

Closed by Polygon stdlib function #4300