-
Notifications
You must be signed in to change notification settings - Fork 45
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 frictionless LCP #61
base: master
Are you sure you want to change the base?
Conversation
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.
I think we should think about the runLcpConstraintEngine()
naming. What's the distinction with runConstraintEngine()
? Seems that runLcpConstraintEngine()
is one possible constraint engine you can use?
@@ -296,12 +296,20 @@ void World(py::module& m) | |||
"runConstraintEngine", | |||
+[](dart::simulation::World* self, bool _resetCommand) -> void { | |||
return self->runConstraintEngine(_resetCommand); | |||
}) | |||
}, | |||
::py::arg("resetCommand")) |
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.
Do we want default args on this? Generally we have resetCommand = false
as default everywhere.
.def( | ||
"runLcpConstraintEngine", | ||
+[](dart::simulation::World* self, bool _resetCommand) -> void { | ||
return self->runLcpConstraintEngine(_resetCommand); | ||
}) | ||
}, | ||
::py::arg("resetCommand")) |
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.
Here's another place where we could have default args, if you want them
+[](dart::simulation::World* self, bool _resetCommand) -> void { | ||
return self->runFrictionlessLcpConstraintEngine(_resetCommand); | ||
}, | ||
::py::arg("resetCommand")) |
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.
And the last place where we could put them
@@ -296,12 +296,20 @@ void World(py::module& m) | |||
"runConstraintEngine", |
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.
What's the difference between runConstraintEngine()
and runLcpConstraintEngine()
? It's not immediately obvious from reading it, so maybe we should make the names a bit more explicit?
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.
Happy Holidays! Seems good, I think just minor changes to the tests and we should be good to go.
{ | ||
mConstraintEngineFn(_resetCommand); | ||
mRegisteredConstraintEngine(_resetCommand); | ||
} | ||
|
||
//============================================================================== | ||
void World::runLcpConstraintEngine(bool _resetCommand) |
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.
Does it actually make sense to have _resetCommand
here? Doesn't the LCP create the impulses in the first place? Why would we ever not want to reset after that?
|
||
// Integrate velocities from solved impulses. Should have non-negative normal | ||
// velocity after integration. | ||
EXPECT_TRUE(box->getRelativeSpatialVelocity()[4] >= 0); |
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.
I think we need one more check here to check that having no friction actually made a difference in this case. Maybe we apply a small horizontal velocity before we run the constraints engine, and check that it's still there afterwards?
It would make sense to apply the same small horizontal velocity to the test for the LCP with friction as well, and check that the friction ends up zeroing out the velocity.
Add python bindings for `Geometry/dAdInvT`
Add frictionless LCP.