compas icon indicating copy to clipboard operation
compas copied to clipboard

Add scale component handling to Shape and PlanarSurface transformations

Open Copilot opened this issue 4 months ago • 8 comments

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?

  • [x] 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).
  • [x] I ran all tests on my computer and it's all green (i.e. invoke test).
  • [x] 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.
  • [x] I have added tests that prove my fix is effective or that my feature works.
  • [x] I have added necessary documentation (if appropriate)

Fixes compas-dev/compas#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 compas-dev/compas#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 compas-dev/compas#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 avatar Oct 15 '25 15:10 Copilot

Codecov Report

:x: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review. :white_check_mark: 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 :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 15 '25 15:10 codecov[bot]

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

gonzalocasas avatar Oct 15 '25 15:10 gonzalocasas

@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 avatar Oct 15 '25 16:10 Copilot

@copilot rebase on top of current main head

gonzalocasas avatar Oct 15 '25 16:10 gonzalocasas

@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 avatar Oct 15 '25 16:10 Copilot

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

tomvanmele avatar Oct 15 '25 19:10 tomvanmele

@copilot please also do this for PlanarSurface

gonzalocasas avatar Nov 19 '25 14:11 gonzalocasas

@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 avatar Nov 19 '25 14:11 Copilot