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

Adding ArcWedge #585

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Adding ArcWedge #585

wants to merge 5 commits into from

Conversation

milonic
Copy link

@milonic milonic commented Feb 17, 2024

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

@milonic
Copy link
Author

milonic commented Feb 20, 2024

@microsoft-github-policy-service agree

@danmarshall
Copy link
Contributor

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

@danmarshall
Copy link
Contributor

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

@milonic
Copy link
Author

milonic commented Feb 20, 2024

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
Copy link
Contributor

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
Copy link
Author

milonic commented Mar 4, 2024

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
Copy link
Contributor

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

Changing ArcWedge tojust add a line at end of arcs. Works in same way as OvalArc but without the radius at each end.
@milonic
Copy link
Author

milonic commented Mar 18, 2024

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
Copy link
Contributor

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
Copy link
Author

milonic commented Mar 18, 2024

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
Copy link
Contributor

danmarshall commented Mar 18, 2024

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.

Removed selfIntersect and added new paths.Circle if start and end angled match
@milonic
Copy link
Author

milonic commented Mar 18, 2024

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.

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