Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various Cleanup and Commenting #1180

Merged
merged 4 commits into from
Nov 11, 2018

Conversation

davetcoleman
Copy link
Member

@davetcoleman davetcoleman commented Nov 1, 2018

Lots of minor, non-behavior cleanup and commenting as I debugged an issue with the planning scene transforms. Changes are cleanly separated by commits and should be Rebased and Merged but not cherry-picked backwards.

  • Cleanup PlanningScene
  • Cleanup RobotModel
  • Cleanup Transforms
  • Cleanup various moveit_ros packages

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I approve this. However, I don't like the renaming of Transforms::transforms_ to Transforms::transforms_map_. That's too verbose for my taste.

@davetcoleman
Copy link
Member Author

Generally I approve this. However, I don't like the renaming of Transforms::transforms_ to Transforms::transforms_map_. That's too verbose for my taste.

I'm really into verbose variables, as they improve quick understanding of the code. This is the MoveIt! style throughout the code. The ROS style guide says:

Be reasonably descriptive and try not to be cryptic. Longer variable names don't take up more space in memory, I promise.

Additionally, I found Transforms::transforms_ particularly confusing as it made me think the class contained itself as a member variable (they have the same name??)

@rhaschke
Copy link
Contributor

rhaschke commented Nov 3, 2018

Why didn't you rebased on master to resolve conflicts?

- Add additional comments
- Rename ftf_ to scene_transforms_
- Split up very large processCollisionObject function
- Add some comments
- Clarify a few variable names
- Rename transforms_ to transforms_map_
- Add comment
- Clarify console output
- Improve comments
- Cleanup function flow
- Change pr2 example to panda
@davetcoleman
Copy link
Member Author

I used Github's web-based conflict merging tool, which turned out a mess but it took significant time so I didn't want to start over.

I just started over locally and rebased properly. Now it should be Rebased and Merged as the commit history is clean

@rhaschke rhaschke merged commit 7d5ea42 into moveit:melodic-devel Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants