mikedh / trimesh

Python library for loading and using triangular meshes.
https://trimesh.org
MIT License
3.02k stars 583 forks source link

Trimesh.union() can not handle multiple meshes ( interface change between v3.12 and v4.4.9 ) and docstring suggests otherwise #2291

Closed manuel-koch closed 1 month ago

manuel-koch commented 2 months ago

Upgraded trimesh from v3.12 to v4.4.9 in my setup.

Definition of trimesh.base.Trimesh.union() is using this arguments, where only a single "other" Trimesh can be combined ( this seems to be an interface change compare to v3.12 where multiple "other" instances were supported ):

def union(
        self,
        other: "Trimesh",
        engine: Optional[str] = None,
        check_volume: bool = True,
        **kwargs,
    ) -> "Trimesh":
    ....

But docstring states that one or more Trimesh instances can be passed to it.

other : Trimesh or (n, ) Trimesh
          Other meshes to union

When I try to pass multiple "other" Trimesh instances to this Trimesh.union() ( like it was working with v3.12 ), I now get an error like the following in the downstream call to trimesh.boolean.union(), where the meshes argument is now a list within a list, e.g.

meshes = [<trimesh.primitives.Cylinder>, [<trimesh.primitives.Cylinder>, <trimesh.Trimesh(vertices.shape=(34, 3), faces.shape=(64, 3))>, <trimesh.Trimesh(vertices.shape=(34, 3), faces.shape=(64, 3))>]]

which triggers an error in the list comprehension inside trimesh.boolean.union()

>   if check_volume and not all(m.is_volume for m in meshes):
E   AttributeError: 'list' object has no attribute 'is_volume'

My question is: Where is the error ?

In the implementation of trimesh.base.Trimesh.union() ( it should support multiple instances ) ?

Or in the docstring ( it falsely states that multiple instances are supported, despite the single-instance-type-hint ) ? But downstream method trimesh.boolean.union() does support multiple instances.

manuel-koch commented 1 month ago

@mikedh Not sure if changes related to "difference" are ok, I stumbled over this too in my migration changes for v3.12 to v4.4.9. Here the underlying boolean.difference() will actually complain that only two instances can be "diff'ed", using more than two instances will raise an exception.

But maybe this is only a problem for the used manifold engine. Though Trimesh.difference() still contains the type hint

other : trimesh.Trimesh, or list of trimesh.Trimesh objects
           Meshes to difference

while in fact just a single "other" will actually work.

See https://github.com/mikedh/trimesh/commit/5fa41458db9ca972e2bd9e33806dcc91184cb721#diff-ef3af4108d42422d355d528db2aca49e913c71710a7c7f0954d24c2c8aeefc4c

mikedh commented 1 month ago

Good catch, I added a fix and test for multiple differences in https://github.com/mikedh/trimesh/commit/2d7185ef2ad7e138a671a8e2dd275b8676de6886