@ruevs and @jwesthues Looking for feedback on this so far. It's got one and two sided, but one-sided make the original go away which doesn't seem right. I think the original solid should remain, but any idea why the solid created in a previous group goes away when its mirrored?

Also wondering about where in the code to add optional constraint when the group is created. For example, any points selected should be constrained coincident with the (invisible) mirror plane. Or a face selected should be used to fully constrain the mirror plane. It looks like those constraints should be created in Group::GenerateEquations() but the selection stuff is available in Group::MenuGroup() which creates the group. What to do here?

If you step and repeat group x, then the step and repeat starts with the same solid model as group x would have started with. This means that if the step and repeat doesn't include the original (e.g., because it starts with copy number one, or because it's two-sided with an even number of copies), then the original disappears. Your current behavior seems consistent with that. I think there'd also generally be no difference between your one-sided and two-sided options if the original were always present?

For the constraints, perhaps best to create them in the same way as user-created constraints using Constraint::AddConstraint(), from Group::MenuGroup()? Then the user can view and edit those constraints later. I actually wanted to do the Sketch in New Workplane group that way, using the constraint solver to define the workplane in terms of the selected geometry. There I decided against that, to avoid further stressing the solver in groups that were likely to contain large amounts of user-created geometry. That's unlikely to be a problem for you though, with just the constraints to define the mirroring plus maybe a bit of construction geometry if needed (and in any case, providing the constraint equations from Group::GenerateEquations() would present the same problem).

Perhaps I will have time to take a look over the weekend.

@jwesthues I don't understand how the previous groups shell is removed when I do one-sided mirror, or how it can happen for repeat groups. What I was hoping is that one-sided would leave the original and create a single mirrored copy, just like the entities. I thought two sided would be tricky because I'd want to remove the previous groups shell and include it in the 2-sided group shell, but that seems to be how it's working already. I just don't want the old shell to go away for one-sided.

Also, I'm not sure why mirroring a 2d sketch results in something that won't create a shell when extruded.

Here's the line that makes the shell/mesh work that way for existing step and repeat groups:

Without that it would be very hard e.g. to extrude a shape and then step and repeat the extrusion four times symmetrically about the original, since there'd be no good way to get rid of the original extrusion. (Of course the original entities still exist, since nothing in a subsequent group can affect them in any way; this is only about the solid model. So this is relevant only if e.g. we extrude and then step, and not if we step and then extrude. But sometimes it's necessary to extrude and then step, for example if the steps don't all lie in the original sketch plane. I believe a similar issue exists with mirror.)

Re the failure to extrude, are you sure that you're ending up with a closed planar section? Enabling the same Group::DrawPolyError() debugging as for sketch-in-workplane groups might give a clue.

@jwesthues OK, the current equivalent code for Mirror looks like this:

        if(!srcg->suppress) {
            if(!IsForcedToMesh()) {
                GenerateForMirror<SShell>(&(srcg->thisShell), &thisShell, srcg->meshCombine);
            } else {
                SMesh prevm = {};
                prevm.MakeFromCopyOf(&srcg->thisMesh);
                srcg->thisShell.TriangulateInto(&prevm);
                GenerateForMirror<SMesh>(&prevm, &thisMesh, srcg->meshCombine);
            }
        }

This makes the original disappear for a one-sided mirror group, which I don't want.

What I want is that when a solid (say g003) gets mirrored into g004 you'll always see 2 objects. For 2 sided a subsequent mirror group (g005) would have 4 objects. If g004 is one-sided it will still have two objects but a subsequent mirror group will only have 3 objects. The entites currently follow this logic.

So I think for 2-sided mirror groups we need to make sure the solid from the previous group (original) is not combined with the overlapping copy in the mirror group (suppress it?). But for 1-sided mirror groups, both the previous group and the mirror group should have a single copy. This should also apply to repeated mirror groups.

I'm still unclear on how these shells are combined and in what order, so the logic to do this is also unclear. I also searched for the suppress flag and only see it being set in the text window (checkbox), so it's not clear how it's affecting translate or rotate groups at all. I'm very confused.

If you make srcg this instead of opA, then I think it may behave as you intend? That's inconsistent with translate and rotate, though. It may also cause e.g. Boolean union failures with a two-sided mirror with rounded geometry, due to the coincidence between the previous group's shell's copy of the original mirrored feature and the mirror group's shell's copy. I guess you could take srcg as this for one-sided but opA for two-sided, though that's ugly.

Shells (and meshes) are combined with Booleans strictly in group order, in the same order in which they appear in the text window. Normally the Boolean (A union B, A minus B, etc.) takes the output of the previous group's Boolean as input A. Step translate and rotate are handled differently for the reasons noted in my earlier comment here. This happens when the different choice of srcg causes a different input A due to

Group::RunningMeshGroup() will probably also need a change to make transform groups (translate, rotate, mirror) with a mirror group as the input work properly.

suppress is currently used for the "suppress this group's solid model" checkbox and nothing else. It affects step translate and rotate groups only indirectly, since we're testing that flag for srcg (e.g., the stepped extrude group or whatever), not the step group itself. The step group effectively inherits that flag from the group that it steps, in the same way that it inherits the "combine as union / difference / etc." behavior. It seems inadvisable to me to repurpose that flag for anything else, better to keep the flag just for that checkbox and change any conditions if necessary.

Finally, note that the entity behavior and the solid model behavior for the existing step translate and rotate groups lack the consistency that you're trying for with mirror. If I step and repeat translating an extrusion four times symmetrically about the original, then the original solid model disappears (since it's an even number of copies), but the original entities don't. I don't love that; but I tolerated that because it's easy to hide the original group if you don't want those entities, but hard to remove the original solid model.