fix(modeling): applyTransform ceate new geometry

[hrgdavor]

The fix

• fix applyTransforms to produce new goemetry so it does not break other operations
• fix geenralize to use mat4.intentity transforms in output geom (because toPolygons was used and transform applied)
• this fix also depends on fix(modeling): undo-revert measureBoundingBox #973 to be fix for Missing breakouts in my model due to eliminated allocations of measureBoundingBox #966

old tests fixed

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Does your submission pass tests?

[z3dev]

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.

[hrgdavor]

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.

---

There are many cases where measurements are being called, followed by calling toPoints.

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 .
the fix also works for the 2 more complex examples from related github issue #966

[z3dev]

I think this should be split into two pieces. The fix for measureBoundingBoz should not rely upon other changes.

[hrgdavor]

ok, it is split now, but both are needed for the fix to be complete

[t1u1]

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.

[hrgdavor]

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

[z3dev]

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.

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.

[t1u1]

Thanks @hrgdavor @z3dev

After some more experiments I think the issue I am seeing is something else. Boolean operations involving geometries that touch each other cause weirdness. It was just getting triggered during rotation at certain angles.

Will file a separate issue for it.

[z3dev]

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

[hrgdavor]

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.

[hrgdavor]

I will revisit this at later time, adn create new issue if needed