-
Notifications
You must be signed in to change notification settings - Fork 511
Entity identifier remap collision causes crash when linking large assembly #688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Does that result in >64k entities? If so this issue might not be solvable with reasonable effort. |
Yes, if it's not something that can be supported without substantial effort maybe a warning dialog could be put up when such a model is saved and loaded? If the IdList scailing is improved, it becomes very easy to cross this threshold and having IdList collision detection throw an assert when assembling is a very rude way to tell the user "You've linked something with too many entities to link as a new group, despite being able to create it in the same tool without trouble." Obviously it'd be nice to just handle it, since with IdList indexing this seems to be an otherwise tenable situation. |
@whitequark Would it make sense not to load/remap entities that are "turned off" when importing a file for assembly? In that case, a user could make new group "sketch in 3d" and trace any entities or things they want to constrain with for the completed part, turn off all previous groups and save the file. Then in the assembly they'd only get that last set of entites instead of the whole construction history worth, which can't be manipulated anyway in an assembly. That might make this completely workable. This depends on the shell or mesh being saved so it doesn't need to be regenerated, is that stuff saved? |
Does that break savefiles? |
Maybe? I didn't mean to alter the individual part files, but it could cause problems with the assembly if someone changes which entities are "on" in a part and then re-opens the assembly. I have also seen comments that the triangle mesh is saved to the .slvs file, I'm wondering if the NURBS shell is too? That's important if we want to load the geometry without the entities needed to regenerate them. It was just a thought, but it very much depends what's in the save files. |
Well, I don't think that any changes that break savefiles are viable. |
Steps to reproduce:
Grab https://pengaru.com/~vc/tmp/solvespace-repro/*
In a new SolveSpace session, select New Group -> Link Assemble, select core-half.slvs from the above linked set of files.
I didn't actually reproduce this on plain master because it was taking too long and overheating my laptop before I grew impatient. But I did repro this on 9253e5f which manages to reach the collision point much faster leading me to file this as an upstream bug. I initially uncovered this on master with https://github.com/Evil-Spirit/solvespace-master/tree/id-list-indexing rebased on it which is also so fast it can fail quickly, but had assumed the changes caused the crash.
After spending many hours last night littering the code with printfs to figure out what was going wrong with the IdList changes it became apparent that https://github.com/solvespace/solvespace/blob/master/src/group.cpp#L1030 was generating an identifier that already existed during https://github.com/solvespace/solvespace/blob/master/src/group.cpp#L754, and it wasn't the IdList changes causing it at all.
The text was updated successfully, but these errors were encountered: