All committers have signed the CLA.

Ideally the permanent history of the project would not have any "fix" commits, because to debug problems, it is useful to use git bisect, and if each commit is self-contained, then one can bisect much easier. Do you think you could squash the commits to be more self-contained?

I suspect the conflict is caused by that fix I offered in a comment to another issue. I also fixed it in my branch, but you also fixed it in master. If you revert the change from April 22 this should be able to merge properly.

If you revert the change from April 22 this should be able to merge properly.

This really isn't the right way to solve this problem. Do you know how to use git rebase?

The problem (I think) is that I made that change in the same commit as some others, so my patch won't apply cleanly now. No, not familiar with rebase but I'll read about it.

Rebase allows you to apply your commits on top of some other point in the history, like current master. If you rebase your commits on top of master in this case, the changes that are already in master will be removed (if possible to do automatically) or you'll get a merge conflict that you can resolve to remove them.

Similarly, git's "interactive rebase" feature allows combining commits together, like I've asked you do in here. With some effort it lets you split commits as well, although that's a bit tricky.

@whitequark Please try this again soon. If there are any issues I don't want to leave thing in their present state for long. It is rebased to master.

@phkahler Thanks, I will take a look at the earliest opportunity.

@phkahler Looks great. I've tried to torture it a bit and it seems to hold up nicely, with only a few NURBS failures around the usual pain points.

Have you thought about adding an inequality constraint (see #81 for implementation) that limits the range of the movement for the revolution? It seems that going over 360° tends to really degrade performance, so I'm worried about the sketch contorting itself into a state where you can't untangle it back easily.

Is it hard to add the remaining derived entities, i.e. arcs and faces?

Have you thought about adding an inequality constraint

Once we add axial displacement for helical revolutions you may want to go way beyond 360 degrees. In its present form that just creates overlapping surfaces which are buggy and slow.

Is it hard to add the remaining derived entities, i.e. arcs and faces?

Arcs I don't know. The code for Circles is still in there but commented out so we can see where that kind of thing would go. There are no helical entities either.

Faces - I'm still trying to figure out why they are not selectable. When I first made a helix from a rectangle I could select the top and bottom faces even though they were not flat, so I took that out. I thought it was back in, but it's not and I'm not sure why.

I wanted to do more with this, but it may be a while.

Once we add axial displacement for helical revolutions you may want to go way beyond 360 degrees. In its present form that just creates overlapping surfaces which are buggy and slow.

I was thinking about having a separate Revolve and Helix commands as you've suggested before. Former would have an inequality constraint, latter would not.

I was thinking about having a separate Revolve and Helix commands as you've suggested before. Former would have an inequality constraint, latter would not.

These are details to be worked out. Maybe we can get some user feedback? Can you accept/close this pull request?

Can you accept/close this pull request?

I can merge it if you rebase it so that each commit is independent and self-contained. If you are not able to do that then I will have to do it myself and I don't know when I will have time for it.

I did rebase it. The series of commits shows the progressive implementation of the feature. Each step can be run. If you want to flatten it go ahead, but I prefer to be able to read the commit messages and have finer grained steps so it's easier to see what changes added to or fixed each thing.

Something is wrong. I did the rebase on another machine, so now I re-cloned my github back home and it won't build and neither will the master branch - I also pulled from solvespace master. I'm getting:

fatal error: gtkmm/filechoosernative.h: No such file or directory
include <gtkmm/filechoosernative.h>

I rebased on 5/11 but now I should locally be up to date while my github repo remains as it was after rebase. BTW I'm on Fedora if that makes any difference.

@whitequark
I just checked and both Master and Geometry seem to build properly if I comment out:
define HAVE_GTK_FILECHOOSERNATIVE
In file guigtk.cpp which which was recently changed (in 838126f). I can't tell what changed with the file chooser, I can still open and save when it builds. This breakage seems totally independent of this PR. Please let me know how I/you/we should handle this.

@phkahler No action is needed from you wrt file chooser in this pull request as it is completely independent. That said, I would be curious as to why it happens--have you tried removing the entire build directory and trying again? I did change the build system a bit in the meantime, so it might be that an old value is cached in config.h somehow. Also, what exactly is your GTK/gtkmm versions?

Regarding this PR I will be able to give it attention in a day or two.

@whitequark My installed and devel versions of GTK may have gotten out of sync (one was 22 and one 24?). I Upgraded the whole system to Fedora 30 and both branches (master and geometry) build fine now. I must have accepted some udpates around the time of the rebase. Sorry for the panic.

My installed and devel versions of GTK may have gotten out of sync (one was 22 and one 24?).

Yes, that would do it. CMake caches certain version information and it has no good way to be notified of updates, so I would expect this.

@phkahler Sorry, had not much time this week and I have some other obligations I need to do soon. Next week hopefully.

FYI I tried a change where going over 360 degrees would create the same shell as lathe. It prevents the self intersecting and performance issues, but does not keep you from winding up the ends. A ranged constraint seems like a better idea.

Thanks for the recent fixes! I have some time to address this now, so let me know when you're done and I'll merge it then.

I've disabled the arcs on Revolve. The fix is not going to be easy and may require creating a new entity type with a different definition - it's a chirality issue.

@phkahler Thank you for this wonderful contribution, and sorry again for the delay. I've merged your commits as 5df53fc, with one minor semantic change: I fixed the hotkey for Group > Revolve. I've also ran clang-format on the diff to match the project's existing code style.