X/T/L HalfLap rework & Lap Volume viz adjustment #340
Conversation
papachap
changed the title
|
There’s quite a bit of repeated code across the X, T, and L joint modules. This might be a good chance to decide what we can generalize and make accessible at a higher level to avoid duplication and keep things cleaner. @chenkasirer @obucklin |
|
@chenkasirer @obucklin I pulled the latest changes from main and merged them. Ready for review |
| from .solver import JointTopology | ||
|
|
||
|
|
||
| class LHalfLapJoint(LapJoint): | ||
| class LHalfLapJoint(Joint): |
There was a problem hiding this comment.
so are we sure we want to get rid of the LapJoint class? discussed with @obucklin as well?
There was a problem hiding this comment.
Yeah, I think we can drop the current LapJoint class since it’s no longer relevant or needed. To keep things cleaner and avoid redundancy, we should combine the XHalfLapJoint, LHalfLapJoint, and THalfLapJoint into one class—maybe call it HalfLapJoint or just LapJoint (same thing that we are doing for the TenonMortiseJoint). Since SUPPORTED_TOPOLOGY is already a list, with some adjustments we can handle all the topologies (X, L, T) in the same class and avoid repeating the same logic in three places.
@obucklin @chenkasirer what do you guys think?
| @property | ||
| def beam_a_ref_side_index(self): | ||
| cross_vector = self.beam_a.centerline.direction.cross(self.beam_b.centerline.direction) | ||
| ref_side_dict = beam_ref_side_incidence_with_vector(self.beam_a, cross_vector, ignore_ends=True) | ||
| if self.flip_lap_side: | ||
| return max(ref_side_dict, key=ref_side_dict.get) | ||
| return min(ref_side_dict, key=ref_side_dict.get) | ||
|
|
||
| @property | ||
| def beam_b_ref_side_index(self): | ||
| cross_vector = self.beam_a.centerline.direction.cross(self.beam_b.centerline.direction) | ||
| ref_side_dict = beam_ref_side_incidence_with_vector(self.beam_b, cross_vector, ignore_ends=True) | ||
| if self.flip_lap_side: | ||
| return min(ref_side_dict, key=ref_side_dict.get) | ||
| return max(ref_side_dict, key=ref_side_dict.get) | ||
|
|
||
| @property | ||
| def cutting_plane_a(self): | ||
| # the plane that cuts beam_a as a planar surface | ||
| ref_side_dict = beam_ref_side_incidence(self.beam_a, self.beam_b, ignore_ends=True) | ||
| ref_side_index = max(ref_side_dict, key=ref_side_dict.get) | ||
| return self.beam_b.side_as_surface(ref_side_index) | ||
|
|
||
| @property | ||
| def cutting_plane_b(self): | ||
| # the plane that cuts beam_b as a planar surface | ||
| ref_side_dict = beam_ref_side_incidence(self.beam_b, self.beam_a, ignore_ends=True) | ||
| ref_side_index = max(ref_side_dict, key=ref_side_dict.get) | ||
| return self.beam_a.side_as_surface(ref_side_index) |
There was a problem hiding this comment.
these look very similar to the ones above, are they the same.
if we don't want these in a parent class, maybe they could be moved into a new higher level function(s) in our joinery toolkit modules where they return what's needed (index of surface) instead of the dictionary.
Then they can be called once in add_features this will be more readable, will have less duplication code and will potentially be more performant as some of these properties end up being called multiple times.
There was a problem hiding this comment.
Yeah, there’s definitely some redundancy here. The first pair of properties is for getting the ref_side for the Lap Processing of each beam, and the second pair is for calculating the planes needed for the alternative constructor of the Lap class. Even though each pair uses a different function to figure out the incidence, the two properties in each pair use the same function, so there’s some code repetition.
I’m not really a fan of generalizing this into something like get_ref_side_index() or get_cutting_plane() since each joint has its own way of calculating these values. It could make things less flexible and harder to adapt for different joint types.
What I’d suggest is converting these properties into lists and iterating through the beams to calculate the planes for each one. That would clean up the code, cut down on duplication, and make it easier to read.
what do you think?
What type of change is this?
This PR concerns the rework of the
HalfLapjoints (X,T,L) so that they use theLapprocess instead. For the time being, they can allow half laps for beams that are aligned (meaning that theirz_vectorsare either perpendicular or parallel). In the future, theLapprocess should support non-aligned configurations. However, this requires first defining a clear logic for extracting theBTLx parametersof theLapprocess from the given geometry.In this PR, the generation of the lap volume was adjusted to fully utilize all relevant
BTLx parameters, ensuring more accurate visualization of theLapprocess.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_timber.datastructures.Beam.