Skip to content

Downcast the shape in Shape.__setstate__ - #2133

Closed
johnnoG wants to merge 1 commit into
CadQuery:masterfrom
johnnoG:fix-setstate-downcast
Closed

johnnoG wants to merge 1 commit into
CadQuery:masterfrom
johnnoG:fix-setstate-downcast

Conversation

@johnnoG

@johnnoG johnnoG commented Nov 8, 2026 •

Copy link
Copy Markdown

copy.copy, copy.deepcopy and pickle all rebuild a Shape through __setstate__ (added in #1762). It reads the BREP into a plain TopoDS_Shape() and assigns it without downcast, so a copied Face wraps a generic TopoDS_Shape instead of a TopoDS_Face. Shape.__init__ and importBin (through cast) both downcast, so this is the one construction path that does not.

The copy's own methods then fail (measured on 2.8.0 with OCP 7.9.3; the exception types may differ on master):

import copy
from cadquery.func import box

s = box(1, 1, 1)
copy.deepcopy(s.Faces()[0]).geomType()    # TypeError: __init__(): incompatible constructor arguments
copy.deepcopy(s.Edges()[0]).Length()      # TypeError: __init__(): incompatible constructor arguments
copy.deepcopy(s.Vertices()[0]).Center()   # TypeError: Pnt_s(): incompatible function arguments
copy.copy(s.Faces()[0]).normalAt()        # DispatchError

Code outside CadQuery that dispatches on the concrete OCP type fails too. For example, ocp_tessellate's to_ocpgroup([copy.deepcopy(cq.Workplane().box(1, 2, 3))]) raises ValueError: Unknown type: <class 'OCP.OCP.TopoDS.TopoDS_Shape'>.

This change calls downcast in __setstate__. A null shape (for example after shape.wrapped.Nullify()) is assigned as is, because downcast raises on a null shape and its round trip works today. The pickle format does not change, so old pickles still load.

Not changed here: __setstate__ still does not restore label or a vertex's X/Y/Z, before or after this change.

Tests in tests/test_pickle.py:

  • test_shape_type checks that copy, deepcopy and a pickle round trip keep the TopoDS type of a solid, a face, an edge and a vertex. It fails on master.
  • test_null_shape checks that a null shape survives a pickle round trip. It passes on master and fails if the IsNull guard is removed.

@johnnoG
johnnoG force-pushed the fix-setstate-downcast branch from 17103dc to 3679482 Compare November 8, 2026 10:16
@johnnoG

johnnoG commented Nov 8, 2026

Copy link
Copy Markdown
Author

The red AppVeyor jobs are 504s from GitHub while downloading the Miniforge installer, before any test runs. The full suite and black passed on Windows and macOS (and Azure is green), and Linux never got past the download. I can re-push once that settles, or feel free to re-run it from AppVeyor.

@adam-urbanczyk @jmwright could one of you review this when you have a moment? It's a one-line fix in Shape.__setstate__ plus tests.

@adam-urbanczyk

Copy link
Copy Markdown
Member

Please get rid of Claude-branded commits

copy.copy, copy.deepcopy and pickle rebuild a Shape through
__setstate__, which assigned a plain TopoDS_Shape. Every other
construction path downcasts (__init__, and importBin through cast), and
the copy's own methods fail on the generic type, for example
deepcopy(face).geomType() and deepcopy(edge).Length().
Code that dispatches on the concrete OCP type, such as ocp_tessellate,
rejects it too. A null shape is assigned as is, since downcast raises
on it.
@johnnoG
johnnoG force-pushed the fix-setstate-downcast branch from 3679482 to 26dd75a Compare November 8, 2026 10:35
@johnnoG

johnnoG commented Nov 8, 2026 •

Copy link
Copy Markdown
Author

@adam-urbanczyk Done, the commit and the description no longer mention Claude.

@johnnoG johnnoG closed this Nov 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants