Geometry Refactoring #1163
Merged
Geometry Refactoring #1163
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lementing this has many side effects
tomvanmele
requested review from
Licini,
chenkasirer,
gonzalocasas and
romanarust
Member
Author
|
@gonzalocasas @chenkasirer @Licini i would (review and) merge as is and add more docs and tests later. otherwise this PR will be never-ending... |
Member
Author
|
? |
Member
Agree. I will review today |
chenkasirer
approved these changes
Jul 31, 2023
Member
chenkasirer
left a comment
chenkasirer
left a comment
There was a problem hiding this comment.
LGTM
Note: I went though the commits themselves and made my comments so maybe some of comments are not relevant cause the code changed in a later commit.
Comment on lines
166
to
176
| # mean, vectors, values = pca_numpy(points) | ||
| # frame = mean, vectors[0], vectors[1] | ||
| # points = world_to_local_coordinates_numpy(frame, points) | ||
|
|
||
| rect1 = minimum_area_rectangle_xy(points) | ||
| # area1 = (rect1[1][0] - rect1[0][0]) * (rect1[3][1] - rect1[0][1]) | ||
| # rect2 = bounding_box(points)[:4] | ||
| # area2 = (rect2[1][0] - rect2[0][0]) * (rect2[3][1] - rect2[0][1]) | ||
| # rect = rect1 if area1 < area2 else rect2 | ||
| # rect = [[x, y, 0.0] for x, y in rect1] | ||
| # bbox = local_to_world_coordinates_numpy(frame, rect) |
Comment on lines
+16
to
+18
| curve = object.__new__(NurbsCurve) | ||
| curve.__init__(*args, **kwargs) | ||
| return curve |
Member
There was a problem hiding this comment.
wait can we do that? I thought that didn't work..
| def __len__(self): | ||
| return 4 | ||
|
|
||
| def __getitem__(self, key): |
Member
There was a problem hiding this comment.
maybe it's worth somehow mentioning this in the main docstring, otherwise I wouldn't expect Box to be subscriptable.
Member
Author
There was a problem hiding this comment.
was rather thinking of removing these accessors from most objects, only keeping them for fundamental geometries like point, vector, plane, frame, etc.
Comment on lines
+491
to
+539
| # @classmethod | ||
| # def from_points(cls, points): | ||
| # """Construct a box from a set of points. | ||
|
|
||
| # Parameters | ||
| # ---------- | ||
| # points : list[:class:`~compas.geometry.Point`] | ||
| # A list of points. | ||
|
|
||
| # Returns | ||
| # ------- | ||
| # :class:`~compas.geometry.Box` | ||
| # The resulting box. | ||
|
|
||
| # """ | ||
|
|
||
| # ========================================================================== | ||
| # methods | ||
| # Transformations | ||
| # ========================================================================== | ||
|
|
||
| # def transform(self, transformation): | ||
| # """Transform the box. | ||
|
|
||
| # Parameters | ||
| # ---------- | ||
| # transformation : :class:`Transformation` | ||
| # The transformation used to transform the Box. | ||
|
|
||
| # Returns | ||
| # ------- | ||
| # None | ||
|
|
||
| # Examples | ||
| # -------- | ||
| # >>> box = Box(Frame.worldXY(), 1.0, 2.0, 3.0) | ||
| # >>> frame = Frame([1, 1, 1], [0.68, 0.68, 0.27], [-0.67, 0.73, -0.15]) | ||
| # >>> T = Transformation.from_frame(frame) | ||
| # >>> box.transform(T) | ||
|
|
||
| # """ | ||
| # self.frame.transform(transformation) | ||
| # # Always local scaling, non-uniform scaling based on frame not yet considered. | ||
| # Sc, _, _, _, _ = transformation.decomposed() | ||
| # scalex = Sc[0, 0] | ||
| # scaley = Sc[1, 1] | ||
| # scalez = Sc[2, 2] | ||
| # self.xsize *= scalex | ||
| # self.ysize *= scaley | ||
| # self.zsize *= scalez |
src/compas/geometry/shapes/shape.py
Outdated
Comment on lines
61
to
84
| @property | ||
| def data(self): | ||
| raise NotImplementedError | ||
|
|
||
| @data.setter | ||
| def data(self, data): | ||
| raise NotImplementedError | ||
|
|
||
| @classmethod | ||
| def from_data(cls, data): | ||
| """Construct a shape from its data representation. | ||
| Parameters | ||
| ---------- | ||
| data : dict | ||
| The data dictionary. | ||
| Returns | ||
| ------- | ||
| :class:`~compas.geometry.Shape` | ||
| The constructed shape. | ||
| """ | ||
| return cls(**data) |
Member
There was a problem hiding this comment.
Isn't this a redefinition of the Data interface?
Comment on lines
+599
to
+614
| # def patch(self, u, v, du=1, dv=1): | ||
| # """Construct a NURBS surface patch from the surface at the given UV parameters. | ||
|
|
||
| # Parameters | ||
| # ---------- | ||
| # u : float | ||
| # v : float | ||
| # du : int, optional | ||
| # dv : int, optional | ||
|
|
||
| # Returns | ||
| # ------- | ||
| # :class:`~compas.geometry.NurbsSurface` | ||
|
|
||
| # """ | ||
| # raise NotImplementedError |
|
|
||
| center, radius = bestfit_sphere_numpy(points) | ||
| return cls(radius, frame=Frame(center, [1, 0, 0], [0, 1, 0])) | ||
|
|
Member
Author
There was a problem hiding this comment.
it is rather the other way around. the surfaces form the underlying geometry of shapes. so the idea would be to redefine the shapes such that they use an underlying surface geometry or geometries to define the shape. a bit like how breps work...
Comment on lines
479
to
490
| # T[0, 0] = 1 | ||
| # T[1, 1] = 1 | ||
| # T[2, 2] = 1 | ||
| # T[3, 3] = 1 | ||
|
|
||
| # T[0, 3] = 0 | ||
| # T[1, 3] = 0 | ||
| # T[2, 3] = 0 | ||
|
|
||
| # T[3, 0] = 0 | ||
| # T[3, 1] = 0 | ||
| # T[3, 2] = 0 |
| """ | ||
| circle = Circle.from_three_points(a, b, c) | ||
| return cls(circle.radius, frame=circle.frame) | ||
|
|
Comment on lines
149
to
190
| # def normal_at(self, u): | ||
| # """Compute the normal at a point on the surface at the given parameters. | ||
|
|
||
| # Parameters | ||
| # ---------- | ||
| # u : float | ||
| # The first parameter. | ||
|
|
||
| # Returns | ||
| # ------- | ||
| # :class:`compas.geometry.Vector` | ||
| # The normal at the given parameters. | ||
|
|
||
| # """ | ||
| # u = u * 2 * pi | ||
| # x = self.radius * cos(u) | ||
| # y = self.radius * sin(u) | ||
| # z = 0 | ||
| # return self.frame.xaxis * x + self.frame.yaxis * y + self.frame.zaxis * z | ||
|
|
||
| # def frame_at(self, u, v): | ||
| # """Compute the frame at a point on the surface at the given parameters. | ||
|
|
||
| # Parameters | ||
| # ---------- | ||
| # u : float | ||
| # The first parameter. | ||
| # v : float | ||
| # The second parameter. | ||
|
|
||
| # Returns | ||
| # ------- | ||
| # :class:`compas.geometry.Frame` | ||
| # The frame at the given parameters. | ||
|
|
||
| # """ | ||
| # u = u * 2 * pi | ||
| # point = self.point_at(u, v) | ||
| # zaxis = self.normal_at(u) | ||
| # yaxis = self.frame.zaxis | ||
| # xaxis = yaxis.cross(zaxis) | ||
| # return Frame(point, xaxis, yaxis) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.