Rhino brep #1106
Rhino brep #1106
Conversation
chenkasirer
requested review from
gonzalocasas and
tomvanmele
and removed request for
tomvanmele
src/compas/geometry/brep/trim.py
Outdated
| West = 3 | ||
| South = 4 | ||
| East = 5 | ||
| North = 6 |
There was a problem hiding this comment.
should these be uppercased?
| def _make_surface_from_data(surface_type, surface_data, uv_domain): | ||
| if surface_type == "plane": | ||
| frame = Frame.from_data(surface_data) | ||
| surface = RhinoNurbsSurface.from_frame(frame, uv_domain[0], uv_domain[1], (1, 1), (2, 2)) |
There was a problem hiding this comment.
what's the meaning of (1, 1), (2, 2)? does it need a comment?
There was a problem hiding this comment.
These are the required constant to create a simple planar NURBS surface with first degree and 2 control point in each direction (4 corners). Agreed with @brgcode these will be moved as default kwargs to the signature of from_frame since these will normally be used.
| def _get_curve_geometry(self): | ||
| curve = self._edge.EdgeCurve | ||
| if self.is_line: | ||
| type_ = "line" |
There was a problem hiding this comment.
what's the meaning of the trailing underscore?
There was a problem hiding this comment.
sometimes I insist on naming a var type so this is to avoid shadowing the builtin. It's apparently PEP8:
single_trailing_underscore_: used by convention to avoid conflicts with Python keyword, e.g.:
|
would it not make more sense to add stuff to the builder instead of adding the builder to each of the |
CHANGELOG.md
Outdated
| ### Added | ||
|
|
||
| * Added conversion function `frame_to_rhino_plane` to `compas_rhino.conversions`. | ||
| * Added `RhinoSuface.from_frame` to `compas_rhino.geometry`. |
There was a problem hiding this comment.
not important but there is an "r" missing in Surface
| builder = RhinoBrepBuilder() | ||
| for v_data in data["vertices"]: | ||
| RhinoBrepVertex.from_data(v_data, builder) | ||
| for e_data in data["edges"]: | ||
| RhinoBrepEdge.from_data(e_data, builder) | ||
| for f_data in data["faces"]: | ||
| RhinoBrepFace.from_data(f_data, builder) | ||
| self._brep = builder.result |
There was a problem hiding this comment.
would it not make more sense to add stuff to the builder rather than to modify the signature of all the constructors to send the builder around?
There was a problem hiding this comment.
The builder serves:
- keeps the reconstruction logic in a single place
- keeps the reconstruction logic separate from the de-serialization
- makes the reconstruction context (i.e.
Brep) available to each of the components while avoiding global state - enables the de-serialized elements to hold the native
Rhino.Geometrytype immediately after de-serialization.
BrepBuilder is the main context, it essentially wraps around the newly created Rhino.Geometry.Brep and holds it until it's properly built.
Since Face, Loop and Trim are nested in each other:
BrepFaceBuilder wraps around Rhino.Geometry.BrepFace and handed over to the loops so that they can add themselves to the face.
BrepLoopBuilder wraps around Rhino.Geometry.BrepLoop and handed over to the trims so that they can add themselves to the loop.
The result property of the builders gives access to the newly built native element once it's ready. BrepLoopBuilder.result returns the Rhino.Geometry.BrepLoop after all the trims have been added to it. BrepFaceBuilder.result returns the Rhino.Geometry.BrepFace after all the loops have been added to it. BrepBuilder.result returns the Rhino.Geometry.Brep after all the vertices, edges and faces have been added to it.
| def result(self): | ||
| return self.loop |
There was a problem hiding this comment.
is this loop somehow updated in the add_trim method
There was a problem hiding this comment.
actually, how/when/where is this result attribute actually used (in any of the builder classes)?
| """ | ||
|
|
||
| def __init__(self, loop=None, instance=None): |
There was a problem hiding this comment.
could you add the types of these parameters in the docstring above because it is not very clear to me what they are supposed to be
There was a problem hiding this comment.
renamed and added to docstrings to maybe make some more sense
|
|
||
| @classmethod | ||
| def from_frame(cls, frame, u_interval, v_interval, uv_degrees, uv_point_counts): | ||
| def from_frame(cls, frame, u_interval, v_interval, uv_degrees=(1, 1), uv_point_counts=(2, 2)): |
There was a problem hiding this comment.
would it not make more sense to do u_degree=1, v_degree=1, u_count=2, v_count=2? since this is how it is done for the intervals as well...
Updated
RhinoBrepserialization format and reconstruction procedure to fix invalid reconstruction on more complex shapes. Highlights are:Changed serialization format to eliminate duplicate elements.
Serializing to/from primitives instead of NURBS where possible
Moved brep reconstruction logic from
RhinoBrepover to a the newbrep/builder.pymodule.Added representation for trims with
BrepTrim.Added
RhinoNurbsSurface.from_frameBug 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
CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.