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

Addition of ArcLength Ratio and ArcLength Difference constraints to Constraints list #1062

Merged
merged 1 commit into from Jun 28, 2021

Conversation

xgenericx
Copy link
Contributor

Addition of ArcLength Ratio and ArcLength Difference constraints to Constraints list

With this change, the user will be able to leverage the RATIO and DIFFERENCE constraint between Line-Segments and Arcs.

These changes add:

Arc-Arc Ratios
Arc-Line Ratios
Arc-Arc Differences
Arc-Line Differences

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2021

CLA assistant check
All committers have signed the CLA.

@xgenericx
Copy link
Contributor Author

image

image

image

image

And similarly for Arc-Arc Ratios and Arc-Arc Differences

@phkahler
Copy link
Member

@xgenericx This is an impressive change for a first time contributiion to solvespace! I have some concerns about it. In particular there are some areas where you comment:

                // As the angle crosses pi, cos theta1 is not invertible;
                // so use the sine to stop blowing up

I haven't written any new constraints myself yet, so I'm not completely familiar with that area. However you are creating expressions conditionally here depending on the angle. My understanding is that these are algebraic expressions and this code will only be run at the time the constraints are created. The constraint solver itself does not AFAIK support changing constraint equations while the user is dragging things around.

We probably need a quick consult from either @jwesthues or @Evil-Spirit.

@jwesthues
Copy link
Member

That's the same logic as the existing arc-length-equals-line-length constraint, and a few other constraints (e.g., parallel in 3d) present a similar issue. The problem is indeed as you note, that we might choose option A based on our initial guess, but a few solver iterations later we might discover that we'd have preferred option B.

But the overlap area where both options work is pretty wide, so in practice it's usually fine (and the choice gets re-made across invocations of the solver, just not across Newton iterations). It would be nicer to avoid this problem (e.g., with a differentiable atan2()), though I suspect that in most cases where the initial conditions are far enough for that to be relevant they're also far enough for other stuff to break.

@phkahler
Copy link
Member

(and the choice gets re-made across invocations of the solver, just not across Newton iterations)

@jwesthues I wasn't aware of that. In that case I guess we'll merge it so it can get some user testing ;-)

@phkahler phkahler merged commit 37de364 into solvespace:master Jun 28, 2021
@xgenericx
Copy link
Contributor Author

Awesome, I'm hoping others find it as useful as I do.

For reference, this PR dates back to 2014, when I asked about getting this implemented. I got it implemented myself soon afterwards but finally decided 7 years later to contribute back to the solvespace project and spent a few days getting it into the v3.0 code.
https://solvespace.com/forum.pl?action=viewthread&parent=148&tt=1394931025

@phkahler
Copy link
Member

phkahler commented Jul 1, 2021

@Michael-F-Bryan thought you may want to see this, as the discussion above talks about an odd aspect of our geometric constraint solver.

@ghost
Copy link

ghost commented Jul 4, 2021

Great addition!

As I understand, for "Equal Length Arc-Arc" its possible to use "Arc-Arc Differences" with 0 value.

Should be there "Equal Length Arc-Arc" as separate?

@phkahler
Copy link
Member

phkahler commented Jul 6, 2021

Should be there "Equal Length Arc-Arc" as separate?

Good point. As is right now, if you select 2 arcs and apply the "q" or equal constraint it gives them equal radius/diameter. I don't think that should change because that's probably what people want most often. However there is no equal length for arcs AFAICT other than perhaps using length difference as you suggest, or length ratio equal to 1.0. I'd probably recommend using the ratio for now.

@ruevs
Copy link
Member

ruevs commented Aug 11, 2021

@xgenericx @phkahler @jwesthues
This is a nice addition but the commit is not complete. Please look here b23336b (where in 2015 I added the length-difference constraint) for a few more files that need to be updated.

In principle - does this not go against the philosophy that we are not going to add any more "custom" constraints like these, and instead wait for (one of us or someone to implement) parametric sketches? #77 #77 (comment)

@ruevs
Copy link
Member

ruevs commented Aug 11, 2021

Do not misunderstand me - I like this - it adds functionality without adding (much) more UI.

@jwesthues
Copy link
Member

This seems orthogonal to parametric sketches to me? Like you could already express this constraint using construction geometry, but it's valuable to do it in a single keystroke. Even if you could also express this constraint using equations, that convenience value would still be there.

@ruevs
Copy link
Member

ruevs commented Aug 11, 2021

You are right. And the shortcuts are the existing ones.

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.

None yet

5 participants