Small fixes#1472
Conversation
tomvanmele
commented
Jun 4, 2025
tomvanmele
commented
- fixed context detection (default should be None and not Viewer)
- fixed discretisation of polyhedron into edges (when data comes from numpy)
- fixed graph construction from pointcloud which used degree parameter in nonsensical way
- added isocurve_u and isocurve_v for spherical and cylindrical surfaces
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1472 +/- ##
==========================================
- Coverage 61.98% 61.93% -0.05%
==========================================
Files 208 208
Lines 22394 22427 +33
==========================================
+ Hits 13880 13890 +10
- Misses 8514 8537 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gonzalocasas
left a comment
gonzalocasas
left a comment
There was a problem hiding this comment.
generally lgtm except the scary commented out context code. The other things are more like an open question
| def isocurve_u(self, u): | ||
| """Compute the isoparametric curve at parameter u. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| u : float | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`compas.geometry.Line` | ||
|
|
||
| """ | ||
| base = self.point_at(u=u, v=0.5) | ||
| return Line.from_point_direction_length(base, self.frame.zaxis, 1.0) | ||
|
|
||
| def isocurve_v(self, v): | ||
| """Compute the isoparametric curve at parameter v. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| v : float | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`compas.geometry.Circle` | ||
|
|
||
| """ | ||
| point = self.center + self.frame.zaxis * v | ||
| xaxis = self.frame.xaxis | ||
| yaxis = self.frame.yaxis | ||
| frame = Frame(point, xaxis, yaxis) | ||
| return Circle(radius=self.radius, frame=frame) |
There was a problem hiding this comment.
I understand the logical reason for returning Circle and Line, but wouldn't make more sense to return Curve instances always? That way both _u and _v would be symmetrical and also this API would be consistent with the one of Surface
There was a problem hiding this comment.
we don't have a functional general curve
There was a problem hiding this comment.
afaik that is also not possible. you need a way to evaluate the curve and this depends on the type of curve it is...
| # other_contexts = [v for v in ITEM_SCENEOBJECT.keys()] | ||
| # if other_contexts: | ||
| # return other_contexts[0] |
There was a problem hiding this comment.
This looks dangerous to commit, isn't that going to break stuff, like the viewer?
There was a problem hiding this comment.
viewer and notebook set the context explicitly so they are unaffected. the fact that we return the viewer instead of none is just by coincidence and actually masks bugs and creates unpredictable behavior
There was a problem hiding this comment.
Just to confirm, I did few tests with viewer too they are not affected
There was a problem hiding this comment.
Good, then let's delete instead of comment out
| def isocurve_u(self, u): | ||
| """Compute the isoparametric curve at parameter u. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| u : float | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`compas.geometry.Circle` | ||
|
|
||
| """ | ||
| origin = self.center | ||
| xaxis = self.point_at(u=u, v=0) - origin | ||
| yaxis = self.point_at(u=u, v=0.25) - origin | ||
| frame = Frame(origin, xaxis, yaxis) | ||
| return Circle(radius=xaxis.length, frame=frame) | ||
|
|
||
| def isocurve_v(self, v): | ||
| """Compute the isoparametric curve at parameter v. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| v : float | ||
|
|
||
| Returns | ||
| ------- | ||
| :class:`compas.geometry.Circle` | ||
|
|
||
| """ | ||
| x = self.point_at(u=0, v=v) | ||
| y = self.point_at(u=0.25, v=v) | ||
| origin = self.center + self.frame.zaxis * (x - self.center).dot(self.frame.zaxis) | ||
| xaxis = x - origin | ||
| yaxis = y - origin | ||
| frame = Frame(origin, xaxis, yaxis) | ||
| return Circle(radius=xaxis.length, frame=frame) |
There was a problem hiding this comment.
Same comment here about aligning these methods to a unified API that returns Curve always
There was a problem hiding this comment.
this is not possible because the base curve class just defines an api but no functionality
tomvanmele
left a comment
tomvanmele
left a comment
There was a problem hiding this comment.
apparently i started a review so i have to leave a comment to submit my comments :)
3 participants
