Surface from rhino #1350
Surface from rhino #1350
Conversation
…RhinoNurbsSurface.__data__ has pre-processing
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1350 +/- ##
=======================================
Coverage ? 59.90%
=======================================
Files ? 206
Lines ? 22162
Branches ? 0
=======================================
Hits ? 13277
Misses ? 8885
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| data["degree_v"], | ||
| data["is_periodic_u"], | ||
| data["is_periodic_v"], | ||
| frame, |
There was a problem hiding this comment.
i am not so sure that any of the methods of the surface class take this frame into account. currently, the frame is not settable and always defaults to world XY...
| @property | ||
| def __data__(self): | ||
| return { | ||
| "frame": self._frame.__data__, | ||
| "domain_u": list(self.domain_u), | ||
| "domain_v": list(self.domain_v), | ||
| } | ||
|
|
||
| @classmethod | ||
| def __from_data__(cls, data): | ||
| frame = Frame.__from_data__(data["frame"]) | ||
| domain_u = tuple(data["domain_u"]) | ||
| domain_v = tuple(data["domain_v"]) | ||
| return cls.from_plane(frame, domain_u, domain_v) |
There was a problem hiding this comment.
since the base surface is abstract and since it should not be possible to create instances of it directly, i don't think it should provide the possibility to create one from data...
There was a problem hiding this comment.
from_plane will invoke the plugin, if available. it just seems to be the canonical surface representation (maybe with exception of frame which it seems I didn't quite get)
There was a problem hiding this comment.
but why should we provide functionality for making a "canonical" surface from data, when the surface can't actually by instantiated in the normal way. i though we agreed that only the spacial constructors can be used to create an actual surface and that the base class only provides the shared interface...
There was a problem hiding this comment.
for CAD agnostic (de)serialization? maybe what's confusing me is that lack of other kinds of surface types
maybe this should be in a PlanarSurface like in Rhino..
There was a problem hiding this comment.
there are plenty of other surface types (including planar surface). they just don't require a Rhino or OCC plugin...
There was a problem hiding this comment.
compas.geometry.surfaces has CylindricalSurface, ConicalSurface, PlanarSurface, SphericalSurface, ToroidalSurface
| def _get_frame_from_planesurface(self): | ||
| u_start = self.domain_u[0] | ||
| v_start = self.domain_v[0] | ||
| success, frame = self.native_surface.FrameAt(u_start, v_start) | ||
| if not success: | ||
| raise ValueError("Failed to get frame at u={} v={}".format(u_start, v_start)) | ||
| return plane_to_compas_frame(frame) |
There was a problem hiding this comment.
i don't think this is correct. control points are usually in world coordinates, not in local coordinates wrt to a frame positioned at the beginning of the domain.
| instance.frame = instance._get_frame_from_planesurface() | ||
| instance._domain_u = native_surface.Domain(0)[0], native_surface.Domain(0)[1] | ||
| instance._domain_v = native_surface.Domain(1)[0], native_surface.Domain(1)[1] |
There was a problem hiding this comment.
this doesn't make sense to me
There was a problem hiding this comment.
the domain part also?
There was a problem hiding this comment.
afaik the u and v domain are read from the native surface by the corresponding properties. they are never set...
There was a problem hiding this comment.
but my comment was mostly about the frame thing indeed
|
replaced by #1375 |
Surfacepluggablefrom_planeso that it's possible to create a planar surface in Rhino.from_nativeinnative_surfaceinSurfaceandRhinoSurfaceto align better with theBrepsystem.RhinoSurface.from_rhino.Boxargument withu_domainandv_domaininRhinoSurface.from_plane.new_surfacepluggin to create aRhinoSurfaceobject instead ofSurface.SurfaceandNurbsSurfaceare protected. Caller must use one of the alternative constructors.https://miro.com/app/board/uXjVK2eWeOk=/
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.