-
Notifications
You must be signed in to change notification settings - Fork 515
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
fix(modeling): applyTransform ceate new geometry #970
Conversation
General comments. The behavior change to applyTransforms is going to affect performance. There are many cases where measurements are being called, followed by calling toPoints. Also, these changes don’t fix measureBoudingBox. At least, I can’t see the fix. So, I don’t think the test cases are robust. |
the fix is in two parts: First part is in the caching code // create a new boundingBox. Mutating the passed boundingBox would break cached calculations
boundingBox = [
vec3.transform(vec3.create(), boundingBox[0], transforms),
vec3.transform(vec3.create(), boundingBox[1], transforms)
] the second is in aplyTransform to not mutate/destroy the geometry.
That is a poor design choice, and such code should be optimized to perform better. I would like to see the examples where this applies. unit test there in the code confirms both when error happens (without the fix), and works after fix . |
I think this should be split into two pieces. The fix for measureBoundingBoz should not rely upon other changes. |
ok, it is split now, but both are needed for the fix to be complete |
Thanks for your work @hrgdavor This looks like an important fix. I have been building a complex model using jscad, and it has been going fine until today. When I added a transform.rotate inside the model, things started looking weird. It took me a long while to realize that it was the rotation that was causing the weird geometry. Perhaps it is related to this issue. I will try to reduce the model to a smaller test case, but in the meanwhile, if this PR looks good it will be great to have this merged. |
@t1u1 this PR is more pragmatic change than a bug-fix. But when you have a test example, we can take look what is causing you issues. |
See issue #985 about some viewer issues. You can check if it’s a viewer issue. Just call toPolygons() before returning the shape from main(). That will rest the transforms, and the viewer should display properly. |
@hrgdavor i think these changes are no longer important. There were changes to improve copy() functions as well as transform() functions. if you still think these are important then let's discuss further. |
I need to consider it a bit more, and see if it could cause issues. I vageuely rememeber that it could actually be ok, in the sense it would not break other instances. |
I will revisit this at later time, adn create new issue if needed |
The fix
old tests fixed