I made a simple shape with some construction and non-construction lines, did a rotate and helix. It works fine until I increase the number of copies in the rotate group. Just 30 little octagons with a couple construction lines to the middle works, 36 fails. Either one can be extruded straight, only the helix operation fails.
helix_test.zip

Edit: My attached file also fails with Lathe unless the number of copies is reduced. I'm not sure the calls to CopyEntity are done correctly in Group::Generate for these types of groups. Still trying to understand Remap() is that just making a hash table? Home grown?

I'm starting to think this is an issue in Remap itself. As I understand thing:

EntityMap is used to track which entities are copied from which others. Example would be extruding a sketch where 2 copies of the entities are created with a translation from the originals. Remap is used to get handle for a new entity given the handle of the original and a "copy number". For extrusions, the copy numbers are REMAP_TOP and REMAP_BOTTOM. For a Lathe they are LATHE_START and LATHE_END. But wait, Lathe actually creates 3 copies and the first one actually uses a running counter for the copy number rather than a fixed value. Why I don't know.

If a copy is requested but already exists, no new entity is created and remap returns the handle to the existing entity. I believe this is so when an underlying sketch is modified, all remaining original entities and their copies are preserved when the modified extrusion is created. That's important because other groups may have dependencies (constraints) built on those entities so they should not be destroyed and recreated.

This a fairly simple setup, but it's using tempates and parts of the standard library that I'm not familiar with. There is an EntityKeyHash function that I believe XORs the handle and the copy number, but that may produce collisions and I'm not sure at all how that is handled. There is also and EntityKeyEqual function that seem to compare both the handle and the copy number (as opposed to comparing hashes of them) so ordering and equality are not based on the same thing?

@rpavlik and @whitequark am I understanding this correctly?

The fixed constants for copy number like REMAP_TOP have values just over 1000, so they will collide with the running entity counter on certain operations with too many entites, but even that doesn't seem to be what's happening here. If I replace the running counter with a constant for the copy number it's more likely to crash.

I think @jwesthues would be most qualified to answer here, I understand remapping about as well as you do.

@phkahler Your basic understanding is correct--the purpose of the remap mechanism is to maintain the association between the original entity and entities derived from it (by extrusion, step and repeat translating/rotating, etc.), even as entities are added to or deleted from the original sketch.

I didn't write the current implementation of Remap(), but looks to me like it's just an STL hash table. The hash function may generate collisions; but that should affect only efficiency (in the usual way for hash tables), not correctness. I also didn't write the lathe entities generation, and I'm not sure I understand it either. The arcs that get generated from points require four output entities per input entity (the arc itself plus the three points defining it); but for some reason the center point for the arc is defined as Remap(predef.origin, ai) where ai is that running count, instead of something like Remap(ep->h, REMAP_LATHE_ARC_CENTER).

REMAP_TOP and the other constants were intended to collide only with the copy count, not the entity count. So I expected e.g. step and repeat translating 2000 times to break, but not translating 100 entities 100 times. I'm thinking that use of ai breaks that, though. I'm not sure I understand why your octagon test breaks at 36, since 36 * 8 * 3 (assuming two endpoints plus the line itself) = 864 < 1000? But it should break somewhere around there.

You could fix that by redefining the remap as above, though that would break associations in existing files. I don't see another great fix that doesn't break existing associations. You could keep the existing remap for ai < 1000 and then switch over, but that's ugly. I think the ai mechanism also breaks associativity when points are deleted?

I also didn't write the lathe entities generation, and I'm not sure I understand it either. The arcs that get generated from points require four output entities per input entity (the arc itself plus the three points defining it); but for some reason the center point for the arc is defined as Remap(predef.origin, ai) where ai is that running count, instead of something like Remap(ep->h, REMAP_LATHE_ARC_CENTER).

Ah, that's really unfortunate--the lathe entities generation was something I asked @Evil-Spirit to add a very long time ago, and it looks like both of us missed this. I'm not sure if we can fix this and upgrade old files, like we did in similar cases before, but if we can't, I lean towards breaking old files, unfortunate as that is.

If desired, I do think you could write a special case upon import to fix up old files to the new convention. Probably not a big deal either way, since the only thing that would break is constraints against (or other references to) the centers of generated arcs, probably a small minority even of files using the lathe operation.

If desired, I do think you could write a special case upon import to fix up old files to the new convention.

It's not clear to me if the old files here can be detected, but I might be missing something.

Probably not a big deal either way, since the only thing that would break is constraints against (or other references to) the centers of generated arcs, probably a small minority even of files using the lathe operation.

I agree; let's just fix this.

I think you could detect old files by the presence of a remap entry in a lathe group with an index that's not REMAP_LATHE_xxx. I probably wouldn't bother, though.

OK, so to fix this particular crash in Helix, it seems to be fine by just eliminating all the copies of the origin. That was copied from the Revolve code, which was copied from the Lathe code and seem to be completely unneeded in both cases. I had it in Revolve to allow creating of arc entities, but those are commented out because they only worked in one direction. Things seem to be working without the extra copies in Revolve and Helix but I'll do some more testing before offering a fix.

Lathe can probably be left alone for now to maintain compatibility. If we Remap the points instead of the origin, the math will have to be changed to project that point onto the axis instead of moving the origin up the axis. But that will prevent excessive copies of any one entity. It's also not clear to me that LATHE_START and LATHE_END are both needed, as they are identical. Fewer copies = smaller files ;-)

Lathe can probably be left alone for now to maintain compatibility.

Can you please do the change for Lathe too, as a separate commit? I think it's important to fix such mistakes going forward, especially if the expected amount of breakage is very low (I think even if you clicked on the remapped point, you might get the original one because of the way selection works). If it turns out we misjudged it, it can be reverted.

@whitequark I agree. I'll try to fix lathe groups after this. It's all good for my understanding since I've got a bunch more features in mind.

Thank you!

This issue is fixed in PR #581.
A helical gear can be created from the original attached sketch, as seen in the picture a few comments up. Some previous files with Revolve or Helix groups might be broken depending what was constrained to duplicate entities that are no longer created.