refactor: optimize copy and swap constructors for robot_model::Model#177
Conversation
domire8
left a comment
There was a problem hiding this comment.
Going in a very nice direction
| this->init_solver(); | ||
| } | ||
|
|
||
| bool QPSolver::init_solver() { |
There was a problem hiding this comment.
This returns a bool but it's not used right? What would a false return value mean?
There was a problem hiding this comment.
A false return values mean that the solver is not initialized (failed to initialize). This is from bool initSolver(); from OsqpEigen library and since we are wrapping around it, I decided to keep it the same as the previous interface.
Co-authored-by: Dominic Reber <71256590+domire8@users.noreply.github.com>
eeberhard
left a comment
There was a problem hiding this comment.
Looks great to me, much cleaner than before. Moving out all of those solver properties from the core class was nice even without changing the copy logic, and now copying without needing to re-initialize everything is even nicer. I suppose it depends on #178 to be fully usable in export, but happy to move forward with it!
| urdf_path_(other.urdf_path_), | ||
| robot_model_(other.robot_model_), | ||
| robot_data_(other.robot_data_), | ||
| geom_model_(other.geom_model_), |
There was a problem hiding this comment.
just to point out, the Pinocchio GeometryModel holds a vector of GeometryObjects which are stored as shared pointers to fcl::CollisionGeometry. This means that the copy refers to the same collision data in memory. In this particular case, a shallow copy is preferable because we only ever mutate the collision geometry when the robot model is constructed, and this way we don't have to duplicate the geometry in memory (which can get quite heavy). However, we must keep this in mind if we ever decide to add functions to modify collision geometry post-construction!
There was a problem hiding this comment.
I see! Thank you for the note. With that, it is more clear to me when to use deep copy/shallow copy of objects in memory.
domire8
left a comment
There was a problem hiding this comment.
I'm still not fully happy with that bool return value of the init_solver since its not used in the constructors of the QP solvers. To me, we want to improve code and here we create a function that has an unnecessary return value. Either
- we make it not return the bool
- we let the constructor raise an exception if the init failed
My vote goes for 2
…ontrol-libraries into refactor/optimize-copy
I completely agree, thanks for raising this! I implemented option two. |
Description
robot_model::Model#174This PR introduces the
robot_model::QPSolverclass, implementing thequadratic programming tasks that was previously integrated within therobot_model::Modelclass. The necessity for this refactor arose from the lack of a copy constructor in theOsqpEigen::Solverattribute ofrobot_model::Model, which impeded the usability ofModel's default copy constructors. Additionally, the original implementation burdened theModelclass with direct solver initialization and configuration, complicating its source code.The newly implemented
robot_model::QPSolverclass encapsulates all solver-related operations, having its own copy constructor and abstracting the solver's complexity away fromrobot_model::Model. This abstraction not only simplifies theModelclass's codebase but also enhances the efficiency of swap and copy operations. Unlike before, Pinocchio models are now directly copied rather than reinitialized, improving the process while preserving operational integrity.Review guidelines
Estimated Time of Review: 15 minutes
Checklist before merging: