copy.copy on a Shape is surprisingly slow

[jsmnbom]

Using copy.copy on a Shape to create a shallow copy of the shape is a lot slower than I would have imagined from the documentation at https://build123d.readthedocs.io/en/latest/assemblies.html which implies that it is very very fast.

Example:

$ uv run -m timeit -s "import build123d,copy;screw=build123d.import_step('M6-1x12-countersunk-screw.step')" "copy.deepcopy(screw)"
200 loops, best of 5: 1.61 msec per loop

$ uv run -m timeit -s "import build123d,copy;screw=build123d.import_step('M6-1x12-countersunk-screw.step')" "copy.copy(screw)"
200 loops, best of 5: 1.54 msec per loop

This seems to imply that is not actually faster to make a shallow copy rather than a deep one. When actually outputting to a .step as in the documentation example, it is still MUCH faster however, so that is true, but I still found this behavior to be weird.

Looking into the code, it seems that __copy__ actually calls deepcopy:

class Shape(NodeMixin, Generic[TOPODS]):
    ...
    def __copy__(self) -> Self:
        reference = copy.deepcopy(self)
        if self.wrapped is not None:
            assert (reference.wrapped is not None)  # Ensure mypy knows reference.wrapped is not None
            reference.wrapped.TShape(self.wrapped.TShape())
        return reference

Is there a good reason to be doing things this way? I tried replacing it with the following, and so far I'm not seeing any issues:

class Shape(NodeMixin, Generic[TOPODS]):
    ...
    def __copy__(self) -> Self:
        cls = self.__class__
        result = cls.__new__(cls)
        result.__dict__.update(self.__dict__)
        return result

Is this a bug for is there some purpose to the deepcopy that I have missed?

I found this behavior when trying to improve the export_step to more accurately apply labels to shapes (see issue [NOT SUBMITTED YET] and possibly fix #301 while I'm at it, as well as foundation for #296, where it is important that TShapes are preserved when possible, and the current copy code seems to somehow break that - although I'm unsure exactly why.

[jsmnbom]

Nevermind my "fix" definitely breaks other things. Back to the drawing board then :/

[jsmnbom]

Okay so it seems like the cadquery OCP does not expose the proper initializer for TopoDS_Shape to be able to copy it (https://github.com/Open-Cascade-SAS/OCCT/blob/876ccbe977a7a8d4910e68fd6b3536645818ed0e/src/ModelingData/TKBRep/TopoDS/TopoDS_Shape.hxx#L53). We can easily get around that by just using .Located() which calls it for us. So I think a proper copy function looks like this:

class Shape(NodeMixin, Generic[TOPODS]):
    ...
    def __copy__(self) -> Self:
        cls = self.__class__
        result = cls.__new__(cls)
        for key, value in self.__dict__.items():
            if key == "wrapped":
                result.wrapped = downcast(self.wrapped.Located(self.wrapped.Location()))
            else:
                setattr(result, key, copy.copy(value))
        return result

$ uv run -m timeit -s "import build123d,copy;screw=build123d.import_step('M6-1x12-countersunk-screw.step')" "copy.copy(screw)" 
20000 loops, best of 5: 17.6 usec per loop

[gumyr]

There was no attempt to make the __copy__ function faster than __deepcopy__ just to make the use of copied objects faster. This looks like a promising way to add some performance though. The parent and joint logic of deepcopy would need to be replicated here wouldn't they?

[jsmnbom]

Oh my mistake then, hope it is still a welcome improvement.
After testing it a lot in my own code I have yet to find any issues, apart from copied objects exporting a bit weirdly if they are the same shape but labelled differently. I believe I've mostly fixed this as far as possible. (.step for example doesn't seem to support having two Solids that aren't duplicated in the source, have two separate names - tho a parent wrapper assembly of course still can so it usually is not an issue)

I am unsure of how topo_parent is used, and I have yet to play around with build123d joints yet so don't actually know.

Would you be interested in a PR with this change assuming I write the proper tests necessary?