Rhino plugin for nurbs surfaces #952
Conversation
|
|
||
| 'RhinoNurbsCurve' | ||
| 'RhinoNurbsCurve', | ||
| 'RhinoNurbsSurface', |
There was a problem hiding this comment.
Sorry my first time looking at this part. I'm a bit confused here RhinoSurface and RhinoNurbsSurface are very similarly named, and they are more or less both wrappers of Rhino geometry instances, however they have quite different kind of APIs, what's the designed context for each of these classes?
There was a problem hiding this comment.
RhinoSurface is a backwards compatibility import from compas_rhino.conversions that allows Rhino surface objects and geometries to be converted to compas equivalents.
RhinoNurbsSurface is the Rhino plugin implementation of compas.geometry.NurbsSurface
| import Rhino.Geometry | ||
|
|
||
|
|
||
| def rhino_surface_from_parameters(points, weights, u_knots, v_knots, u_mults, v_mults, u_degree, v_degree, is_u_periodic=False, is_v_periodic=False): |
There was a problem hiding this comment.
Nitpicking here, just feel a bit strange of this function hanging here outside with a class, could it be part of class or in a collection of helpers in case it can be re-used elsewhere? (Same goes with RhinoNurbsCurve)
gonzalocasas
left a comment
gonzalocasas
left a comment
There was a problem hiding this comment.
sorry the late reviewing...
LGTM!!! 💯
| * Fixed bug in `compas_rhino.conversions.plane_to_compas_frame`. | ||
| * Changed implementation of `compas.geometry.NurbsSurface.xyz`. |
There was a problem hiding this comment.
Not to change right now, but these messages should be a tad more specific, both fixed bug and changed impl are so generic that a user would need to go read the code to understand what's being talked about.
| :class:`ConversionError` | ||
| If the surface BRep contains more than one face. | ||
| """ | ||
| from compas.geometry import NurbsSurface |
There was a problem hiding this comment.
any reason for this import to be inlined instead of at the top?
There was a problem hiding this comment.
was originally because of a circular import but perhaps this is not the case anymore. will investigate...
What type of change is this?
Checklist
Put an
xin 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.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.