Skip to content

RhinoBrep.from_curves#1452

Merged
chenkasirer merged 5 commits intocompas-dev:mainfrom
obucklin:clean_brep_from_cuves
Apr 9, 2025
Merged

RhinoBrep.from_curves#1452
chenkasirer merged 5 commits intocompas-dev:mainfrom
obucklin:clean_brep_from_cuves

Conversation

@obucklin
Copy link
Contributor

@obucklin obucklin commented Mar 25, 2025

solves #1451
This adds implementation for Brep.from_curves for rhino.

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@obucklin obucklin changed the title Clean_brep_from_cuves RhinoBrep.from_cuves Mar 25, 2025
@obucklin obucklin changed the title RhinoBrep.from_cuves RhinoBrep.from_curves Mar 25, 2025
@obucklin
Copy link
Contributor Author

obucklin commented Apr 1, 2025

@chenkasirer do I ping someone for a review here? should I just be patient?


Returns
-------
list of :class:`~compas_rhino.geometry.RhinoBrep`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that CreatePlanarBreps returns a list of Breps but I imagined this function creating a solid from the outline of multiple faces of the (eventually) same brep. I'm also wondering because of the original docstring specifies a single Brep as the return type. There's currently no OCC implementation so nothing to compare to, but I'm simply not sure what's the intention here.

Could you maybe provide a use-case for this method that illustrates what we should be able to make with it?

Copy link
Contributor Author

@obucklin obucklin Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that CreatePlanarBreps returns a list of Breps but I imagined this function creating a solid from the outline of multiple faces of the (eventually) same brep.

this is the general use case. however if not all of the faces are joinable, how do you decide which ones to get rid of?

I'm also wondering because of the original docstring specifies a single Brep as the return type. There's currently no OCC implementation so nothing to compare to, but I'm simply not sure what's the intention here.

I can either update the docstring to return a list of Breps, or I can return None if the faces don't join into one single Brep. I would typically go for the former and have the user decide if they only accept one single Brep or if they use multiple Breps.

Could you maybe provide a use-case for this method that illustrates what we should be able to make with it?

I was developing this to build a brep of a timber plate in the way that the compas_model.Plate._compute_geometry() does it, that is by building faces from polyline points. That type builds a mesh from vertices and I want to build a Brep, obviously. sample code looks like this:

        for i in range(len(self.outline_a)-1):
            a = self.outline_a.points[i]
            b = self.outline_a.points[i+1]
            c = self.outline_b.points[i+1]
            d = self.outline_b.points[i]
            curves.append(NurbsCurve.from_points([a, b, c, d, a], degree=1))
        plate_geo = Brep.from_curves(curves)
        if len(plate_geo) != 1:
            raise ValueError("The curves could not be joined into a single Brep.")  
        else:   
            plate_geo = plate_geo[0] 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, fair enough!

@obucklin obucklin requested a review from chenkasirer April 3, 2025 12:26
obucklin and others added 2 commits April 9, 2025 11:05
Co-authored-by: Chen Kasirer <chen902@gmail.com>
Copy link
Member

@chenkasirer chenkasirer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM!

@chenkasirer chenkasirer merged commit 8496686 into compas-dev:main Apr 9, 2025
16 checks passed
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