Refactor model generation for readability

[chapulina]

While working on #48, I went over the model generation code and refactored it a bit to make it easier for a new user to understand what it's doing. Some of the changes:

• Removed for loops over sdf and model tags, because there's only one of each
• Consistently used the _tag suffix for variables that hold XML tags
• Renamed main_body to base_link to be consistent with SDF
• Handle only one float value for X moments, instead of an array that always has 5 zeroes
• Added more assertions and comments throughout to make flow more explicit
• Refer to links by name instead of indirectly getting to them based on whether or not they have a property
• Don't pass the total_mass global variable as an argument

@arjo129 , these are just suggestions based on my understanding. Let me know if you disagree with anything.

[chapulina]

CI is broken because of Garden debs, there are fixes in the works

[arjo129]

Thanks for these suggestions.

• Stylistically, I agree with the changes proposed.
• I don't quite agree with special cases for link names. Personally feel link names should be independent of the script itself, this is to allow for future additions to be made. Lets say we want to add more mass shifters etc (not relevant for this project but may be relevant for others), or for instance there is some very heavy sensor payload we want to add - having link names hard coded may make the script more inflexible. If this dependency were removed I'd be happy to accept the changes.

Ideally, the next version of this script should be more generic than the current version. In an ideal world I'd like to give this a bit of thought before rushing in to change things.

[chapulina]

I don't quite agree with special cases for link names. Personally feel link names should be independent of the script itself, this is to allow for future additions to be made.

Gotcha, now I know what you were going for and I agree that it would be nice to make the script easily extensible. I'm not sure what the correct heuristics would be though. Looking at each case:

base_link

Instead of if link_name == "base_link":, the original script is looking for a link that has an inertia element with <mass>@calculate</mass> to assign main_body_com_tag and main_body_com_pose. I'd argue that those variables are tightly linked to the base link, and not to all links that have a mass to be calculated, so here maybe it makes sense to keep the name?

battery and drop_weight

The logic for these 2 is hacky on both the original script and on this PR. Instead of going through proper transform calculation, we're relying on the fact that the battery's only offset is <inertial><pose>, and the drop weight's only offset is <link><pose>. That's very fragile. To be general, we'd have to calculate the linkPose * inertialPose for every link, because it's possible that a link has both of them.

So if I don't use the name for now, what do you suggest? Not sure if it's worth it doing the proper transform math at this point. Maybe this is a bit better?

if has_link_pose and not has_inertial_pose
  use link_pose
else if has_inertial_pose and not has_link_pose
  use_inertial_pose
else
  fail #for now

[arjo129]

Yep both of these proposals sound awesome. Thanks! I think keeping base_link unique is OK for now. I agree about the inconsistency of poses. I think your proposed approach is the way forward.

[chapulina]

Yep both of these proposals sound awesome

Great! Done in d013573

[arjo129]

Have a minor knit but otherwise this LTGM. Thanks for refactoring the script.