`FreeContour` BTLxProcessing #382

obucklin · 2025-01-30T13:58:03Z

This adds the FreeContour BTLx Processing and updates the Plate element to function as a BTLx Part. The Plate.geometry is now created with FreeContour (and other) processings. The SurfaceModel and SurfaecModel.Window have been updated to generate FreeContour features and output BTLx files with all elements.

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

left some comments.
if @papachap can have a look it would be great

src/compas_timber/fabrication/btlx.py

src/compas_timber/fabrication/free_contour.py

src/compas_timber/fabrication/btlx.py

chenkasirer · 2025-02-06T09:03:02Z

src/compas_timber/fabrication/btlx.py

+        # create subprocessing elements
+        if self.subprocessings:
+            for subprocessing in self.subprocessings:
+                processing_element.append(self._create_processing(subprocessing))


as BTLxWriter_create_processing() gets removed, recursively call create_processing here.

@papachap can you confirm that this works as currently written? is there a processing we could test this on?

CHANGELOG.md

obucklin · 2025-02-06T17:50:25Z

I responded to @chenkasirer s comments. Will wait for @papachap :-)

added test, removed writer._create_part

papachap · 2025-02-10T12:46:38Z

src/compas_timber/fabrication/btlx.py

@@ -155,8 +155,8 @@ def _create_project_element(self, model):
        # create parts element
        parts_element = ET.SubElement(project_element, "Parts")
        # create part elements for each beam
-        for i, beam in enumerate(model.beams):  # TODO: we need to add at least Plates to this too.
-            part_element = self._create_part(beam, i)
+        for i, element in enumerate(list(model.beams) + list(model.plates)):


maybe update the _create_part() method as well to accept an element instead of a beam and adjust the docstring.

papachap · 2025-02-10T12:52:14Z

src/compas_timber/fabrication/btlx.py

    processings : list
        A list of the processings applied to the beam.
    et_element : :class:`~xml.etree.ElementTree.Element`
        The ET element of the BTLx part.

    """

-    def __init__(self, beam, order_num):
-        self.beam = beam
+    def __init__(self, element, order_num):


Please update the docstring accordingly.

papachap · 2025-02-10T13:01:00Z

src/compas_timber/fabrication/btlx.py

+        )
+        # create parameter subelements
+        for key, value in self.params_dict.items():
+            print(key, value)


remove print

papachap · 2025-02-10T18:06:15Z

src/compas_timber/elements/plate.py

@@ -177,7 +193,7 @@ def compute_aabb(self, inflate=0.0):
        box.zsize += inflate
        return box

-    def compute_obb(self, inflate=0.0):
+    def compute_obb(self):


update docstring

papachap · 2025-02-11T08:33:42Z

src/compas_timber/elements/plate.py

+    @property
+    def blank_length(self):
+        return self._blank.xsize
+


update the docstring here as well

papachap · 2025-02-11T08:58:06Z

src/compas_timber/elements/plate.py

@@ -62,6 +63,10 @@ def __init__(self, outline, thickness, vector=None, frame=None, **kwargs):
        self.attributes = {}
        self.attributes.update(kwargs)
        self.debug_info = []
+        self._ref_frame = None


Isn't it redundant to have both a vector and frame argument?

papachap · 2025-02-11T09:09:19Z

src/compas_timber/elements/plate.py

+        contour_feature = FreeContour.from_polyline_and_element(self.outline.points, self, interior=False)
+        self.add_feature(contour_feature)


is it necessary to add a contour feature by default? maybe consider making this optional if not always needed.

It feels there are a lot of hiddent dependencies in this code where properties are set implicitly. For example, the blank property is set when the compute_obb() method is called, which happens when ref_frame is accessed, and ref_frame is triggered when the FreeContour processing is instantiated.

All this creates an extensive chain of implicit side effects that we need to reduce by explicitly setting some of these properties.

papachap · 2025-02-11T09:15:22Z

src/compas_timber/elements/plate.py

+        for feature in self.features:
+            try:
+                plate_geo = feature.apply(plate_geo, self)
+            except FeatureApplicationError as error:
+                self.debug_info.append(error)


unless you want to completely remove the posibility of computing the geometry without the features (which in this case you should remove the include_features argument), add an additional if statement for that.

papachap · 2025-02-11T10:41:34Z

src/compas_timber/fabrication/free_contour.py

+    @property
+    def params_dict(self):
+        return FreeCountourParams(self).as_dict()


I don't see the use of this property anymore since the create_processing() doesn't access it and instead it directly creates the XML elements for the processing.

papachap · 2025-02-11T11:02:34Z

src/compas_timber/fabrication/free_contour.py

+    def create_processing(self):
+        """Creates a processing element. This method creates the subprocess elements and appends them to the processing element.
+        NOTE: moved to BTLxProcessing because some processings are significantly different and need to be overridden.
+
+        Parameters
+        ----------
+        processing : :class:`~compas_timber.fabrication.btlx.BTLxProcessing`
+            The processing object.
+
+        Returns
+        -------
+        :class:`~xml.etree.ElementTree.Element`
+            The processing element.
+
+        """
+        # create processing element
+        processing_element = ET.Element(
+            self.PROCESSING_NAME,
+            self.header_attributes,
+        )
+        contour_element = ET.SubElement(processing_element, "Contour", self.contour_attributes)
+        ET.SubElement(contour_element, "StartPoint", BTLxPart.et_point_vals(self.contour_points[0]))
+        for pt in self.contour_points[1:]:
+            point_element = ET.SubElement(contour_element, "Line")  # TODO: consider implementing arcs. maybe as tuple? (Point,Point)
+            point_element.append(ET.Element("EndPoint", BTLxPart.et_point_vals(pt)))
+        return processing_element


I understand that this is a completely different BTLx processing and requires a different approach. However, I'm not a huge fan of mixing XML generation with the core logic. This approach bypasses the parameters dictionary for this class. I wonder if we can refactor the FreeContourParams class to achieve what we need. Perhaps we could use a similar approach to the House BTLx processing, where we nest processings.

papachap · 2025-02-11T11:07:16Z

@obucklin @chenkasirer I added some comments as well. If you want we can sit have a look at it together.

obucklin added 11 commits January 29, 2025 11:23

start

e157d93

free_contour

4844984

working_but_basic

85d914f

orientation correct

410b7f2

works, gonna break it now

26dc2a0

boom working good

a72c9dd

removed brepSubtraction feature

90aed71

format lint

9775798

Merge branch 'main' into Contour_BTLx

ce73c55

added tests

3b56947

ready

67ba3bc

obucklin marked this pull request as ready for review January 31, 2025 13:52

obucklin requested review from chenkasirer and papachap January 31, 2025 13:52

doc stuff

2cfa5e5

obucklin linked an issue Jan 31, 2025 that may be closed by this pull request

BTLx contour processing for plates #383

Open

fixed inclination setter

4cbed3b

chenkasirer reviewed Feb 6, 2025

View reviewed changes

obucklin mentioned this pull request Feb 6, 2025

Wall rework #386

Closed

10 tasks

Merge branch 'main' into Contour_BTLx

c82cf7c

obucklin added 3 commits February 7, 2025 09:29

forgot to push last night

651c237

added test, removed writer._create_part

fixed unit-tests

7e27347

fixed subprocessing

28397d1

papachap reviewed Feb 10, 2025

View reviewed changes

papachap reviewed Feb 11, 2025

View reviewed changes

obucklin added 2 commits February 12, 2025 15:09

Merge branch 'main' into Contour_BTLx

69fabed

merge fix

e583c09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`FreeContour` BTLxProcessing #382

`FreeContour` BTLxProcessing #382

obucklin commented Jan 30, 2025 •

edited

Loading

chenkasirer left a comment

chenkasirer Feb 6, 2025

obucklin Feb 6, 2025

obucklin commented Feb 6, 2025

papachap Feb 10, 2025

papachap Feb 10, 2025

papachap Feb 10, 2025

papachap Feb 10, 2025

papachap Feb 11, 2025

papachap Feb 11, 2025

papachap Feb 11, 2025

papachap Feb 11, 2025 •

edited

Loading

papachap Feb 11, 2025

papachap Feb 11, 2025

papachap Feb 11, 2025

papachap commented Feb 11, 2025

		contour_feature = FreeContour.from_polyline_and_element(self.outline.points, self, interior=False)
		self.add_feature(contour_feature)

FreeContour BTLxProcessing #382

Are you sure you want to change the base?

FreeContour BTLxProcessing #382

Conversation

obucklin commented Jan 30, 2025 • edited Loading

What type of change is this?

Checklist

chenkasirer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obucklin commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap commented Feb 11, 2025

`FreeContour` BTLxProcessing #382

`FreeContour` BTLxProcessing #382

obucklin commented Jan 30, 2025 •

edited

Loading

papachap Feb 11, 2025 •

edited

Loading