Skip to content
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

Add test for scaling objects to zero without any crash #2491

Merged
merged 13 commits into from
Apr 6, 2025

Conversation

rahat2134
Copy link
Contributor

Fixes a part of #2465

This PR adds a test case that verifies the application correctly handles scaling objects to exactly zero. This edge case is important to test as it creates matrices with zero determinants, which could potentially cause numerical instability or crashes.

@rahat2134
Copy link
Contributor Author

@0HyperCube PTAL!

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Scaling to zero should probably always work. However we've previously had problems whereby after scaling to zero and then transforming, a NaN value is used. This causes the SVG to be invalid. Please could you also test for applying another scale after setting the transform to zero? Thanks.

@0HyperCube 0HyperCube marked this pull request as draft March 29, 2025 20:25
@rahat2134 rahat2134 requested a review from 0HyperCube March 30, 2025 11:54
@rahat2134
Copy link
Contributor Author

Thanks for working on this. Scaling to zero should probably always work. However we've previously had problems whereby after scaling to zero and then transforming, a NaN value is used. This causes the SVG to be invalid. Please could you also test for applying another scale after setting the transform to zero? Thanks.

@0HyperCube PTAL!

@rahat2134 rahat2134 requested a review from 0HyperCube April 5, 2025 18:28
@rahat2134
Copy link
Contributor Author

@0HyperCube 0HyperCube marked this pull request as ready for review April 5, 2025 18:43
@rahat2134
Copy link
Contributor Author

rahat2134 commented Apr 5, 2025

@0HyperCube will it be merge automatically after cli test will pass?

@Keavon

@Keavon Keavon force-pushed the master branch 3 times, most recently from aa7ff13 to e11b57a Compare April 6, 2025 11:41
@0HyperCube 0HyperCube merged commit 1001ec2 into GraphiteEditor:master Apr 6, 2025
4 checks passed
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