-
Notifications
You must be signed in to change notification settings - Fork 279
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
feat: Minkowski penalties for Ellipse-Ellipse #977
Conversation
Codecov Report
@@ Coverage Diff @@
## main #977 +/- ##
==========================================
+ Coverage 64.15% 64.73% +0.57%
==========================================
Files 59 59
Lines 7412 7471 +59
Branches 1675 1678 +3
==========================================
+ Hits 4755 4836 +81
+ Misses 2556 2535 -21
+ Partials 101 100 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Deploying with Cloudflare Pages
|
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
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.
Looks amazing, thank you so much for doing this! I had a few comments.
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.
Looks good to me! Awesome work, thank you :)
Oh actually it looks like the tests are failing; I'm guessing once you fix those we should be good to merge this? |
@@ -370,7 +370,7 @@ describe("polyRoots tests", () => { | |||
expect(gradient[2]).toBeCloseTo(-1 / 3); | |||
}); | |||
|
|||
test("quintic", () => { | |||
test("quartic", () => { |
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.
Hey @jiriminarcik, I just noticed that you erroneously changed this test description in this PR. The polynomial is indeed quintic, since the fifth-degree coefficient is implicitly 1. I'm opening a PR to change this line back.
Description
Related PR: #911
This PR implements Minkowski constraints for Ellipses using root solver from #906. It also covers penalties for Ellipse-Circle combination as a special case.
Implementation strategy and design decisions
The helper functions for computations involving implicit shapes are now in a new file
packages/core/src/contrib/ImplicitShapes.ts
.Checklist
diagrams/
folderOpen questions
There is only one remaining family of shapes to be covered by Minkowski penalties: Bezier curves.