Skip to content

compas_model update - #454

Merged
papachap merged 109 commits into
mainfrom
compas_model_update
Oct 27, 2025
Merged

compas_model update#454
papachap merged 109 commits into
mainfrom
compas_model_update

Conversation

@papachap

@papachap papachap commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

This PR is an attempt to align with the current development of compas_model. It removes the old version pin (now set to the latest), refactors affected parts for compatibility, and adapts to recent API changes.

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)

@papachap papachap changed the title Compas model update compas_model update Jun 30, 2025
@papachap
papachap requested a review from Copilot June 30, 2025 15:52
@papachap
papachap marked this pull request as ready for review June 30, 2025 15:53

This comment was marked as outdated.

@obucklin
obucklin requested a review from paulocinco October 3, 2025 11:36
@papachap papachap mentioned this pull request Oct 8, 2025
10 tasks
@papachap

papachap commented Oct 23, 2025

Copy link
Copy Markdown
Contributor Author

@obucklin @chenkasirer The elephant is ready for review again..! Don't get scared by the 7000+ lines of code changes, at least 6000 of those are from the new json and btlx files that we store for testing.

@papachap
papachap requested review from Copilot and obucklin October 23, 2025 17:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 47 out of 49 changed files in this pull request and generated 4 comments.

Comment thread src/compas_timber/elements/timber.py
Comment thread tests/compas_timber/test_joint_rules.py
Comment thread src/compas_timber/fabrication/lap.py
Comment thread CHANGELOG.md Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

@chenkasirer chenkasirer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@papachap @obucklin I quickly went through this again, added a few small comments but I think it does what we've been discussing. this will have consequences on other PRs in the queue so I suggest we merge this one so that we can start confronting those.

LGTM!!

Comment thread src/compas_timber/elements/beam.py Outdated

@property
def blank(self):
"""The blank of the beam in model space."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is shape + extensions, right? I'd make it clear in the docstring. btw property docstrings go in the class docstrings in the Attributes section

return cls(**data)

@reset_computed
def _reset_computed_dummy(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this for testing? should it be removed?

Comment thread src/compas_timber/elements/timber.py
Comment thread src/compas_timber/elements/timber.py
The origin is at the bottom far corner of the element.
The ref_frame is always in model coordinates.
"""
return Frame(self.blank.points[1], Vector.from_start_end(self.blank.points[1], self.blank.points[2]), Vector.from_start_end(self.blank.points[1], self.blank.points[7]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should cache blank ref_frame or probably both, these will be called a lot

Comment thread src/compas_timber/model/model.py Outdated
# Override the base method to also reset computed properties of elements
super().transform(transformation)
for element in self.elements():
element.reset_computed_properties()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to future us: I guess here all computed geometries are gonna invalidate making transforming the entire model a very expensive operation. if the cached geometries are all some how in local space, then the only thing that needs to be re-applied are the new transformations, which is a much less expensive process, as you'd expect it to be.

@obucklin obucklin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE!

@papachap

Copy link
Copy Markdown
Contributor Author

NerfGunsGIF

@papachap
papachap merged commit 80a1a26 into main Oct 27, 2025
17 of 18 checks passed
@papachap
papachap deleted the compas_model_update branch October 27, 2025 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants