Lap bugfix by papachap · Pull Request #431 · gramaziokohler/compas_timber

papachap · 2025-05-06T16:15:12Z

⚠️ This PR depends on compas-dev/compas#1462 being merged first.

What type of change is this?

This PR fixes a bug in the Lap BTLxProcessing occurring when the beams that should be joint with a lap, are perpendicular to each other.

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)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

chenkasirer

lgtm!

chenkasirer · 2025-05-14T07:48:02Z

            raise BeamJoiningError(self.elements, self, debug_info=str(ex))
-        self.main_beam.add_blank_extension(start_main, end_main, self.main_beam_guid)
-        self.cross_beam.add_blank_extension(start_cross, end_cross, self.cross_beam_guid)
+        tol = TOL.absolute


sure that's enough? TOL.absolute is like 1e-9 iirc..

obucklin

looks fine except the double definition of angle_vectors_projected

papachap · 2025-06-02T08:21:16Z

@obucklin should we merge this already? or wait and replace it with the compas.geometry function once your PR is reviewed?

obucklin · 2025-06-02T09:24:37Z

@obucklin should we merge this already? or wait and replace it with the compas.geometry function once your PR is reviewed?

maybe use the existing compas.geometry function and do the check for vectors parallel to the normal outside of that? Can we link this somehow so when the one is merged we are reminded to update this??

papachap · 2025-06-02T12:53:48Z

@obucklin AFAIK there is no built-in function in github for hard-linking cross repo PRs.

I replaced the function with the compas.geometry one in all occasions, and I completely removed the one in our utils toolkit.
Also left a note on the description of this PR for the dependency of this PR.

Once your PR in compas is approved and merged, come back and approve this one too.

chenkasirer · 2025-07-03T11:52:06Z

I propose, as also suggested by @obucklin above, that we do the check ourselves (is_parallel, before calling angle_vectors_projected) and not wait for the PR to be merged. Core should probably change but not sure when and exactly how so let's not hold our breath.

…into lap-bugfix

codecov · 2025-07-08T11:17:39Z

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@6a43bfb). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/compas_timber/connections/l_lap.py	25.00%	3 Missing ⚠️
src/compas_timber/fabrication/lap.py	80.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #431   +/-   ##
=======================================
  Coverage        ?   63.64%           
=======================================
  Files           ?       76           
  Lines           ?    10861           
  Branches        ?        0           
=======================================
  Hits            ?     6913           
  Misses          ?     3948           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

papachap · 2025-07-08T11:19:11Z

As suggested by @chenkasirer I temporarily added a check before calling the angles_vector_projected() function from core. This is ready for review. @obucklin needs to accept the changes.

Copilot

Pull Request Overview

This PR fixes several issues in the Lap joint and related processing when beams are perpendicular, improves how ref_side_index is detected, and consolidates the use of angle_vectors_projected from compas.geometry.

Removed the deprecated internal angle_vectors_projected and now import the one from compas.geometry, adding deg=True to its calls.
Corrected the ref_side_index check to handle 0, added a perpendicular‐vector fallback for inclination in Lap, and updated slope/inclination calculations.
Simplified start_frame logic and applied global tolerance offsets to blank extensions in the L-lap connection.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/compas_timber/utils/init.py	Removed custom `angle_vectors_projected` and its import of `Projection`.
src/compas_timber/fabrication/pocket.py	Updated calls to `angle_vectors_projected` to include `deg=True`.
src/compas_timber/fabrication/lap.py	Fixed `ref_side_index` false‐zero bug, added perpendicular‐case fallback for inclination, removed conditional start‐frame branch, and updated angle calls.
src/compas_timber/connections/l_lap.py	Added tolerance offsets (`TOL.absolute`) to `add_blank_extension` calls.
CHANGELOG.md	Documented bug fixes for `Lap` behavior and `ref_side_index`.

Comments suppressed due to low confidence (3)

src/compas_timber/fabrication/lap.py:444

Consider adding tests to cover the new perpendicular vector branch to ensure correct behavior when yyaxis is perpendicular to zzaxis or front_plane.normal.

        if TOL.is_zero(abs(yyaxis.dot(zzaxis))) or TOL.is_zero(abs(yyaxis.dot(front_plane.normal))):  # TODO: follow changes in compas.geometry and update this acordingly

src/compas_timber/fabrication/lap.py:742

The conditional handling for FaceLimitedStart was removed, causing start_frame to always be computed via _start_frame_from_params_and_beam; verify this change doesn't alter intended downstream behavior or restore the original branch if needed.

        tol = Tolerance()

CHANGELOG.md:104

This changelog entry mentions adjusting angle_vectors_projected to return None, but the code doesn't include such a change; update the entry to reflect actual modifications.

* Adjusted `angle_vectors_projected` function so that it returns `None` if any of the projections results in a zero-vector.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…_timber into lap-bugfix

Co-authored-by: Chen Kasirer <ckasirer@ethz.ch>

chenkasirer · 2025-07-08T11:35:46Z

    return distance_point_point(pt_seg_a, pt_seg_b)


-def angle_vectors_projected(vector_a, vector_b, normal):


add to changelog in the Removed section

EDIT: you already did.. sorry!

I already did. It should appear to you

Co-authored-by: Chen Kasirer <ckasirer@ethz.ch>

chenkasirer

commented on a few minor issues, but other than that lgtm!

obucklin

LGTM

papachap added 4 commits May 6, 2025 17:56

add tol in extensions

2287780

return none if projection fails

c51c008

fix minor bugs

e674f87

changelog

e204627

papachap requested a review from Copilot May 13, 2025 23:01

papachap marked this pull request as ready for review May 13, 2025 23:01

papachap linked an issue May 13, 2025 that may be closed by this pull request

Lap Processing bug #427

Closed

This comment was marked as outdated.

Sign in to view

Update src/compas_timber/fabrication/lap.py

ba92f40

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

papachap requested review from chenkasirer and obucklin May 13, 2025 23:03

chenkasirer approved these changes May 14, 2025

View reviewed changes

obucklin requested changes May 15, 2025

View reviewed changes

Comment thread src/compas_timber/fabrication/lap.py Outdated

obucklin requested changes May 15, 2025

View reviewed changes

Comment thread src/compas_timber/fabrication/lap.py Outdated

Merge https://github.com/gramaziokohler/compas_timber into lap-bugfix

30e810a

papachap requested a review from obucklin June 2, 2025 08:19

papachap added 2 commits June 2, 2025 14:36

replace angle_vectors_projected with core function

049b475

remove function from utils module

68fcb2e

papachap added 2 commits July 8, 2025 10:55

Merge branch 'main' of https://github.com/gramaziokohler/compas_timber …

9090b4e

…into lap-bugfix

temporary fix for angle_vectors_projected()

5335dcc

papachap requested review from chenkasirer and Copilot July 8, 2025 11:19

Copilot AI reviewed Jul 8, 2025

View reviewed changes

Comment thread src/compas_timber/fabrication/lap.py Outdated

Update src/compas_timber/fabrication/lap.py

03f0e67

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

papachap added 2 commits July 8, 2025 13:26

changelog

9dd732b

Merge branch 'lap-bugfix' of https://github.com/gramaziokohler/compas…

21c5002

…_timber into lap-bugfix

chenkasirer reviewed Jul 8, 2025

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

1b62317

Co-authored-by: Chen Kasirer <ckasirer@ethz.ch>

chenkasirer reviewed Jul 8, 2025

View reviewed changes

Comment thread src/compas_timber/fabrication/lap.py Outdated

chenkasirer reviewed Jul 8, 2025

View reviewed changes

Update src/compas_timber/fabrication/lap.py

ac25c2d

Co-authored-by: Chen Kasirer <ckasirer@ethz.ch>

chenkasirer approved these changes Jul 8, 2025

View reviewed changes

obucklin approved these changes Jul 14, 2025

View reviewed changes

papachap merged commit 6e49734 into main Jul 16, 2025
17 checks passed

		return distance_point_point(pt_seg_a, pt_seg_b)


		def angle_vectors_projected(vector_a, vector_b, normal):

Conversation

papachap commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of change is this?

Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

chenkasirer left a comment

Choose a reason for hiding this comment

Uh oh!

chenkasirer May 14, 2025

Choose a reason for hiding this comment

Uh oh!

obucklin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

papachap commented Jun 2, 2025

Uh oh!

obucklin commented Jun 2, 2025

Uh oh!

papachap commented Jun 2, 2025

Uh oh!

chenkasirer commented Jul 3, 2025

Uh oh!

codecov Bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

papachap commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chenkasirer Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papachap Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

chenkasirer left a comment

Choose a reason for hiding this comment

Uh oh!

obucklin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

papachap commented May 6, 2025 •

edited

Loading

codecov Bot commented Jul 8, 2025 •

edited

Loading

papachap commented Jul 8, 2025 •

edited

Loading

chenkasirer Jul 8, 2025 •

edited

Loading