Joint candidates #499
Joint candidates #499
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a joint candidate system that separates the identification of potential beam joints from actual joint creation. Joint candidates are stored separately from actual joints, allowing for a two-stage workflow where candidates are first identified and then selectively promoted to actual joints based on analysis.
- Added
joint_candidatesproperty and management methods toTimberModelclass - Renamed
GenericJointtoJointCandidateto better reflect its purpose - Modified
connect_adjacent_beams()to create candidates instead of actual joints
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compas_timber/model/model.py | Added joint candidates property, add/remove methods, and modified beam connection logic |
| src/compas_timber/connections/generic_joint.py | Renamed GenericJoint class to JointCandidate |
| src/compas_timber/connections/analyzers.py | Updated to use joint_candidates instead of filtering joints |
| tests/compas_timber/test_model.py | Added comprehensive tests for joint candidate functionality |
| tests/compas_timber/test_joint.py | Updated tests to reflect new candidate behavior |
| examples/model/0008_joint_candidates_demo.py | Added demonstration of joint candidate workflow |
Comments suppressed due to low confidence (1)
tests/compas_timber/test_joint.py:279
- This test verifies that JointCandidate.create() creates actual joints, but there's no test verifying that the created joint has the correct topology and location parameters that were passed to create().
joint = JointCandidate.create(model, beams[0], beams[1], topology=JointTopology.TOPO_T, location=Point(0.5, 0, 0))
src/compas_timber/model/model.py
Outdated
| edge = (element_a.graph_node, element_b.graph_node) | ||
| if edge not in self._graph.edges(): | ||
| self._graph.add_edge(*edge) | ||
| self._graph.edge_attribute(edge, "interactions", []) # initialize new edge, otherwise calls to model.joints breaks |
There was a problem hiding this comment.
The comment refers to a potential issue but doesn't explain why this initialization is necessary. Consider expanding the comment to explain what specific scenario would cause model.joints to break without this initialization.
| self._graph.edge_attribute(edge, "interactions", []) # initialize new edge, otherwise calls to model.joints breaks | |
| self._graph.edge_attribute(edge, "interactions", []) | |
| # Initialize the "interactions" attribute for the new edge. | |
| # This ensures that subsequent calls to `model.joints` do not break. | |
| # Without this initialization, accessing or modifying `model.joints` could raise | |
| # a KeyError because the "interactions" attribute would not exist for the edge. | |
| # This step is critical for maintaining the integrity of the model's graph structure. |
Copilot uses AI. Check for mistakes.
| # Create candidates | ||
| model.connect_adjacent_beams() | ||
|
|
||
| # Should have 4 candidates (one for each edge of the square) |
There was a problem hiding this comment.
The comment suggests 4 candidates for a square pattern, but this depends on the specific beam intersection logic. The comment should clarify what type of intersections are expected (corner joints, overlapping beams, etc.).
| # Should have 4 candidates (one for each edge of the square) | |
| # Should have 4 candidates, one for each corner joint where beams meet at the vertices of the square. | |
| # This behavior is determined by the `connect_adjacent_beams()` method, which identifies corner intersections. |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
+ Coverage 64.05% 64.12% +0.06%
==========================================
Files 76 76
Lines 10887 10913 +26
==========================================
+ Hits 6974 6998 +24
- Misses 3913 3915 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@obucklin ready for review! |
There was a problem hiding this comment.
I was like, "where'd the JointCandidate class go... I guess you just merged that one. Do you want to change the name of the generic_joint.py module as well?
Do we want the JointCandidate to be persistent? in that case we need to support multiple edges per element pair. Not sure if this is currently possible, but see #498 to see what I mean. Is that what the "interaction" and "candidate" edge attributes are for?
| candidate : :class:`~compas_timber.connections.JointCandidate` | ||
| The joint candidate to remove. | ||
| """ | ||
| for interaction in candidate.interactions: |
There was a problem hiding this comment.
please take a quick look at issue #498. I don't know what happens under the hood of the Graph, but it looks like we need to support multiple edges for a pair of elements.
| for interaction in candidate.interactions: | ||
| element_a, element_b = interaction | ||
| edge = (element_a.graph_node, element_b.graph_node) | ||
| if edge in self._graph.edges(): |
There was a problem hiding this comment.
I think we need to do the same thing for TimberModel.remove_joint(). As I understand it, that method will just remove any edge between the two elements. It should probably filter to just remove interactions.
| # HACK: calls to `model.joints` expect there to be a "interactions" on any edges | ||
| self._graph.edge_attribute(edge, "interactions", []) | ||
|
|
||
| # this is how joints and candidates co-exist on the same edge, they are stored under different attributes |
There was a problem hiding this comment.
Thanks for the clarification
Adding joint candidates in their own attribute so they can co-exist with joints (
interactions).Left the promotion logic out of it, done manually (see example script)
joint_candidatesproperty toTimberModel.add_joint_candidatemethod toTimberModel.remove_joint_candidatemethod toTimberModel.NBeamKDTreeAnalyzernow usesmodel.joint_candidatesinstead of filteringmodel.joints.What type of change is this?
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.