compas-dev / compas

Core packages of the COMPAS framework.
https://compas.dev/compas/
MIT License
309 stars 104 forks source link

Problem with Mesh.face_circle() #1389

Closed robin-oval closed 1 week ago

robin-oval commented 1 week ago

The Frame in Circle in Mesh.face_circle() is wrongly initiated as Circle((point, normal), radius), instead of Circle(radius, (point_0, point_1, point_2)).

robin-oval commented 1 week ago

I saw them, but it seems that Mesh.face_circle() uses the base constructor, not from_plane_and_radius.

On Tue, 10 Sept 2024 at 15:16, Petras Vestartas @.***> wrote:

@robin-oval https://github.com/robin-oval

Did you check the constructor overloads too?

from_plane_and_radiusfrom_point_and_radiusfrom_three_pointsfrom_points

— Reply to this email directly, view it on GitHub https://github.com/compas-dev/compas/issues/1389#issuecomment-2340742841, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIFZVBE7ZNFT7X5KEWPUIVLZV3WJDAVCNFSM6AAAAABN6XHS4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBQG42DEOBUGE . You are receiving this because you were mentioned.Message ID: @.***>

petrasvestartas commented 1 week ago

I also get the same error:


from compas.geometry import Box
from compas_viewer import Viewer
from compas.datastructures import Mesh

# Create a box.
box = Box(1, 1, 1)
mesh = Mesh.from_shape(box)
faces = list(mesh.faces())

# Vizualize geometry.
viewer = Viewer()
viewer.scene.add(mesh)
viewer.scene.add(mesh.face_circle(faces[1]))
viewer.show()

The circle constructor needs to be changed to this:

    def face_circle(self, face):
        """The circle of a face.

        Parameters
        ----------
        face : int
            The face identifier. 

        Returns
        -------
        :class:`compas.geometry.Circle`
            The circle of the face.

        """
        from compas.geometry import bestfit_circle_numpy
        point, normal, radius = bestfit_circle_numpy(self.face_coordinates(face))
        return Circle.from_plane_and_radius(Plane(point, normal), radius)

Instead of using the default constructor

Circle((point, normal), radius)

image

@robin-oval can you make a pull-request?

And it would be great to make unit-tests for such methods in compas. I am wondering if it makes sense to make more unit-tests for other methods in compas ?

robin-oval commented 1 week ago

Solved with PR