@iscgar I'm still testing things and thinking about this. The first problem is a real problem - pruning requests should actually prune requests, not entities. The entities need to go too, but I'm assuming that was working prior to the bug. The second piece of code that tags and removes entities in a workplane that has been tagged for deletion should stay - I think. If a workplane is going to be deleted, any requests living in that workplane will need to be deleted as well.

Most of the time when we sketch in a workplane it is by doing NewGroup->Sketch in New Workplane. Which means deleting the workplane will delete the group, and hence all entities in the group/workplane. However, you can do Sketch in 3D, then create a workplane (just Sketch->Workplane), select that workplane and then Sketch->In Workplane and start drawing. Nothing in this scenario creates a new group, and you get a bunch of entities in a workplane that is just another part of the current group. If that plane is deleted we need to delete all the requests and entities belonging to it. Are we sure this is handled somewhere else?

The first problem is a real problem - pruning requests should actually prune requests, not entities. The entities need to go too, but I'm assuming that was working prior to the bug. The second piece of code that tags and removes entities in a workplane that has been tagged for deletion should stay - I think. If a workplane is going to be deleted, any requests living in that workplane will need to be deleted as well.

[snip]

If that plane is deleted we need to delete all the requests and entities belonging to it. Are we sure this is handled somewhere else?

Both pieces of code are doing the exact same thing after my changes.

In general, all entities are created by requests. Group entities (remap entities) are simply copies of other entities, that may themselves be copies of other entities (through other remaps), but the chain of copies always ends in an entity that was created by a request.

During regeneration all of the entities are thrown away and regenerated from the existing requets and groups. So when a request is removed, all of its entities go away automatically and there's no need to prune individual entities.

Requests generate all of their entitites on their own, and don't generally share entities, so when a request is removed, in theory we don't need to worry about other requests at all. However, the only exception to that rule is a workplane entity, which is shared by the request that created it and all of the entitities that are sketched in that workplane, and that's exactly why PruneRequests() exists.

This PR restores the logic of PruneRequests() to be this: iterate over all of the entities, and if we encounter an entity that was generated by a request, and its workplane is from a request that doesn't exist, the request from which the current entity was generated is also removed (this doesn't need to be done recursively because only workplace entities are shared between requests, and workplane requests themselves don't depend on other requests, since they have their workplane set to FREE_IN_3D).

The other piece is now redundant, since it implements the same logic, and was only needed because the changes in 86f20cc broke that logic by pruning entities instead of requests, so that's why I reverted it.

In general, all entities are created by requests. Group entities (remap entities) are simply copies of other entities, that may themselves be copies of other entities (through other remaps), but the chain of copies always ends in an entity that was created by a request.

For extrusions, remap create entities from prior entities. The new ones are not AFICT associated with any request. Also when linking files remap is used to assign new handles to entities in the other file, and these are considered to have the group as their parent. Similarly, when linking STL or IDF files, entities are created using remap from nothing they are not even descended from a request.

The remap table allows re-created entities to have the same handle so that subsequent constraints can be preserved. BTW In the future I'd prefer if remapped entities are not deleted and recreated if possible so that we may assign other attribute to them that get preserved ( I'm thinking fillet or chamfer, but right now we can't even assign a different style).

I guess I'm arguing to preserve the code that whitequark added. OTOH if you're sure its removing exactly the same entities then the code can be removed. But one function is removing group-specific entities and the other is removing anything that's been tagged from the global sketch. Or is the one being called after tagging by the previous one?

For extrusions, remap create entities from prior entities. The new ones are not AFICT associated with any request. Also when linking files remap is used to assign new handles to entities in the other file, and these are considered to have the group as their parent. Similarly, when linking STL or IDF files, entities are created using remap from nothing they are not even descended from a request.

Right, sorry. What I meant to highlight is that the entities themselves are thrown away and regenerated during GenerateAll(), and that's true for linked groups as well (except instead of being regenerated from request entities, the impEntities is used as a source). For extrusions, the new entities are copied from existing request entities though, and while they have the group set as their parent instead of a request, the extrude group is associated with a workplane, and the code in the PruneGroups() function is responsible for finding and pruning groups whose workplane or origin are gone.

In any case, there's code to handle pruning of both group-generated entities and request-generated entities, and pruning the entities themselves is useless because they are always regenerated.

The remap table allows re-created entities to have the same handle so that subsequent constraints can be preserved. BTW In the future I'd prefer if remapped entities are not deleted and recreated if possible so that we may assign other attribute to them that get preserved ( I'm thinking fillet or chamfer, but right now we can't even assign a different style).

The best way to do that would be to extend the remap table to contain a mini version of request that stores metadata as well. That way, during regeneration the metadata could be attached to the regenerated entities too.

I guess I'm arguing to preserve the code that whitequark added. OTOH if you're sure its removing exactly the same entities then the code can be removed. But one function is removing group-specific entities and the other is removing anything that's been tagged from the global sketch. Or is the one being called after tagging by the previous one?

It's indeed removing exactly the same entities. I'm not sure I follow your question though. Neither piece of code is removing group-specific entities. In fact, there's no code to do that, only a workplace can be depended upon by other entities, so when a workplane request is removed, we only need to remove specific requests that depend on it, or an entire group if it depends on it for its workplane (group pruning is done recursively using PruneGroups(), because groups can depend on previous groups that get pruned). If you're referring to pruning of remap entities, then those are never pruned directly (they can only be pruned as a result of their entire group being pruned).

@phkahler I am with @iscgar on this one. 86f20cc did introduce the bug (#628) by removing only the first entity in the group without a workplane. The old code found all such entities and removed their requests. 586b047 was an ugly hack to fix it.

And while on the topic. I think the change in SolveSpaceUI::PruneConstraints is also not functionally equivalent - std::find_if exists as soon as it finds the first matching item - so at most one item will be removed. This is not the same as the for loop it replaced.

And the two std::find_if in SolveSpaceUI::PruneOrphans are "wrong" for the same reason.

@rpavlik are we missing something or did we miss it at the time?

not functionally equivalent - std::find_if exists as soon as it finds the first matching item

Oh that's bad.

Could this be the cause for #1239?

And while on the topic. I think the change in SolveSpaceUI::PruneConstraints is also not functionally equivalent - std::find_if exists as soon as it finds the first matching item - so at most one item will be removed. This is not the same as the for loop it replaced.

The original for loop also returned after removing a single request, so the conversion to std::find_if() by @rpavlik is correct in that case. However, I intentionally changed it to tag all bad requests and remove them at once,, because otherwise we recurse into GenerateAll() for each request that is removed, which is wasteful.

And the two std::find_if in SolveSpaceUI::PruneOrphans are "wrong" for the same reason.

@rpavlik are we missing something or did we miss it at the time?

No, the original code behaved that way for PruneOrphans() and PruneConstraints() as well. I have a branch where I changed them too to remove all of the requests and constraints that match the criteria instead of just the first one, and I planned to submit it as a separate PR later. I can submit those changes as part of this PR if you prefer that, or restore the code of PruneRequests() to prune a single request each time and submit the bulk removal change as a separate PR.

Could this be the cause for #1239?

Unlikely, since that message is shown only in the code path that doesn't prune any objects (i.e. done regenerating), while the combination of 86f20cc and 586b047 doesn't change that. It just caused PruneRequests() to never actually prunes anything (because all of the relevant requests are already pruned by the code introduced in 586b047), allowing the code to continue to try pruning constraints and groups without recursing back into GnenerateAll() through the pruned path.

And while on the topic. I think the change in SolveSpaceUI::PruneConstraints is also not functionally equivalent - std::find_if exists as soon as it finds the first matching item - so at most one item will be removed. This is not the same as the for loop it replaced.

The original for loop also returned after removing a single request, so the conversion to std::find_if() by @rpavlik is correct in that case. However, I intentionally changed it to tag all bad requests and remove them at once,, because otherwise we recurse into GenerateAll() for each request that is removed, which is wasteful.

And the two std::find_if in SolveSpaceUI::PruneOrphans are "wrong" for the same reason.
@rpavlik are we missing something or did we miss it at the time?

No, the original code behaved that way for PruneOrphans() and PruneConstraints() as well. I have a branch where I changed them too to remove all of the requests and constraints that match the criteria instead of just the first one, and I planned to submit it as a separate PR later. I can submit those changes as part of this PR if you prefer that, or restore the code of PruneRequests() to prune a single request each time and submit the bulk removal change as a separate PR.

OK I read more carefully (not from the phone) this time and you are right about everything.

1. Leave PruneOrphans alone. This:
   

   solvespace/src/generate.cpp

   Line 206 in 2c5bb5d

   while(PruneOrphans())

   
   may be a bit stupid, but it is harmless :-)

2. If we are going to change PruneRequests then we should change PruneConstraints to prune everything at once as well. After all both decide
   

   solvespace/src/generate.cpp

   Line 245 in 2c5bb5d

   if(PruneRequests(hg) || PruneConstraints(hg))

   
   whether we will goto pruned; and call GenerateAll again.

3. This will be a very nice optimization and a significant speed up in some cases.

4. My only lingering doubt about doing it is whether pruning the requests and constraints one by one and running GenerateAll each time - an thus solving "a little bit at a time" (everything already pruned) - somehow helps with the model not rearranging itself drastically when something is deleted... This may be stupid. This is the first time I am looking at this code.

@ruevs So do you want to push the green button? ;-) I'm satisfied.

2. If we are going to change PruneRequests then we should change PruneConstraints to prune everything at once as well. After all both decide
   

   solvespace/src/generate.cpp

   Line 245 in 2c5bb5d

   if(PruneRequests(hg) || PruneConstraints(hg))

whether we will goto pruned; and call GenerateAll again.

As I said, I already have that ready in a private branch. I can just add it to this PR if you prefer to tackle them both here.

4. My only lingering doubt about doing it is whether pruning the requests and constraints one by one and running GenerateAll each time - an thus solving "a little bit at a time" (everything already pruned) - somehow helps with the model not rearranging itself drastically when something is deleted... This may be stupid. This is the first time I am looking at this code.

As long as things get pruned, we simply recurse into GenerateAll() again, and we don't try to solve the current group. So it doesn't matter if we do it one by one or all at once, since the solver will only be called to rearrange things after we've pruned everything we could in the current group.

As I said, I already have that ready in a private branch. I can just add it to this PR if you prefer to tackle them both here.

@iscgar ooops I committed a change to PruneOrphans and PruneConstraints, to prune all items, before reading your comment - and I forgot you already mentioned it earlier. Sorry. Feel free to revert it and take your versions.

But yes - let us do it all at once in this PR.

As long as things get pruned, we simply recurse into GenerateAll() again, and we don't try to solve the current group. So it doesn't matter if we do it one by one or all at once, since the solver will only be called to rearrange things after we've pruned everything we could in the current group.

Reading even more carefully :-) you are right again - all it does is Generate stuff before the pruning. So there is no problem, but the optimization will not be as large as I initially (incorrectly) thought.

@iscgar ooops I committed a change to PruneOrphans and PruneConstraints, to prune all items, before reading your comment - and I forgot you already mentioned it earlier. Sorry. Feel free to revert it and take your versions.

But yes - let us do it all at once in this PR.

No worries, your code is almost exactly what I had in my branch. I added another minor optimisation from my branch, but feel free to drop it if you think that it doesn't belong in this PR.

Thank you @iscgar .