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

Rigid body system #10

Merged
merged 6 commits into from Jun 9, 2019
Merged

Rigid body system #10

merged 6 commits into from Jun 9, 2019

Conversation

panchagil
Copy link

  • Moved rigid bodies to physics/ folder
    • added RigidBodySystem that keeps list of RB.
  • Moved solvers to solvers/ folder
  • Removed (a lot of)unused code

- moved RigidBody class to namespace ccd::opt
- made RigidBody attributes private to ensure consistency of CM
- moved solvers to solver/
- moved rigid_body to physics/
- added RigidBodySystem that has a list of rigid bodies
   - handles the mapping of local to global coordinates
- Added get/set of RB velocities
- Added method to clear RBSystem
@panchagil panchagil requested a review from zfergus June 7, 2019 16:59
Copy link
Member

@zfergus zfergus left a comment

Choose a reason for hiding this comment

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

I really like these changes. I just have a couple of questions and comments.

total_edge_length += edge_length;
}
Eigen::VectorXd cm;
cm = (vertex_masses.asDiagonal() * vertices).colwise().sum()
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a function to build the mass matrix. This function would then call the mass matrix function. The function would be

double build_mass_matrix(const Eigen::MatrixXd& vertices, const Eigen::MatrixXi& edges, Eigen::SparseMatrix<double>& mass_matrix)

where the function return the total edge length.

Copy link
Author

Choose a reason for hiding this comment

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

ok. I can make the build_mass_matrix. Is the total length the same as mass_matrix.sum()? If that is the case, I'll rather make the return void.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, is the same as mass_matrix.sum(), so returning nothing makes more sense.

Copy link
Author

Choose a reason for hiding this comment

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

done.

= hvertices * Tm.transpose();

Eigen::MatrixXd displacements;
displacements = tvertices.rowwise().hnormalized();
Copy link
Member

Choose a reason for hiding this comment

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

I will convert this to not use homogeneous coordinates after we accept this pull request. It makes taking the gradient and hessian.


Eigen::MatrixX2d world_vertices() const
{
return vertices.rowwise() + position.transpose();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this definition not in the cpp file?

Copy link
Author

Choose a reason for hiding this comment

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

I've seen short functions defined on the .hpp file, idk why. I can move it to the cpp.

Copy link
Member

Choose a reason for hiding this comment

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

No, need I was just curious.

Copy link
Author

Choose a reason for hiding this comment

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

done.

void assemble_displacements();
void assemble_displacements(
const Eigen::VectorXd& v, Eigen::MatrixXd& u);
void add_rigid_body(RigidBody rb) { rigid_bodies.push_back(rb); }
Copy link
Member

Choose a reason for hiding this comment

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

Should this function call assemble or do something to update the fields?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it would be expensive if we need to add many rigid bodies.

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, so when is assemble called?

Copy link
Author

Choose a reason for hiding this comment

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

after you add them all. Look at state.cpp update_fields_from_rigid_bodies.

@@ -1,6 +1,6 @@
// Functions for optimizing functions.
// Includes Newton's method with and without constraints.
#include <opt/newtons_method.hpp>
#include "newtons_method.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Why use quotes here when we mostly use <solvers/newton_method.hpp>?

Copy link
Author

Choose a reason for hiding this comment

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

I've been using quotes when is its own .hpp, mostly because that is the template of my IDE.

Copy link
Member

Choose a reason for hiding this comment

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

Okay

@zfergus zfergus self-requested a review June 9, 2019 15:39
@panchagil panchagil merged commit 5466ce6 into master Jun 9, 2019
@panchagil panchagil deleted the rigid_body_system branch June 10, 2019 00:58
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.

None yet

2 participants