jrl-umi3218 / RBDyn

RBDyn provides a set of classes and functions to model the dynamics of rigid body systems.
BSD 2-Clause "Simplified" License
168 stars 48 forks source link

Switch from numerical to textual ID #12

Closed haudren closed 8 years ago

haudren commented 8 years ago

This PR is a follow up to #10 : it is not yet complete, I put it out there for people to be able to review the changes and suggest improvements. I will also probably rebase and squash before merging. It could also be interesting to tag and release the current version, because this will break all libraries relying directly on RBDyn.

In summary, I removed the int id property, and equivalently replaced it by std::string name. The main thing to watch out when adapting code tot this version is that

mbg.makeMultibody(0, true);

Is a valid construct, as 0 can be interpreted as a const char* and in turn as a std::string but this breaks horribly when run:

terminate called after throwing an instance of 'std::logic_error' what(): basic_string::_S_construct null not valid [1] 15016 abort ./test_string

Other methods are equally affected, but this one is triggered most often as the base id was almost always zero.

One way to find out those problems is to turn on the zero-as-null-pointer-constant warning from GCC but it also triggers a number of problems within Eigen (In relation with Eigen::Ref apparently). If anyone has good suggestions to avoid this problem ? I sent an e-mail on the Eigen mailing list to get some advice. @gergondet also suggested adding some private versions of the problematic methods that take an int in order to break at compilation, but this is quite inelegant.

List of changes:

gergondet commented 8 years ago

This looks good to me (as soon as Benchmarks and bindings are adapted to this change). It may have been nice (to make the transition smoother) to keep the id() function and change its signature but this is just pushing back the inevitable.

@gergondet also suggested adding some private versions of the problematic methods that take an int in order to break at compilation, but this is quite inelegant.

I'm not saying this should stay in the library indefinitely but at least between this release and the next in order to catch such cases easily. This has the advantage of making the API break everywhere it is invoked using int for id instead of string.

Anyway, I'm not sure it's worth worrying about too much as there isn't that many occurrences outside RBDyn:

haudren commented 8 years ago

Thanks for the comments! I adapted the benchmarks (I did not notice any slowdown) and the python bindings (Should pass the travis build). I'll wait to see if there are any other suggestions/questions, and then we'll move on with your plan.