-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wall rework #386
base: main
Are you sure you want to change the base?
Wall rework #386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I left some comments. In general it seems to be quite limited and maybe only applies to rectangular walls. If this is the case then we could define with a rectangle or a box and make it even simpler. There are also some functions which are already implemented elsewhere which could be reused.
What is your intention regarding the beam populator? Use the existing SurfaceModel
code and input the wall attributes?
@@ -21,17 +24,28 @@ class Wall(TimberElement): | |||
@property | |||
def __data__(self): | |||
data = super(Wall, self).__data__ | |||
data["width"] = self.width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this a bit. Would it make sense to define a wall as a box, like we do with beams? This would presumably translate well to the Hüllkörper.
return Frame(points[0], xaxis, yaxis) | ||
|
||
@staticmethod | ||
def _oriented_polyline(polyline, normal): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems very limited and also wouldn't work on a polyline on the xy plane. also don't forget the 5th point needed to close the Polyline. Maybe use the correct_polyline_direction()
from the PR #382
else: | ||
internal_vertices.append((i + 1) % num_points) | ||
|
||
# vertices that lie on the outline but are at openings count as internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe openings should be reserved for doors or windows which generate further framing elements. Not sure how to parse this.
|
||
|
||
def test_wall_with_door_openings(): | ||
outline = Polyline( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this Polyline is open
|
||
|
||
def test_wall_with_door_openings_ccw(): | ||
outline = Polyline( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this Polyline is open
return Box.from_bounding_box(bounding_box(obb.points)) | ||
|
||
def compute_obb(self, inflate_by=0.0): | ||
assert self.frame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is self.obb
supposed to be aligned to self.frame
?
end = self.frame.point + self.frame.xaxis * self.length | ||
return Line(start, end) | ||
if not self._corners: | ||
points = self.outline.points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes 4-sided outline. Why not make a box that contains an arbitrary number of points?
if not outline.is_closed: | ||
raise ValueError("Outline is not closed.") | ||
if len(self.outline) != 5: | ||
raise ValueError("Wall outline must have 4 segments.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why limit yourself to 4? does it need to be rectangle?
This rework to
Wall
is an extract from #336 and standalone enough to be reviewed separately.What type of change is this?
Checklist
Put an
x
in 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.md
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).compas_timber.datastructures.Beam
.