@Evil-Spirit do you know where/how selected faces get rendered vs not-selected?

@phkahler there you go ;-)

How would the "point on three faces" constraint be stored? A completely new type?

We should not merge this before it can do something useful - it would be rather confusing.

@phkahler there you go ;-)

How would the "point on three faces" constraint be stored? A completely new type?

No. We shall create 3 point-on-face constraints. I've been pondering how to do the variadic constraints request and looking at the neglected PR on the topic.

I'm not sure why whitequark didnt accept it. It isnt really conforming to the style, nor is the existing style really great. I think creating multiple constraints inline would be fine in most cases vs creating a list of them.

OK understood - select a point and one to three faces; hit O - get one to thee constraints automatically generated. Makes sense. Of course then there would be three constraints to delete but that could legitimately be called a feature.

In the meantime the face selection code was too ugly for me - I cleaned it up a bit.

...variadic constraints request and looking at the neglected PR on the topic.
I'm not sure why whitequark didnt accept it.

There are two PR-s on the topic #530 and #836 which one are you pondering? ;-)

I'd like to just merge it now.

There are two PR-s on the topic #530 and #836 which one are you pondering? ;-)

Well, #530 is a little odd in making a list and then creating them all at once. #836 was designed for a specific use-case and assumes pairs of things to constrain (not what we want) but I think the style of the code is more in line with how I'd like it. So I was planning to just start rewrite the code for each type of constraint over again. If anyone wants to give that a go, I'm happy to merge PRs for these.

I'd like to just merge it now.

Please don't.

If for no other reason, than for the awful "loop unrolling" in describesreen. And even if we were to merge it, we have to set MAX_SELECTABLE_FACES to 2 until selecting three does something (but then the commit messages make no sense and need to be changed).

In general a lot of code in constraint and describescreen fells like "copy-paste, but not quite" - it annoys me every time I look at it. I've dealt with this pattern in the past and have some ideas. But there is A LOT of code and logic, and thus huge opportunity for regressions in both files. And the test suite covers none of it :-( So it will be a lot of work to plan, refactor and test.

And in my opinion neither #530 nor #836 nor your planned "a point and one to three faces coincident" goes in before such a clean up.

In general a lot of code in constraint and describescreen fells like "copy-paste, but not quite" - it annoys me every time I look at it.

Agreed, but it is fairly readable. Many areas of solvespace look like this often because the failure to use polymorphism, but I can understand and sometimes appreciate the simplicity of it.

One thought I had on the constraints was to make a table indicating what constraint type could be applied to which combinations of entities, but I haven't really dug into that idea enough to like it or not. That might even simplify the code for dialogs that pop up if you misapply a constraint. The complexity will never go away, so it's a question of how to embody it in code so people can read and understand it.

Allowing multiple point-on-plane constraints at once should be simple by modifying this:

            } else if(gs.points == 1 && gs.workplanes == 1 && gs.n == 2) {
                c.type = Type::PT_IN_PLANE;
                c.ptA = gs.point[0];
                c.entityA = gs.entity[0];
            } else if(gs.points == 1 && gs.lineSegments == 1 && gs.n == 2) {

The condition changes to:

            } else if(gs.points == 1 && gs.workplanes > 0 && gs.n == (gs.workplanes+1) ) {

Then just do the body of the condition in a loop over all the workplanes including the "AddConstraint(&c);" line from further down.

Then we either have to add a "break" when done, or move the "AddConstraint(&c);" to all the cases within ON_ENTITY. Neither of those seems desirable, but the other option seems to be to make a list of constraints like PR #530 and loop over them where "AddConstraint()" is called now.

@ruevs I don't see a cleaner way to do this. Please fill me in on what you think it should look like. Maybe @rpavlik can recommend something too, he seems to appreciate clean C++ .

Edit: The code above is in constraint.cpp

Agreed, but it is fairly readable

It is indeed readable. But it is getting... long.

One thought I had on the constraints was to make a table indicating...

A "table" of sorts is what I imagine as well! The complexity will indeed not go away - it will just be encoded in a more... structured... way. If I ever get to it I'll share the results of course.

@ruevs I want to push this in. I made a 3.2 milestone and I intend to do variadic constrain equal length and diameter, as well as probably 1-3 point-on-face constraints. However, my local and personal repos on github are quite broken so I want to re-fork and I don't want to lose these changes since they work and are not that big.

@phkahler I rebased on master, fixed the copy-pasta in describescreen.cpp and also pushed the branch to my fork here: https://github.com/ruevs/solvespace/tree/wip_phkahler_three-faces
so it is safe.

If you want I can also push the branch to the main repository, but in my opinion it is unwise to merge it into master- after all one can select three faces but can not do anything with them for now.

P.S. I also pushed to your branch.

@ruevs I rebased this on master after merging variadic constraints. Then I added a commit here to allow constraining a point to multiple faces at once. This is very handy for getting a point on a vertex that is a result of boolean operations where no point gets created.

@ruevs I rebased this on master after merging variadic constraints. Then I added a commit here to allow constraining a point to multiple faces at once.

Happy New Year @phkahler. This looks reasonable, but I have not tried it. Tomorrow I'll try to take a look at (and test) this and all the "multi-constraint" changes from the last few days.

@ruevs I rebased this on master after merging variadic constraints. Then I added a commit here to allow constraining a point to multiple faces at once.

Happy New Year @phkahler. This looks reasonable, but I have not tried it. Tomorrow I'll try to take a look at (and test) this and all the "multi-constraint" changes from the last few days.

In that case I'll just merge it. I tested it on 2 and 3 faces. Merging multi-constraints broke a bunch of stuff and needed fixing on master. It's slightly messy but I'd rather just do testing with master since it's still changing.