-
Notifications
You must be signed in to change notification settings - Fork 115
Fix broken behavior with Sphere.scaled(), Cylinder.scaled(), and Capsule.scaled() and add scale() for Cone and Torus #1489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1489 +/- ##
==========================================
+ Coverage 62.00% 62.13% +0.13%
==========================================
Files 208 208
Lines 22427 22437 +10
==========================================
+ Hits 13905 13942 +37
+ Misses 8522 8495 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gonzalocasas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! But @copilot : please add a CHANGELOG.md entry
|
@copilot please address the review finding above |
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
gonzalocasas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
|
@tomvanmele does this look good to you as a fix? |
gonzalocasas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please move the scaled() implementation to the base class Shape so that it's not repeated 3 times
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
|
should we remove |
|
should we also add the missing implementations of |
|
Let's add |
|
@copilot please add |
Co-authored-by: gonzalocasas <933277+gonzalocasas@users.noreply.github.com>
Added |
gonzalocasas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@tomvanmele what do you think? good to go? |
|
go for it |
Fixed the broken
scaled()behavior inSphere,Cylinder, andCapsuleclasses by adding ascaled()method to theShapebase class. This eliminates code duplication and provides a consistent implementation for all shapes that support uniform scaling.Additionally, added missing
scale()implementations forConeandTorusclasses to support uniform scaling that preserves proportions.Problem
Calling
scaled()onSphere,Cylinder, orCapsuleobjects resulted in aTypeError:The base class
Geometry.scaled()method callsscale(x=x, y=y, z=z)with keyword arguments to support non-uniform scaling. However,Sphere,Cylinder, andCapsuleoverride thescale()method to only accept a singlefactorparameter for uniform scaling (since non-uniform scaling would change the fundamental nature of these primitive shapes).Solution
Added
scaled()method toShapebase class that accepts a singlefactorparameter and calls the shape'sscale(factor)method. This provides a consistent implementation for all shapes supporting uniform scaling.Added
scale()implementations forConeandTorus:Cone: Scales bothradiusandheightby the same factorTorus: Scales bothradius_axisandradius_pipeby the same factorChanges Made
scaled(factor)method toShapebase classscale(factor)method toConeclassscale(factor)method toTorusclassscaled()methods fromSphere,Cylinder, andCapsuleclassesScaling Behavior
All shape classes now support consistent scaling behavior:
scale(factor): Modifies the shape in place with uniform scalingscaled(factor): Returns a scaled copy without modifying the originalNote:
Boxretains its customscale(x, y, z)andscaled(x, y, z)methods to support non-uniform scaling.Testing
scaled()scale()andscaled()work correctly for all shapesFixes #1488
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.