Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 15, 2025

Description

This PR fixes the issue where scale components in transformations were being ignored when applied to Shape and Surface subclasses. Previously, calling .transform() or .transformed() with a transformation containing a scale component would only transform the shape's frame while leaving dimensional parameters unchanged.

Changes Made

Shape Subclasses

  • Box: Applies non-uniform scaling to xsize, ysize, zsize based on X, Y, Z scale factors
  • Sphere: Applies average of scale factors to radius to maintain spherical shape
  • Cylinder: Applies average of X,Y scale factors to radius, Z scale factor to height
  • Cone: Applies average of X,Y scale factors to radius, Z scale factor to height
  • Capsule: Applies average of X,Y scale factors to radius, Z scale factor to height
  • Torus: Applies average of X,Y scale factors to radius_axis, Z scale factor to radius_pipe

PlanarSurface

  • PlanarSurface: Applies X scale factor to xsize, Y scale factor to ysize

Base Class Changes

  • Shape.transform(): Changed from transforming the frame to raising NotImplementedError for consistency with other transformation methods like scale()
  • All shape subclasses override transform() to decompose transformations and extract scale components before applying them to dimensional parameters

Testing

  • Added comprehensive test suite in tests/compas/geometry/test_shapes_transformations.py with 29 tests covering:
    • Uniform and non-uniform scaling for all shape types and PlanarSurface
    • Both transform() and transformed() methods
    • Combined transformations (translation + rotation + scale)
    • Volume verification after transformation

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

  • 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.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Fixes #1493

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 and others added 3 commits October 15, 2025 15:34
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
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

❌ Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.35%. Comparing base (e34268a) to head (bc4bcf5).

Files with missing lines Patch % Lines
src/compas/geometry/shapes/shape.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
+ Coverage   62.26%   62.35%   +0.09%     
==========================================
  Files         208      208              
  Lines       22452    22510      +58     
==========================================
+ Hits        13979    14037      +58     
  Misses       8473     8473              

☔ 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

Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
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...

@gonzalocasas
Copy link
Member

@copilot please also do this for PlanarSurface

…e.transform()

- Changed Shape.transform() to raise NotImplementedError like other transformation methods (scale, etc.)
- Added transform() implementation to PlanarSurface that extracts scale components
- PlanarSurface.transform() applies X,Y scale factors to xsize, ysize respectively
- Added comprehensive tests for PlanarSurface transformations
- All linting checks pass

Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot please also do this for PlanarSurface

Added transform() implementation to PlanarSurface with scale component handling and comprehensive tests (commit fd3dd45).

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

Changed Shape.transform() to raise NotImplementedError for consistency with other transformation methods like scale() (commit fd3dd45).

Copilot AI changed the title Add scale component handling to Shape transformations Add scale component handling to Shape and PlanarSurface transformations Nov 19, 2025
Copilot finished work on behalf of gonzalocasas November 19, 2025 14:52
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