Automatically create body?

[ian.rees]

Where does the text in the current pop-up come from? I expected a changed message but I only see additions.

When you say current pop-up, do you mean the one in the screenshot in the OP? That's produced by PartDesignGui::getBody(), which I'm asking not to produce a message in this case. The message in the OP needs to stay around for other usages, but the new one needs a different message. -Ian-

[simonvanderveldt]

Where does the text in the current pop-up come from? I expected a changed message but I only see additions.

When you say current pop-up, do you mean the one in the screenshot in the OP? That's produced by PartDesignGui::getBody(), which I'm asking not to produce a message in this case. The message in the OP needs to stay around for other usages, but the new one needs a different message. -Ian-

Yeah, that's the one I meant. Thanks for the clarification, obviously makes sense

[ian.rees]

How does this look? https://github.com/ianrrees/FreeCAD_tin ... -auto-body -Ian-

Add it for primitives too, I think. Thus, it may be a good idea to wrap the code you added to new-sketch command into a standalone routine, like "autoNewBody()"...

Yep, I'll do that in a few hours. Thanks!

[wmayer]

The automatically created body should be undo/redo-able, too.

[ian.rees]

The automatically created body should be undo/redo-able, too.

Good catch, thanks! I'll try to get to this after work today. -Ian-

[ian.rees]

DeepSOIC - I'm having a little trouble understanding some of the logic in PartDesign/Gui/Command.cpp CmdPartDesignNewSketch::activated().

Basically, I would like to to delay the creation of the new Sketch as far as possible, ideally until near to the openCommand(), so we don't have to sprinkle abortCommand() calls everywhere that the method might return early. I think some of the major branches are impossible in the case of a newly-created Body. For example, I guess (FaceFilter.match() || PlaneFilter.match()) won't ever be true?

The current PR is at https://github.com/FreeCAD/FreeCAD/pull/667 - I added a couple TODOs around a note at the top of CmdPartDesignNewSketch::activated()

Any chance you have some time to take a look? Thanks! -Ian-

[DeepSOIC]

DeepSOIC - I'm having a little trouble understanding some of the logic in PartDesign/Gui/Command.cpp CmdPartDesignNewSketch::activated().

Basically, I would like to to delay the creation of the new Sketch as far as possible, ideally until near to the openCommand(), so we don't have to sprinkle abortCommand() calls everywhere that the method might return early. I think some of the major branches are impossible in the case of a newly-created Body. For example, I guess (FaceFilter.match() || PlaneFilter.match()) won't ever be true?

Unfortunately I don't quite understand, even after scrolling through the changes of your PR. So here's my attempt to answer, maybe way off from what you wanted to get

Often with complicated logic, I use exceptions to cancel the process, rather than returns. Then, an exception handler around the whole logic calls the abortCommand(), and maybe does other cleanup.

If you want to include body creation into "new sketch" transaction, I'd just recommend to create an independent transaction for body creation, and don't bother any further. Or is transaction management of sketch creation itself broken, and you are attempting to fix it?

[DeepSOIC]

I think some of the major branches are impossible in the case of a newly-created Body. For example, I guess (FaceFilter.match() || PlaneFilter.match()) won't ever be true?

Not sure. Creation of body will likely purge selection, so probably yes. But I imagine the situation:
1. Part Box
2. Select face of Box
2. PartDesign Sketch
In this case, a user generally expects a sketch created on a face, which can then be padded to enlarge the box, etc. Transplanting this to a new workflow, this should happen:
1. New body
2. Body.BaseFeature = Box
3. new sketch
4. Sketch.Support = Box.Face1
Done.

[ian.rees]

Thanks, I'll take another stab at it tonight hopefully. Most of that PR is just refactoring the commands for adding additive/subtractive primitives so the new logic isn't repeated in 7 nearly identical places. On that front - I realised that we shouldn't be making new Body objects for subtractive primitives, because that would imply that it's OK to have a shape which is is not in a Body, that the new primitive will be subtracted from. For additive primitives, I think it does make sense to make a new Body as discussed.

The part I'm not understanding is all in CmdPartDesignNewSketch::activated(). And, the issue is mostly that I haven't got a good understanding of all the sketch creation modes.

Often with complicated logic, I use exceptions to cancel the process, rather than returns. Then, an exception handler around the whole logic calls the abortCommand(), and maybe does other cleanup.

But, that's the textbook use of goto!

[DeepSOIC]

But, that's the textbook use of goto!

I don't mind using gotos as long as they make code more readable, and help avoid repetitions, of course. And I even did it in FC code once, and I think it was almost merged. But it caused interesting compilation problems on Linux (related to jumps over variable initializations). Then I switched to exception approach.

And using exceptions is more interesting than plain gotos, because I can raise an exception deeper in stack. And a side benefit of gracefully handling unexpected failures...

Anyway, it's up to you.

And, the issue is mostly that I haven't got a good understanding of all the sketch creation modes.

I'll try gather it up.