Skip to content

Conversation

@gswifort
Copy link
Collaborator

>>> Ge.Matrix3d.scaling(-5, Ge.Point3d.kOrigin).scale3d()
PyGe.Scale3d(5.00000000000000,5.00000000000000,5.000000000000000)  # -5.0 != 5.0

I think this PR solves this problem.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in Matrix3d.scale3d() where negative scaling factors were being returned as positive values. The issue occurred because the method used length() to extract scale from coordinate system vectors, which always returns positive values. The fix extracts the diagonal components directly (x.x, y.y, z.z) to preserve the sign of scaling factors.

Changes:

  • Modified AcGeMatrix3dGetScaling3d() to use x.x, y.y, z.z instead of x.length(), y.length(), z.length() when returning the Scale3d object

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

AcGeVector3d z;
xf.getCoordSystem(pnt, x, y, z);
return AcGeScale3d(x.length(), y.length(), z.length());
return AcGeScale3d(x.x, y.y, z.z);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2D version of this function (AcGeMatrix2dGetScaling2d at line 865) still uses x.length() and y.length(), which would have the same issue with negative scaling factors. Consider applying the same fix to the 2D version for consistency: return AcGeScale2d(x.x, y.y) instead of AcGeScale2d(x.length(), y.length()).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

AcGeVector3d z;
xf.getCoordSystem(pnt, x, y, z);
return AcGeScale3d(x.length(), y.length(), z.length());
return AcGeScale3d(x.x, y.y, z.z);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test case for negative scaling to verify this fix works correctly. For example, test that Matrix3d.scaling(-5, Point3d.kOrigin).scale3d() returns Scale3d(-5, -5, -5) and not Scale3d(5, 5, 5).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

@gswifort gswifort requested a review from Copilot January 26, 2026 11:47
@gswifort gswifort marked this pull request as draft January 26, 2026 11:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

AcGeVector3d z;
xf.getCoordSystem(pnt, x, y, z);
return AcGeScale3d(x.length(), y.length(), z.length());
return AcGeScale3d(x.x, y.y, z.z);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes negative scaling for axis-aligned matrices but breaks behavior for non-axis-aligned transformations.

For axis-aligned scaling (e.g., Matrix3d.scaling(-5, ...)):

  • Basis vectors: x=(-5,0,0), y=(0,-5,0), z=(0,0,-5)
  • Old: (5, 5, 5) - incorrect, loses sign ✗
  • New: (-5, -5, -5) - correct ✓

For rotated transformations (e.g., 45° rotation, no scale):

  • Basis vectors: x≈(0.707,0.707,0), y≈(-0.707,0.707,0), z=(0,0,1)
  • Old: (1, 1, 1) - correct magnitudes ✓
  • New: (0.707, 0.707, 1) - incorrect, not meaningful scale values ✗

Consider one of these approaches:

  1. If scale3d() is only intended for axis-aligned matrices, add documentation stating this limitation
  2. Use a proper decomposition: sign(determinant) * length() for uniform scale, or sign(x.x) * x.length() for each axis where sign() uses the component with largest absolute value
  3. Check if vectors are axis-aligned (only one non-zero component) and use component if true, else use length()

Copilot uses AI. Check for mistakes.
@CEXT-Dan
Copy link
Owner

hmm, yeah, length is clearly wrong.

though what if I rotate the matrix?

consider

        xf = Ge.Matrix3d.kIdentity
        xf.setToRotation(math.radians(45),Ge.Vector3d.kZAxis,Ge.Point3d.kOrigin)
        print(xf.xAxis().x, xf.yAxis().y,xf.zAxis().z)
        print(xf.scale3d())

@CEXT-Dan
Copy link
Owner

this seems to work

def extract_scale(xf: Ge.Matrix3d):
    vecs = [xf.xAxis(), xf.yAxis(), xf.zAxis()]
    orig = [Ge.Vector3d.kXAxis, Ge.Vector3d.kYAxis, Ge.Vector3d.kZAxis]
    scales = []
    for i in range(3):
        length = vecs[i].length()
        if vecs[i].dotProduct(orig[i]) < 0:
            scales.append(-length)
        else:
            scales.append(length)
    return tuple(scales)

def test():
    xf = Ge.Matrix3d.kIdentity
    pl = Ge.Plane(Ge.Point3d.kOrigin, Ge.Vector3d.kXAxis)
    xf.setToMirroring(pl)
    print(extract_scale(xf)) #(1.0, 1.0, 1.0) wrong
 
    xf = Ge.Matrix3d.kIdentity
    pl = Ge.Plane(Ge.Point3d.kOrigin, Ge.Vector3d.kYAxis)
    xf.setToMirroring(pl)
    print(extract_scale(xf)) #(1.0, 1.0, 1.0) wrong
    
    xf = Ge.Matrix3d.kIdentity
    pl = Ge.Plane(Ge.Point3d.kOrigin, Ge.Vector3d.kZAxis)
    xf.setToMirroring(pl)
    print(extract_scale(xf)) #(1.0, 1.0, 1.0) wrong
    
    xf = Ge.Matrix3d.kIdentity
    xf.setToRotation(math.radians(45), Ge.Vector3d.kZAxis, Ge.Point3d.kOrigin)
    print(extract_scale(xf)) #(1.0, 1.0, 1.0) correct

@CEXT-Dan CEXT-Dan marked this pull request as ready for review January 26, 2026 12:58
@CEXT-Dan CEXT-Dan merged commit dba15a3 into CEXT-Dan:main Jan 26, 2026
15 checks passed
@CEXT-Dan CEXT-Dan mentioned this pull request Jan 26, 2026
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.

2 participants