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

Sync multiconstraint with current state #1314

Merged
merged 1 commit into from Dec 31, 2022

Conversation

77maxikov
Copy link
Contributor

Didn't test it thoroughly. Also there is a question about undo - should multiple constraints be trated as individual items for undo?

@phkahler
Copy link
Member

phkahler commented Dec 30, 2022

@77maxikov No. Undo should remove all the constraints at once. Or look at it as undoing the users last keypress.

c.type = Type::VERTICAL;
}
newcons.push_back(c);
}
Entity *e = SK.GetEntity(c.entityA);
Copy link
Member

@phkahler phkahler Dec 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do these 3 lines do now? Or what did they do before? the 3 starting at 619.

@phkahler
Copy link
Member

@77maxikov I think there is a problem in the Horizonal and Vertical constraints. It seems ha and hb are the end points of a line but go unused. It looks like the constraints apply to two points, and the new version is not adding the points to the constraint c, does that sound right?

Otherwise I think it just needs some testing. I'm curious about the automatic creating of REF dimensions instead of regular constraints. I want to make sure that works OK in a few tests.

@77maxikov
Copy link
Contributor Author

Ageee with unused ha and hb. They came from very old revision a8001ad
New rev accounts for constrained obj here c.entityA = enti; as I understood.

@phkahler
Copy link
Member

I think the dialogs will need updated from "two XXX" to "two or more XXX". Looks like this applies to:

EQUAL: lines, circles or arcs
H/V: lines but we should also add doing this for multiple points
PARALLEL: faces, line segments, normals

If you want to make another PR that's fine, or I'll get the dialogs after adding more (I wanted to add pt-face for up to 3 faces).

@ruevs
Copy link
Member

ruevs commented Jan 6, 2023

@phkahler @77maxikov
This is actually pretty buggy and at the very least (playing with it very superficially) breaks two existing Q constraint possibilities:

                       * four line segments or normals
                            (equal angle between A,B and C,D)
                       * three line segments or normals "
                            (equal angle between A,B and B,C)

" * four line segments or normals "

So this type of constraint is impossible to create now!
image

I suspect there will be more problems if I dig more.

@phkahler
Copy link
Member

phkahler commented Jan 7, 2023

@ruevs Ouch. The problem with 2 equal angles A/B and C/D is that we now treat that as 4 lines that need to have equal length. This should be easy to fix by moving the case for: (gs.vectors == 4 && gs.n == 4) above the other one for arbitrary number of lines, but that's kind of arbitrary. So the question is how should we handle that ambiguity? This is problem regardless of the implementation.

Same looks to be true of the 3 line segments A,B,C where the old intent it to set equal angle between A-B and B-C.

This is a problem for variadic constraints in general. We can fix them, but the question is how?

My first thought is to add some code to determine (in the 3 lines case) if they all share a point do the equal angle constraint. Also in the 4 lines case if A & B share a point and C & D share a point do equal angles. But of course that may still break existing sketches. The only option seems to be to allow multiple line segments Q to mean equal length for 2, or 5 or more lines.

What should we do here?

@phkahler
Copy link
Member

phkahler commented Jan 7, 2023

@ruevs another bizarre option might be to have N lines and M points indicate N equal length lines. I was thinking about the case where we use box selection to pick entities, and in the case of lines we inadvertently select the end points as well and it would make sense to interpret lines and points equal as just line lengths equal. I don't like this because it's really struggling to infer user intent, but it would allow multiple lines to be equal length and also equal angles in the cases that already do that.

@77maxikov 77maxikov deleted the multico_upd branch January 7, 2023 10:16
@77maxikov 77maxikov restored the multico_upd branch January 7, 2023 10:17
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.

"Variadic" constrain equal Allow constraints to apply to multiple objects
3 participants