Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Wall rework #386

wants to merge 4 commits into from

Conversation

chenkasirer
Copy link
Contributor

This rework to Wall is an extract from #336 and standalone enough to be reviewed separately.

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

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.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas_timber.datastructures.Beam.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@chenkasirer chenkasirer requested review from obucklin and papachap and removed request for obucklin February 6, 2025 10:43
Copy link
Contributor

@obucklin obucklin left a 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
Copy link
Contributor

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):
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about doing something similar for doors, but I'm not sure this is a good solution. what if you have a wall shaped like this:
image

Copy link
Contributor

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(
Copy link
Contributor

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(
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.")
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants