ladybug-tools / ladybug-geometry

🐞 📦 A library with geometry objects used throughout the Ladybug Tools core libraries
https://www.ladybug.tools/ladybug-geometry/docs/
GNU Affero General Public License v3.0
25 stars 23 forks source link

Laybug Face3D .area error after duplicate() > to_dict() > from_dict() #404

Open ed-p-may opened 5 months ago

ed-p-may commented 5 months ago

Hi - I'm reposing this issue from the ladybug-tools forum here as well for reference. I recently had to revisit this issue for another project, and I'm pretty sure this is what is causing the issue:

> Original Post

When Face3D.__copy__() is called, it passes self.vertices in as the boundary argument for the new Face3D.__init__()

def __copy__(self):
     _new_face = Face3D(**self.vertices**, self.plane) #<----- HERE
     self._transfer_properties(_new_face)
     _new_face._holes = self._holes
     _new_face._polygon2d = self._polygon2d
     _new_face._mesh2d = self._mesh2d
     _new_face._mesh3d = self._mesh3d
     return _new_face

I believe this is incorrect and source of the issue. As stated in the docstring for the vertices property:

@property
def vertices(self):
    """Tuple of all vertices in this face.
    Note that, in the case of a face with holes, some vertices will be repeated
    since this property effectively traces out a single boundary around the
    whole shape, winding inward to cut out the holes.
    """
    ...

whereas the boundary argument is actually supposed to be only the outer boundary (without holes), according to its docstring:

@property
def boundary(self):
    """Tuple of vertices on the boundary of this face.
    For most Face3D objects, this will be identical to the vertices property.
    However, when the Face3D has holes within it, this property stores
    The outer boundary of the shape.
    """
    ...

So it appears that when duplicating a Face3D with holes in it, the holes area incorrectly included as part of the boundary which is passed in. Then, when the holes are set explicitly by the line _new_face._holes = self._holes they end up being doubled up.


For my test case, I have found that by just adding the line _new_face._boundary = self._boundary to __copy__, the error outlined above in the .area property appears to go away and I get the result I was expecting. But I’m not sure if that is a good enough fix to resolve all the cases where this issue might occur? There are a lot of various polygon methods on Face3D that I have not looked at very closely.