search

microsoft / maker.js

📐⚙ 2D vector line drawing and shape modeling for CNC and laser cutters.
http://maker.js.org
Apache License 2.0
1.76k stars 269 forks source link

Adding ArcWedge #585

Open milonic opened 7 months ago

milonic commented 7 months ago

Adding ArcWedge - A modified version of OvalArc to change the end caps to something other than a full semi circle.

milonic commented 7 months ago

@microsoft-github-policy-service agree

danmarshall commented 7 months ago

Thanks @milonic ! Impressive for a first time GitHub PR 🥇 I'll be reviewing it shortly and put it through a few paces.

danmarshall commented 7 months ago

I'm curious why there is still a capRadius, aren't the ends meant to be flat? image

milonic commented 7 months ago

Hi Dan, capRadius value is variable, just thought it would be good to have the option to set a radius. Also, have the option of an inner arc as well as an outer arc, just specify negative and positive capRadius values. You can also specify an Array as capRadius and have each end as opposites to one another.

I did want to talk to you about line vs arc within the function. From what I can tell, the return is expecting a IPathArc unless I physically specify a return of IPathLine, I couldn't seem to figure out a way to do both (typescript restriction) so the Wedge part of the arcWedge name may not be strictly true anymore.

What I'd like to do is return a Line from the addCap function if capRadius is close to being straight, otherwise return an arc but I can't seem to figure out a typescript way of doing that.

Also, to try and get a straight line, the radius does need to go quite high due to they way arc with 5 variables works. You may know a better way but I just thought the 5 variable Arc was the easiest route.

danmarshall commented 7 months ago

I think that the flat-capped version should be the ArcWedge. If someone were to want the curved caps, they ought to use OvalArc. Do you think the 'cornered' arc caps are a useful shape? I'm not sure I've seen a shape like this as a common primitive.

milonic commented 7 months ago

Hi Dan, just letting you know that I've not forgotten about this. I'm a little snowed under at the moment and I'll get on it as soon as I can.

danmarshall commented 7 months ago

@milonic its absolutely ok to move at a glacial pace! Hope you can thaw out a bit.

milonic commented 6 months ago

Hi Dan,

Finally got to this. I have removed all code for modifying the arc and now it's just a plain old line to create the wedge.

I kept the old code for future use as I have some other ideas I'd like to incorporate into another primitive for potential inclusion into MakerJS.

Let me know what you think.

Andy

danmarshall commented 6 months ago

Nice work, it's looking good. Now maybe we should reconcile the need for the selfIntersect param. With lines as caps, there's only one scenario that would self intersect: when the endAngle is >= startAngle + 360. In that case we shouldn't add the caps.

milonic commented 6 months ago

Nearly got this but I have a question about MakerJs.paths.Arc that have the same start angle and end angle.

When I try to create an arc with the same Start and End angles I get no result back from MakerJS. If that's how it should be then that's fine. But is there a way of getting an Arc to become a Circle in this instance? That's kinda what I was hoping for.

OvalArc also returns a soft error at https://maker.js.org/playground/?script=OvalArc if I try to declare Start and End angles that match, same issue. Is that how it should be?

danmarshall commented 6 months ago

sigh yes this is a flaw in the way that angles are normalized. In the next version of Maker.js I'd like to not use endAngle since it can be ambiguous. For this PR, perhaps you can remove selfIntersect altogether.

milonic commented 6 months ago

OK, so I removed selfIntersect and cheated by adding a circle if the angles match. Hope it's not too hacky but I think it will be ok for ArcWedge.