Curve functionalities #6
Conversation
| Returns | ||
| ------- | ||
| None |
There was a problem hiding this comment.
return value should be updated to the type of the returned object.
There was a problem hiding this comment.
in the segment (vs. segmented) nothing is returned actually (using the same convention as in compas, e.g transform vs. transformed). -> in that case, should there then be no Return section at all? (in compas this appears inconsistent, sometimes it returns None, sometimes there is no return at all...)
There was a problem hiding this comment.
returning nothing or returning None is the same. but you are right that the return type was indeed correct since the curve is modified in place
| return GCPnts_AbscissaPoint_Length(GeomAdaptor_Curve(self.occ_curve)) | ||
|
|
||
| def segment(self, u: float, v: float, precision: float = 1e-3) -> None: | ||
| """Modifies this curve by segmenting it between the parameters u and v. |
There was a problem hiding this comment.
could you add a comment about what segmenting actually is
There was a problem hiding this comment.
Not sure what you mean...
Maybe something like " a segment is a portion of the curve bounded by the points of two curve parameters u and v." ?
There was a problem hiding this comment.
to something like "Reduces the curve to the segment between parameters u and v." is a lot more descriptive of what actually happens
| if u < s or v > e: | ||
| raise ValueError('At least one of the given parameters is outside the curve domain.') |
There was a problem hiding this comment.
as far as i understand the OCC docs, is this requirement not necessary
There was a problem hiding this comment.
in the case of e.g. u<v, Occ gives this error:
"RuntimeError: Standard_ConstructionErrorGeom_BSplineCurve::InsertKnots raised from method Segment of class Geom_BSplineCurve"
it just doesn't really become clear in my opinion what the problem is in this descriptionion.
What do you think?
| raise ValueError('At least one of the given parameters is outside the curve domain.') | ||
| if u == v: | ||
| raise ValueError('The given domain is zero length.') | ||
| else: |
There was a problem hiding this comment.
this else is not necessary because the if triggers an error
| point = Point.from_occ(projector.NearestPoint()) | ||
| if not parameter: | ||
| return point | ||
| else: |
There was a problem hiding this comment.
else is not necessary because if already returns
|
btw, i took the liberty of already making those changes and merging the PR |
|
thanks for that! |
2 participants
added new features in a backwards-compatible manner together with example files:
NurbsCurve.segment(...)NurbsCurve.segmented(...)NurbsCurve.closest_point(...)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.