Jack cut feature #267

chenkasirer · 2024-07-17T13:18:09Z

Example implementation of a merged JackRafterCut and the CutFeature. Including client code in L-Miter.

I'm making an effort to make this change as gradual as possible. Meaning the two systems live side-by-side. For that purpose I created the new JackRafterCut in a new package _fabrication as otherwise there are some circular import issues.

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)

…eam.py

papachap · 2024-07-19T11:47:46Z

src/compas_timber/_fabrication/jack_cut.py

+        if point_start_x is None:
+            raise ValueError("Plane does not intersect with beam.")
+
+        start_x = distance_point_point(ref_edge.point, point_start_x)


Suggested change

start_x = distance_point_point(ref_edge.point, point_start_x)

start_x = distance_point_point(ref_side.point, point_start_x)

ref_edge is a compas Line, no? in this case shouldn't it be ref_edge.start? I thought it was a typo or smth ;p

papachap · 2024-07-19T13:39:53Z

src/compas_timber/elements/beam.py

@@ -252,7 +265,7 @@ def __str__(self):
    # ==========================================================================

    def compute_geometry(self, include_features=True):


Question: This replaces the geometry consumers right?

exactly. Beam now creates its own geometry as a result as its blank + features.

papachap · 2024-07-19T14:02:36Z

src/compas_timber/elements/beam.py

+            The index of the reference side to be returned. 0 to 5.
+
+        """
+        # TODO: maybe this should be the default representation of the ref sides?


I believe so as well. Using a parametric planar surface for the ref_side makes it more comprehensible i think. Also it could be convenient to have access to beam width/height(xsize) or beam length(ysize).

alright then I'd keep a mental note of maybe removing the frame-like ref sides. and we get rid of them in the future

I think we need frames somewhere because they are important for getting the various BTLx parameters. It's not clear from the docs, but I'm guessing that PlanarSurface is created with the Frame at the center. I agree that the different systems are a bit confusing.

this comment was clarified in another PR, you can ignore my comment

papachap · 2024-07-19T14:04:19Z

src/compas_timber/elements/beam.py

@@ -406,6 +420,7 @@ def _create_shape(frame, xsize, ysize, zsize):

    @reset_computed


Question: This resets the cached attributes of the class?

yes. some Element has some internal instance attributes which are cached after the first time they are created. For example Element._geometry is one. These need to be cleared when changes are made which might result in new geometry. This decorator takes care of that.

papachap · 2024-07-19T14:17:33Z

src/compas_timber/_fabrication/jack_cut.py

+    def orientation(self):
+        return self._orientation
+
+    @orientation.setter


Question: what is the benefit of using setters and private properties for this class?

this an object oriented thing that's strongly related to the encapsulation principle. In statically typed languages where private access is enforced you'd typically always need setters and getters. In python, since nothing is enforced by the language, it can get blurry.

There's different reasons why you'd want to use properties in python. In this particular case, I used properties and their setters for input validation. Since BTLx has a clear specification for the allowed value ranges for each parameter it seemed like a good place to validate those.

papachap · 2024-07-19T14:30:51Z

src/compas_timber/_fabrication/jack_cut.py

+    END = 1
+
+
+class JackRafterCut(BTLxProcess):


Where does the serialization of the btlx process parameters happen? shouldn't there be a class method for that? Also does this mean that header attributes are being removed?

You're right, that's missing. I introduced this in one of the last commits, might have not pushed yet.

I'm still very much debating though. I'd like to extract the XML schema specific stuff out of these classes, but they are nevertheless related. In the meantime, I created a JackRafterCutParams class whose sole responsibility is to populate the parameter values from JackRafterCut in they appropriate key-value pair according to the specification.

it's in the latest commit aec2795

papachap · 2024-07-19T14:36:25Z

src/compas_timber/connections/l_miter.py

With these changes does the l_mitter factory module gets removed?

yup. that's the intention. LMiterJoint becomes the de-facto factory as it directly assigns the processes/features to the respective beams.

papachap · 2024-07-23T08:23:17Z

In case there are two btlx processes that are correlated (i.e StepJoint, StepJointNotch) and share most of the parameters, should the process be implemented as:

two separate classes in the same module (_fabrication/step_joint) and generate the parameters twice if they both need to be applied in the case of a joint with two beams (one on main_beam and one cross_beam).

or

as one class containing all the combined parameters with a boolean for applying the step or the step_notch

(I attached the parameters for each one of the processes for the example of the StepJoint/StepJointNotch the common parameters should have the same values if a joint between two beams is created)

papachap · 2024-07-23T09:41:28Z

src/compas_timber/_fabrication/jack_cut.py

+        start_x = distance_point_point(ref_edge.point, point_start_x)
+        angle = cls._calculate_angle(ref_side, plane, orientation)
+        inclination = cls._calculate_inclination(ref_side, plane, orientation)
+        return cls(orientation, start_x, start_y, start_depth, angle, inclination, ref_side_index=ref_side_index)


should the ref_side_index be in this class constructor, since it is not a parameter of the class?

ref_side_index is an attribute of the parent class, as all processes need this parameter. notice JackRafterCut knows nothing about it, it gets it as a kwarg and passes it through to the parent in its call to super().__init__().

chenkasirer · 2024-07-24T09:48:00Z

In case there are two btlx processes that are correlated (i.e StepJoint, StepJointNotch) and share most of the parameters, should the process be implemented as:

two separate classes in the same module (_fabrication/step_joint) and generate the parameters twice if they both need to be applied in the case of a joint with two beams (one on main_beam and one cross_beam).

or

as one class containing all the combined parameters with a boolean for applying the step or the step_notch

(I attached the parameters for each one of the processes for the example of the StepJoint/StepJointNotch the common parameters should have the same values if a joint between two beams is created)

here I'd implement both separately. could be in the same module, but definitely as two separate classes. If the parameters of one of them is a subset of the other's then you could use inheritance to avoid having duplication code.

Additionally, if it makes sense, we could make a factory method which creates matching pairs of StepJoint and StepJointNotch from the necessary collection of parameters.

chenkasirer · 2024-07-24T16:11:05Z

@papachap I think I addressed all of your comments.
I think it is currently not necessary to partially replace the processes just for the L-Miter. I'd leave it like this for now and make the swap from joint factories to process/features once we have all processes properly implemented.

If it's all the same to you, I'd go ahead an merge this one. Just hit approve if it's all good, otherwise feel free to leave some more comments.

papachap

@chenkasirer Perfect, thanks! This was helpful! I'll try to do a pull request before next week for the StepJoint and StepJointNotch based on your implementation of the JackRafterCut and your feedback. We can discuss on that once I have it :)

obucklin

generally looks good. I think we just need to test the generation of the two angles with various plane angles.

obucklin · 2024-09-04T11:44:40Z

src/compas_timber/elements/beam.py

+        elif side_index in (4, 5):  # end faces
+            xsize = self.width
+            ysize = self.height
+        return PlanarSurface(xsize, ysize, frame=ref_side, name=ref_side.name)


I dont know for sure, but doesn't PlanarSurface have the frame at the center? AFAIK the ref_side frame s at the corner of the beam.

obucklin · 2024-09-04T11:49:28Z

src/compas_timber/_fabrication/jack_cut.py

+    def _calculate_inclination(ref_side, plane, orientation):
+        # vector rotation direction of the plane's normal in the horizontal direction
+        inclination_vector = Vector.cross(ref_side.yaxis, plane.normal)
+        inclination = angle_vectors_signed(ref_side.xaxis, inclination_vector, ref_side.yaxis, deg=True)


This was a problem with the equivalent drilling module because it was returning values out of range. you could consider using the angle_planes() module or angle_vectors() with the plane normals.

obucklin · 2024-09-05T12:13:56Z

src/compas_timber/elements/beam.py

+            The index of the reference side to be returned. 0 to 5.
+
+        """
+        # TODO: maybe this should be the default representation of the ref sides?


I think we need frames somewhere because they are important for getting the various BTLx parameters. It's not clear from the docs, but I'm guessing that PlanarSurface is created with the Frame at the center. I agree that the different systems are a bit confusing.

obucklin · 2024-09-09T12:09:07Z

src/compas_timber/elements/beam.py

+            The index of the reference side to be returned. 0 to 5.
+
+        """
+        # TODO: maybe this should be the default representation of the ref sides?


this comment was clarified in another PR, you can ignore my comment

chenkasirer added 10 commits July 4, 2024 22:14

WIP: added jackcut feature merge

ab65d45

Merge branch 'reference_frame' into jack_cut_feature

8555a7a

added Beam,side_as_surface. completed missing typing information in b…

24a8a88

…eam.py

WIP: first prototype JackRafterCut feature merge

1b84518

extended jack cut to work with both orientations

5c919e6

allow Frame or Plane. completed docstrings

26dcda1

moved new jack_cut to new temporary package to allow gradual transition

d55f10b

send beam to Feature.apply()

ed02f46

de-linting

ede5de3

updated changelog

3aceb89

chenkasirer requested a review from papachap July 17, 2024 13:24

chenkasirer added 3 commits July 17, 2024 15:35

moved btlx process base class to own module

09c1138

moved stuff around. implemented data interface for jack cut

b9d1212

added orientation type to 2nd level import

d01155c

papachap approved these changes Jul 19, 2024

View reviewed changes

papachap reviewed Jul 23, 2024

View reviewed changes

added parameters class

aec2795

chenkasirer requested a review from papachap July 24, 2024 16:08

papachap approved these changes Jul 24, 2024

View reviewed changes

chenkasirer requested a review from obucklin July 30, 2024 16:29

chenkasirer added 2 commits August 9, 2024 17:57

add process features that are part of the new system to BTLx output

525ce79

directly use beam attributes as they are now aligned with btlx

6efaf47

obucklin reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jack cut feature #267

Jack cut feature #267

chenkasirer commented Jul 17, 2024 •

edited

Loading

papachap Jul 19, 2024

chenkasirer Jul 24, 2024

papachap Jul 24, 2024

papachap Jul 19, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

papachap Jul 19, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

obucklin Sep 5, 2024

obucklin Sep 9, 2024

papachap Jul 19, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

papachap Jul 19, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

papachap Jul 19, 2024

chenkasirer Jul 24, 2024

chenkasirer Jul 24, 2024

papachap Jul 19, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

papachap commented Jul 23, 2024 •

edited

Loading

papachap Jul 23, 2024 •

edited

Loading

chenkasirer Jul 24, 2024

chenkasirer commented Jul 24, 2024

chenkasirer commented Jul 24, 2024

papachap left a comment •

edited

Loading

obucklin left a comment

obucklin Sep 4, 2024

obucklin Sep 4, 2024

obucklin Sep 5, 2024

obucklin Sep 9, 2024

	start_x = distance_point_point(ref_edge.point, point_start_x)
	start_x = distance_point_point(ref_side.point, point_start_x)

		@@ -252,7 +265,7 @@ def __str__(self):
		# ==========================================================================

		def compute_geometry(self, include_features=True):

		@@ -406,6 +420,7 @@ def _create_shape(frame, xsize, ysize, zsize):

		@reset_computed

Jack cut feature #267

Are you sure you want to change the base?

Jack cut feature #267

Conversation

chenkasirer commented Jul 17, 2024 • edited Loading

What type of change is this?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap Jul 19, 2024 • 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 Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap Jul 19, 2024 • 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

Choose a reason for hiding this comment

papachap Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papachap commented Jul 23, 2024 • edited Loading

papachap Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenkasirer commented Jul 24, 2024

chenkasirer commented Jul 24, 2024

papachap left a comment • edited Loading

Choose a reason for hiding this comment

obucklin left a 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

chenkasirer commented Jul 17, 2024 •

edited

Loading

papachap Jul 19, 2024 •

edited

Loading

papachap Jul 19, 2024 •

edited

Loading

papachap Jul 19, 2024 •

edited

Loading

papachap Jul 19, 2024 •

edited

Loading

papachap Jul 19, 2024 •

edited

Loading

papachap commented Jul 23, 2024 •

edited

Loading

papachap Jul 23, 2024 •

edited

Loading

papachap left a comment •

edited

Loading