State.com_ may be inconsistant with State.configuration_

[pFernbach]

The member com_of the struct Stateis not initialized in the constructor and never updated inside the Statestruct. It rely entirely on the user of the struct to correctly initialize and update this field. I think that this design is dangerous, and the struct State should guarantee that it's fields configuration_and com_are always consistant.

I propose to make both field private with accessors and mutators that check if the CoM position is up to date and compute it otherwise before returning it's value. The issue is that Statedo not store a pointer to the device (required to compute the CoM position) so we need to add a DevicePtr_tmember to State.

Do you see a better way to do this ? Or should we just ignore this and guarantee the consistancy outside of the struct ? If so, some changes need to be made elsewhere in the code because the consistancy is not guarantee right now.

[stonneau]

I think putting device in state is a really bad idea. The consistency can be ensured when creating the state, and not when updating it indeed. Maybe the good thing to do here is to remove the attribute COM.

Can you point where you use it ?

[pFernbach]

I tried to use it in the unit test : https://github.com/humanoid-path-planner/hpp-rbprm/blob/7cb8dac87119a97de4d3c2f71c65387c05acfb40/tests/test-projection.cc#L81 and found out that after a projection the CoM position is not updated but simply copied from the initial state.

I don't know if it's used elsewhere in the code.

[pFernbach]

Maybe the good thing to do here is to remove the attribute COM.

I think it's a good idea to avoid future errors as it's not used a lot and will not require too much refactoring.

I just checked and there isn't any place where state.com_ is used and a pointer to Device is not available, so the change will be easy.

[stonneau]

let's do this then thanks!

[stonneau]

Thanks, should I close the issue ?

[pFernbach]

Yes