-
Notifications
You must be signed in to change notification settings - Fork 32
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(engine): added resource to easily configure magic numbers in solver #1379
feat(engine): added resource to easily configure magic numbers in solver #1379
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
- Coverage 53.43% 53.40% -0.04%
==========================================
Files 451 451
Lines 25827 25844 +17
Branches 2390 2390
==========================================
+ Hits 13800 13801 +1
- Misses 12027 12043 +16 ☔ View full report in Codecov by Sentry. |
31567f9
to
76f250b
Compare
@SrGesus will only be able to review this PR in 5 days. |
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.
Thanks for working on this! 👍 Overall looks good to me, just have a couple of details that need sorting out.
engine/src/physics/solver/integration/physics_constants_integration.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp
Outdated
Show resolved
Hide resolved
14d27c5
to
9dab08d
Compare
engine/src/physics/solver/integration/physics_constants_integration.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp
Outdated
Show resolved
Hide resolved
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.
Other than what I pointed out LGTM! Ty for working on this!
If you could open a follow up issue to add a tool for configuring these values at runtime it would be cool 👀 might be a good first issue for someone interesting in playing with tools, or for one of you in the physics team if you want to be able to properly debug stuff
engine/src/physics/solver/integration/physics_constants_integration.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp
Outdated
Show resolved
Hide resolved
engine/src/physics/solver/integration/physics_constants_integration.hpp
Outdated
Show resolved
Hide resolved
Also, don't forget to update the CHANGELOG.md! |
7b33cd3
to
96f8578
Compare
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! 👍 Don't forget to update the CHANGELOG
96f8578
to
7227aef
Compare
7227aef
to
91355be
Compare
Description
Added a resource to easily configure magic numbers in solver.
Checklist