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

Removing more boost usage #1372

Merged
merged 54 commits into from
Jun 17, 2022
Merged

Removing more boost usage #1372

merged 54 commits into from
Jun 17, 2022

Conversation

henrygerardmoore
Copy link
Contributor

@henrygerardmoore henrygerardmoore commented Jun 16, 2022

Description

As a folllow-on to #1331, this PR was made to break up the work to avoid necessitating too much review. This PR includes
replace boost::regex
replace boost::noncopyable
replace boost::lexical_cast
replace boost::optional
replace boost::thread_group

  • Replacing boost::regex
  • Replacing boost::noncopyable
  • Replacing most usage of boost::lexical_cast
  • Replacing boost::optional
  • Replacing boost::thread_group

Some remaining boost usage is there because there is no simple replacement in std. There is also some that is still there because ROS itself uses boost and thus the moveit side must use boost instead of switching to std.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #1372 (7f1822b) into main (1c3e1ea) will increase coverage by 0.02%.
The diff coverage is 67.08%.

@@            Coverage Diff             @@
##             main    #1372      +/-   ##
==========================================
+ Coverage   61.54%   61.56%   +0.02%     
==========================================
  Files         274      274              
  Lines       24981    24977       -4     
==========================================
+ Hits        15373    15374       +1     
+ Misses       9608     9603       -5     
Impacted Files Coverage Δ
..._core/collision_detection/src/collision_matrix.cpp 37.15% <ø> (ø)
moveit_core/collision_detection/src/world.cpp 88.11% <ø> (ø)
...ene/include/moveit/planning_scene/planning_scene.h 43.55% <ø> (ø)
...del/include/moveit/robot_model/joint_model_group.h 90.48% <ø> (ø)
...veit_core/robot_model/src/floating_joint_model.cpp 43.30% <0.00%> (ø)
moveit_core/robot_model/src/joint_model_group.cpp 63.89% <0.00%> (ø)
moveit_core/robot_model/src/robot_model.cpp 74.64% <ø> (+0.04%) ⬆️
...nclude/moveit/robot_state/cartesian_interpolator.h 57.90% <ø> (ø)
moveit_core/robot_state/src/attached_body.cpp 74.67% <ø> (ø)
moveit_core/robot_state/src/conversions.cpp 63.71% <0.00%> (ø)
... and 26 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 1c3e1ea...7f1822b. Read the comment docs.

@henrygerardmoore henrygerardmoore marked this pull request as ready for review June 16, 2022 17:05
@henrygerardmoore henrygerardmoore mentioned this pull request Jun 16, 2022
7 tasks
@henrygerardmoore henrygerardmoore marked this pull request as draft June 16, 2022 17:17
@henrygerardmoore henrygerardmoore marked this pull request as ready for review June 16, 2022 18:24
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

I went through all the changes and they look really good to me. Great work!

@moveit moveit deleted a comment from mergify bot Jun 17, 2022
moveit_core/robot_state/src/robot_state.cpp Outdated Show resolved Hide resolved
@@ -34,7 +34,6 @@

/* Author: Bryce Willey */

#include <boost/algorithm/string_regex.hpp>
#include <boost/math/constants/constants.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

You could get rid of this because PI is available from math.h. I don't care too much about this suggestion, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use #define _USE_MATH_DEFINES before including cmath or math.h, or use add_definitions(-D_USE_MATH_DEFINES) (as mentioned here)? Or should we just leave it as using boost? We might be able to remove boost from some CMakeList.txts or package.xmls if we change.

Copy link
Member

Choose a reason for hiding this comment

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

If this stackexchange answer is right, you can just do #include <math.h>

https://stackoverflow.com/a/1727896

No need to #define anything or add_definitions as far as I can tell since we're targeting newer systems. I could be wrong, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok great sounds good, I'll switch to that then. Will now close #1373

moveit_core/utils/src/robot_model_test_utils.cpp Outdated Show resolved Hide resolved
@henrygerardmoore
Copy link
Contributor Author

Closes #1373

@AndyZe AndyZe merged commit 491f1c8 into moveit:main Jun 17, 2022
@henrygerardmoore henrygerardmoore deleted the remove_boost2 branch June 17, 2022 16:35
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
* initial pass on removing boost

* removing boost filesystem usage

* removing boost and changing calls to work without boost

* got everything to build and fixed locked robot state test

* made thread joining safer

* remove install and log

* auto formatting

* fixed clang format

* changes solution callback empty check to work with std::function

* removed unnecessary comment

* removing unnecessary boost inclusions

* changed comment

* removing forward declarations

* removed reliance on indirect includes

* final changes, removing unnecessary boost includes from CMakeLists

* removed now-unnecessary package

* added direct includes

* fixed formatting

* added back mistakenly removed cmake command

* removing unnecessary includes thanks to Abi's review

* resolved rebase conflicts

* updated std::optional usage

* changing variant usage to be consistent with std

* fixed outdated calls to boost

* updating comments and adding/removing includes

* added commit and removed comment

* change to std asserts

* removing unnecessary boost usage

* removed boost regex

* fixed bug in string tokenizing

* re-adding boost serializer

* further boost removal

* switching to eigen

* fixing std rng usage

* removing unnecessary includes

* re-adding removed include

* removing more includes

* removing more includes

* removing unnecessary includes

* removing unnecessary includes

* switched back to boost version

* sorting includes alphabetically

* changing to cmath constants instead of boost constants

Co-authored-by: Vatan Aksoy Tezer <vatan@picknik.ai>
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

3 participants