Geometry brep #1057

chenkasirer · 2022-07-13T14:57:14Z

WIP: added initial implementation of a compas.geometry.BRep with a Rhino backend in compas_rhino.geometry.brep.

Copied over the interface for BRep from compas_occ into compas.geometry.BRep and generalized.
Started implementing the package compas_rhino.geometry.brep with wrapping types for brep, edge, vertex and loop.
Implemented the serializing part of data (in some also the setter but not tested)

I could serialize a simple box brep with:

from pprint import pprint
from compas.geometry import Box
from compas.geometry import BRep
from compas.data import json_dumps

box = Box.from_width_height_depth(5., 5., 10.)
brep = BRep.from_box(box)

pprint(json_dumps(brep))

Opening this draft PR to start a conversation about the approach.

What type of change is this?

New feature in a backwards-compatible manner.

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)

src/compas/geometry/brep/brep.py

gonzalocasas · 2022-07-13T15:08:30Z

src/compas/geometry/brep/brep.py

+    type : {TopAbs_COMPOUND, TopAbs_COMPSOLID, TopAbs_SOLID, TopAbs_SHELL, TopAbs_FACE, TopAbs_WIRE, TopAbs_EDGE, TopAbs_VERTEX, TopAbs_SHAPE}, read-only
+        The type of BRep shape.
+    orientation : {TopAbs_FORWARD, TopAbs_REVERSED, TopAbs_INTERNAL, TopAbs_EXTERNAL}, read-only


also this constants seem to be occ specific, right?

yes they are. we could wrap this and make it more general so it applies to both/all backends...

Question about this: do we need an external/interface representation for these? In compas_occ I've only seen them being used via is_compound(), is_shell() etc.. If that's the case we don't really need a generic representation. Do you see a need?

no, no need...

src/compas/geometry/brep/brep.py

tomvanmele

good start! :)

src/compas/geometry/brep/__init__.py

src/compas_rhino/geometry/brep/__init__.py

tomvanmele · 2022-07-13T20:58:24Z

src/compas/geometry/__init__.py

-    'NurbsSurface'
+    'NurbsSurface',
+
+    "Brep",


will need to expose vertex, edge, loop, and face as well...

tomvanmele · 2022-07-13T20:59:20Z

src/compas_rhino/geometry/brep/brep.py

+class RhinoBRep(Geometry):
+    def __init__(self):
+        super(RhinoBRep, self).__init__()
+        self.rhino_brep = None


self._brep?
and the same for compas_occ then, instead of self.occ_shape?

tomvanmele · 2022-07-13T21:00:37Z

src/compas_rhino/geometry/brep/brep.py

+
+    @classmethod
+    def from_faces(cls, faces):
+        brep = Brep()


To be implemented.

While we're here, this part will have to be a bit different than compas_occ. BrepEdge, BrepFace and even BrepVertex all don't have a public constructor and can only be created via the Brep instance (e.g. Brep().Vertices.Add(Point3d)).

Consequently, de-serializing a compas_rhino.geometry.RhinoBRepVertex alone does not result in a Rhino.Geometry.BrepVertex instance. Therefore, I imagine the de-serialized building blocks will be containing COMPAS types and the actual reconstruction of the native Rhino Brep will take place here, in from_faces. It can then also "inject" the native types to each of the underlying objects.

Do you see a better way to go about it?

no, that makes sense to me. in fact, also in compas_occ we should try to funnel everything through the Brep class, and expose the other types only through the element accessors (.vertices, .edges, .loops, .faces), or for development pruposes.

src/compas_rhino/geometry/brep/brep.py

src/compas_rhino/geometry/brep/edge.py

tomvanmele · 2022-07-13T21:07:25Z

src/compas_rhino/geometry/brep/face.py

+
+class RhinoBRepFace(Data):
+
+    TOLERANCE = 1e-6


need a better solution for this.
also just in general, we need a formal location to store compas-wide configuration options...

@gonzalocasas mentioned once these can be handled with some global COMPAS precision system. I'll look into it.

well, i wouldn't necessarily call it a system, but there is indeed a precision setting compas.PRECISION :)
a more robust version of this that also allows for other settings would be a good thing...

src/compas_rhino/geometry/brep/face.py

tomvanmele

this is an old review i forgot to submit so it might be a bit outdated...

src/compas_rhino/geometry/brep/brep.py

src/compas/geometry/brep/brep.py

src/compas/geometry/brep/edge.py

docs/tutorial/brep.rst

src/compas/geometry/brep/brep.py

src/compas/geometry/brep/vertex.py

src/compas_rhino/geometry/brep/brep.py

tomvanmele · 2022-08-23T08:54:28Z

src/compas_rhino/geometry/brep/brep.py

+        """
+        Parameters
+        ----------
+        data
+
+        Returns
+        -------
+
+        """


setters are meant to be documented on the getter...

tomvanmele · 2022-08-23T08:55:40Z

src/compas_rhino/geometry/brep/brep.py

+        """
+        Returns the native representation of this Brep.
+
+        Returns
+        -------
+        :class: `Rhino.Geometry.Brep`
+        """


any reason some of the properties and others not? perhaps leave this one out as well?

You mean why there's public access to the native type in Brep but not in the others?

I found this is useful at the moment due to the lack of a BrepArtist for example. That way you can plug-in Brep.native_brep to a GH geometry component to visualize.

Maybe once there's more extensive support for Brep in COMPAS it would make sense to remove this one as well.

no sorry, there were some crucial words missing...

any reason some of the properties HAVE DOCSTRINGS and others not

No particular reason ;)
Removed all docstrings from properties and added to Attributes instead.

tomvanmele · 2022-08-24T14:12:52Z

src/compas/geometry/brep/face.py

+from compas.data import Data
+
+
+class BrepFace(Data):


if i remember correctly this creates major problem in ironpython with memory leaks etc

Chen Kasirer added 3 commits July 12, 2022 18:17

WIP: start of a compas.core.Brep implementation

7e1d7e6

added basic implementations for edge, face, vertex and loop

a062549

formatted with black

ab65dc7

chenkasirer requested review from gonzalocasas and tomvanmele July 13, 2022 14:58

gonzalocasas reviewed Jul 13, 2022

View reviewed changes

tomvanmele reviewed Jul 13, 2022

View reviewed changes

Chen Kasirer added 12 commits July 18, 2022 16:02

BRep->Brep, fixed pluging signatures, fixed call to default constructor

9027829

added interfaces for Brep components in compas.geometry

f937532

fixed serialization

96b0e1a

WIP: reconstructing brep

19b5dd6

solved invalid geometry

4075a41

valid brep reconstruction

84d356b

tidying of brep reconstruction

c2b3724

added error handling to brep reconstruction

ad9ccda

added circle and nurbs curves to possible edge geometries

82c53f5

cleanup, linting and formatting

1d6dd82

updated CHANGELOG

c86095d

fixed ambiguous variable names

6536d35

chenkasirer requested review from romanarust and tomvanmele July 22, 2022 18:12

Chen Kasirer added 3 commits July 25, 2022 11:47

updated some comments

86519a3

implemented Geometry interface in Brep, removed None syntax error

d67a810

added documentation, started Brep json schema

cc3a022

chenkasirer marked this pull request as ready for review July 26, 2022 15:13

Chen Kasirer added 2 commits July 26, 2022 19:14

added trim operation to brep

89d4d53

updated tutorial

62e9f1c

tomvanmele reviewed Jul 27, 2022

View reviewed changes

Chen Kasirer added 2 commits July 27, 2022 11:54

fixed indentation errors in tutorial

81dc3cf

removed already implemented methods

ddc6cb9

Chen Kasirer and others added 8 commits July 27, 2022 17:35

added docstrings

f636dc8

fixed weird trimming bug on de-serialized brep

64fa58b

dtype hack for backend agnostic de-serialization of brep

550099d

added turorial screenshots

713141b

removed unused import

66c3335

Merge branch 'main' into geometry_brep

0b92fdb

completed missing docstrings

a40516d

formatted with black

c529ec1

chenkasirer requested review from gonzalocasas and tomvanmele August 15, 2022 14:27

tomvanmele reviewed Aug 23, 2022

View reviewed changes

chenkasirer added 4 commits August 23, 2022 11:28

new classes inherit from object

877fed9

fixed brep docstrings

a2e2fce

standardized docstrings in brep modules

48d9a9e

remove property docstrings

7879313

tomvanmele approved these changes Aug 24, 2022

View reviewed changes

chenkasirer added 7 commits August 25, 2022 11:34

standardized docstrings in compas_rhino

2042d25

small docstring corrections

937da8e

removed unused module attribute

b273650

Brep.trim takes cutting plane also as Plane

d2540f7

black formatting

0e62739

fixed typo in conf.py

84b8dfd

minor docs fixes

4d5d249

chenkasirer merged commit ec1b9f8 into main Aug 26, 2022

chenkasirer deleted the geometry_brep branch August 26, 2022 08:12

chenkasirer mentioned this pull request Oct 4, 2022

Brep artist #1074

Merged

7 tasks

chenkasirer mentioned this pull request May 3, 2023

Precision system #1140

Closed

Geometry brep #1057

Geometry brep #1057

Uh oh!

Conversation

chenkasirer commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of change is this?

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomvanmele left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomvanmele left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

chenkasirer commented Jul 13, 2022 •

edited

Loading