Use size_t for index variables

[tylerjw]

Description

This relates to #944

I wasn't able to find anywhere in the code where these values are checked if they are negative before they are used. This was causing an issue with the work I'm doing on bio_ik.

This is an ABI-breaking change but it shouldn't break anyone's code that depends on these values. At best it'll throw some compiler warnings if they were storing the resulting values in signed variables.

Checklist

• Required by CI: Code is auto formatted using clang-format

[v4hn]

I disagree. The way joints are constructed, the RobotModel has to specify the joint index from the outside and the value should stay invalid until it is set from there.
So if you insist we can discuss ssize_t, std::optional or exceptions from getJointIndex as alternatives but I'm not convinced refactoring this is any more helpful than annoying. How often do you expect to see robots with 2^31 joints?

[tylerjw]

About as often as I expect to see a robots with joint indexes of -1.

It seems to be that this is a really odd interface. There are a ton of values in these objects that are always used and assumed to be valid.

It is a really basic thing to expect objects to only contain valid values after they are constructed.

I know that this is a bandaid on a tiny part of the API here. I'll look into refactoring these objects so they can only be constructed in a valid state.

[v4hn]

About as often as I expect to see a robots with joint indexes of -1.

You never do unless you do not bind the joints to a robot model before use.

It seems to be that this is a really odd interface. There are a ton of values in these objects that are always used and assumed to be valid.
It is a really basic thing to expect objects to only contain valid values after they are constructed.

More or less agreed. At the same time I thought PickNik is interested in reconfiguring robot models (and thus possibly modifying the joint indices at runtime).

I forgot to mention the alternative to assert uses of the index to check whether it was initialized.

You can restructure all of this of course, but it is at the core of everything. Let's see what you come up with! 🐈

[tylerjw]

It seems to be that this is a really odd interface. There are a ton of values in these objects that are always used and assumed to be valid.
It is a really basic thing to expect objects to only contain valid values after they are constructed.

More or less agreed. At the same time I thought PickNik is interested in reconfiguring robot models (and thus possibly modifying the joint indices at runtime).

We are but I don't think that means we need index values to be mutable and invalid ever. We know what they are when we construct them.

I forgot to mention the alternative to assert uses of the index to check whether it was initialized.

You can restructure all of this of course, but it is at the core of everything. Let's see what you come up with! cat2

Do you mind reviewing my latest changes here? I refrained from refactoring everything in these yet, but this PR makes so the index values are never invalid so we can set them to the type that operator[] takes on construction.

[tylerjw]

So if you insist we can discuss ssize_t, std::optional or exceptions from getJointIndex as alternatives but I'm not convinced refactoring this is any more helpful than annoying.

I would like to save std::optional (or some other monad) for cases where the code should be able to fail. This is one case where we have all the information to set the variable to a valid value on construction. Using optional here is silly.

[codecov]

Codecov Report

Merging #946 (1bf9a5e) into main (1faa2b1) will decrease coverage by 0.23%.
The diff coverage is 89.05%.

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
- Coverage   57.95%   57.72%   -0.22%     
==========================================
  Files         310      310              
  Lines       26132    26129       -3     
==========================================
- Hits        15141    15080      -61     
- Misses      10991    11049      +58     

Impacted Files	Coverage Δ
.../include/moveit/robot_model/floating_joint_model.h	0.00% <ø> (ø)
...el/include/moveit/robot_model/planar_joint_model.h	33.34% <ø> (ø)
...include/moveit/robot_model/prismatic_joint_model.h	60.00% <ø> (ø)
.../include/moveit/robot_model/revolute_joint_model.h	100.00% <ø> (ø)
...bot_model/include/moveit/robot_model/robot_model.h	93.45% <ø> (ø)
moveit_core/robot_model/src/robot_model.cpp	74.79% <84.00%> (-0.03%)	⬇️
...bot_model/include/moveit/robot_model/joint_model.h	80.89% <100.00%> (-1.06%)	⬇️
...obot_model/include/moveit/robot_model/link_model.h	87.50% <100.00%> (-0.50%)	⬇️
moveit_core/robot_model/src/fixed_joint_model.cpp	33.34% <100.00%> (+2.57%)	⬆️
...veit_core/robot_model/src/floating_joint_model.cpp	48.97% <100.00%> (+0.27%)	⬆️
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1faa2b1...1bf9a5e. Read the comment docs.

outdated. I can't review again at this moment though.

[henningkayser]

Overall, the switch to size_t makes a lot of sense to me. As we change the API, we should probably consider more conceptual changes as well as suggested in the comment.

[mergify]

This pull request is in conflict. Could you fix it @tylerjw?

[henningkayser]

btw, I very much prefer having explicit indexes enforced with the constructor over allowing constructing stand-alone joints in an invalid state.

[henningkayser]

I'm fine with these changes, but let's merge #970 first so that this doesn't end up in the docker images for Galactic