Skip to content
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

Entity identifier remap collision causes crash when linking large assembly #688

Open
vcaputo opened this issue Aug 28, 2020 · 6 comments
Open
Labels

Comments

@vcaputo
Copy link

vcaputo commented Aug 28, 2020

Steps to reproduce:

  1. Grab https://pengaru.com/~vc/tmp/solvespace-repro/*

  2. 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.

@whitequark whitequark added the bug label Aug 28, 2020
@whitequark
Copy link
Contributor

Does that result in >64k entities? If so this issue might not be solvable with reasonable effort.

@vcaputo
Copy link
Author

vcaputo commented Aug 31, 2020

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.

@phkahler
Copy link
Member

phkahler commented Sep 1, 2020

@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?

@whitequark
Copy link
Contributor

Would it make sense not to load/remap entities that are "turned off" when importing a file for assembly?

Does that break savefiles?

@phkahler
Copy link
Member

phkahler commented Sep 2, 2020

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.

@whitequark
Copy link
Contributor

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.

Well, I don't think that any changes that break savefiles are viable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants