search

machines-in-motion / kino_dynamic_opt

Kino-dynamic optimization algorithm for multiped robots
BSD 3-Clause "New" or "Revised" License
40 stars 9 forks source link

Issues with setting values of DynamicsState from Python #33

Open Neotriple opened 4 years ago

Neotriple commented 4 years ago

Hi,

I'm attempting to modify DynamicsState values such as the Center of Mass and Linear Momentum directly in python. Python bindings do not currently exist for the functions that are used to modify these parameters (for example, in order to change value of the center of mass, I would call the DynamicsState::centerOfMass() function located here: https://github.com/machines-in-motion/kino_dynamic_opt/blob/master/momentumopt/include/momentumopt/dynopt/DynamicsState.hpp )

In order to get around this, I modified my local version of the repo as follows:

void setCenterOfMass(const Eigen::Vector3d& com) {com_ = com;}

.def("setCoM", &DynamicsState::setCenterOfMass, py::arg("com"))

I am testing the setter functions in Python, specifically in the motion_planner.py file as follows:

dyn_optimizer.dynamicsSequence().dynamics_states[0].setCoM(np.ones(3))

When I run this, although it gets to the c++ function setCenterOfMass() above, and it looks to actually set the com_ variable that belongs to DynamicsState, if I then print out the value of the dynamicsState in the next python line as follows:

print dyn_optimizer.dynamicsSequence().dynamics_states[0] this does not reflect the change I've made. (it will still report to 0)

For reference, the setCenterOfMass() function works fine in C++ (though I realize it would be redundant, since there is already a function for this in C++)

MaximilienNaveau commented 3 years ago

Hi @Neotriple ,

You should create a pull request for this kind of issue. Please fork this repository and provide a PR. We can then discuss on how to improve the code from there.

Thanks,

Neotriple commented 3 years ago

Hi @MaximilienNaveau ,

I've created the PR here: https://github.com/machines-in-motion/kino_dynamic_opt/pull/40

I don't think it should be merged since it was in my fork which had a few other edits, but I think it's good enough for a PR. If you feel otherwise (i.e. I should clear out some of the comments/extra code I added to the solver side of the packages, I can create a new PR as you see fit).

MaximilienNaveau commented 3 years ago

I did a review on the PR, as you said it's not mergeable right now but with a little bit of cleaning it viable.