Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 15, 2025

  • Explore repository structure and understand the issue
  • Verify the issue exists - scale components in transformations are ignored
  • Write unit tests for transform() and transformed() methods for all Shape subclasses
  • Implement transform() override in Shape subclasses to handle scale component
    • Box: Apply non-uniform scaling to xsize, ysize, zsize
    • Sphere: Apply average scaling to radius
    • Cylinder: Apply radial scaling to radius, axial scaling to height
    • Cone: Apply radial scaling to radius, axial scaling to height
    • Capsule: Apply radial scaling to radius, axial scaling to height
    • Torus: Apply planar scaling to radius_axis, normal scaling to radius_pipe
  • Verify tests pass with the implementation
  • Review and finalize changes
    • All 512 geometry tests pass
    • JSON serialization tests pass
    • Documentation updated and clarified per review feedback
    • Code review completed
    • Ruff linting issues fixed
    • Rebased on top of current main head
Original prompt

This section details on the original issue you should resolve

<issue_title>More transformations of shapes</issue_title>
<issue_description>Following up on #1489, it seems like using .transform() or .transformed() in any subclass of compas.geometry.Shape will result in the scale component of the transformation to be ignored, because only the shape's frame is transformed.

There's a commented out piece of code in Box that seems to have attempted a fix to that, doing matrix decomposition of the transformation and then applying the scale to the dimensions of the box, but for some reason is commented out and it's also not implemented in any of the other shapes (as far as I checked).

As a first step, we should write unit tests that verify the transform and transformed results for each of the shapes in compas.geometry (sub-classes of Shape). If these tests fail, it means the transformation is indeed not applying the scale component, so the second step would be to add the implementation for that, and hopefully get the unit tests to pass.
</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #1493


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add unit tests for transformations of shapes in compas.geometry Add scale component handling to Shape transformations Oct 15, 2025
Copilot AI requested a review from gonzalocasas October 15, 2025 15:41
Copilot finished work on behalf of gonzalocasas October 15, 2025 15:41
@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.08%. Comparing base (b8cef0d) to head (a722e17).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
+ Coverage   62.00%   62.08%   +0.08%     
==========================================
  Files         208      208              
  Lines       22427    22478      +51     
==========================================
+ Hits        13905    13955      +50     
- Misses       8522     8523       +1     

☔ 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.

@gonzalocasas
Copy link
Member

@copilot run the ruff linter (through invoke tasks) and fix all linting issues

Copy link
Contributor Author

Copilot AI commented Oct 15, 2025

@copilot run the ruff linter (through invoke tasks) and fix all linting issues

Fixed all ruff linting issues. Removed 3 unused variables from test file (commit 0a2be4d).

Copilot finished work on behalf of gonzalocasas October 15, 2025 16:01
@gonzalocasas
Copy link
Member

@copilot rebase on top of current main head

Copy link
Contributor Author

Copilot AI commented Oct 15, 2025

@copilot rebase on top of current main head

Rebased on top of current main head (43cb6b6). Resolved merge conflict in cone.py where the new scale() method from main was integrated with the transform() method. All tests pass.

Copilot finished work on behalf of gonzalocasas October 15, 2025 16:26
@tomvanmele
Copy link
Member

if we are re-adding these implementations, we should remove the frame transformation from the base class, and raise a NotImplementedError, like with the other transformation methods...

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.

More transformations of shapes

3 participants