-
Notifications
You must be signed in to change notification settings - Fork 113
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 <[email protected]>
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:
|
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 <[email protected]>
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? |
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 <[email protected]>
should we remove |
should we also add the missing implementations of |
Let's add |
@copilot please add |
Co-authored-by: gonzalocasas <[email protected]>
Added |
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
, andCapsule
classes by adding ascaled()
method to theShape
base class. This eliminates code duplication and provides a consistent implementation for all shapes that support uniform scaling.Additionally, added missing
scale()
implementations forCone
andTorus
classes to support uniform scaling that preserves proportions.Problem
Calling
scaled()
onSphere
,Cylinder
, orCapsule
objects 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
, andCapsule
override thescale()
method to only accept a singlefactor
parameter for uniform scaling (since non-uniform scaling would change the fundamental nature of these primitive shapes).Solution
Added
scaled()
method toShape
base class that accepts a singlefactor
parameter and calls the shape'sscale(factor)
method. This provides a consistent implementation for all shapes supporting uniform scaling.Added
scale()
implementations forCone
andTorus
:Cone
: Scales bothradius
andheight
by the same factorTorus
: Scales bothradius_axis
andradius_pipe
by the same factorChanges Made
scaled(factor)
method toShape
base classscale(factor)
method toCone
classscale(factor)
method toTorus
classscaled()
methods fromSphere
,Cylinder
, andCapsule
classesScaling 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:
Box
retains 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.