Fix/refactor codebase #1
Conversation
- Refactored test_scenario_perception_pipeline.cpp - Added event handler because of interaction problem between panda_arm_spawner and test_scenario_perception_pipeline node
| class ScenarioPerceptionPipelineTestCaseCreator | ||
| { | ||
| private: | ||
| static inline std::vector<std::vector<geometry_msgs::msg::Pose>> test_cases_ = {}; |
There was a problem hiding this comment.
Is this part okay or do we need to get better implementation?
There was a problem hiding this comment.
I also need some insights about this part, @sjahr, @henningkayser
|
|
||
| BENCHMARK_DEFINE_F(ScenarioPerceptionPipelineFixture, test_scenario_perception_pipeline)(benchmark::State& st) | ||
| { | ||
| auto selected_test_case = ScenarioPerceptionPipelineTestCaseCreator::selectTestCases(0); |
There was a problem hiding this comment.
Test case selection must be based on given arguments to benchmark class.
There was a problem hiding this comment.
Dear @henningkayser and @sjahr, I actually solved this issue by giving this argument in launch file. But i need some insights related to this part.
|
Dear @henningkayser and @sjahr , I need some review for beginning of implementations. I already reviewed some parts before. While reviewing, it would be great to review all parts of code, not changed parts at the moment. Thank you in advance for your efforts 🙏 |
There was a problem hiding this comment.
Thank you! I gave it a first high-level pass, still need to test it though. It is great that you have already developed so much and we can start refining it 👍 An important first step is to restructure the repository such that it follows the same patterns as MoveIt does and thus make it easier to review and iterate for all of us (see my comments)
| } | ||
| } | ||
| moveit::planning_interface::MoveGroupInterface::Plan plan_msg; | ||
| const auto ok = static_cast<bool>(move_group_interface_ptr_->plan(plan_msg)); |
There was a problem hiding this comment.
That static_cast shouldn't be necessary because the result class has a bool operator. I think you could just put move_group_interface_ptr_->plan(plan_msg) into the if statement without temporary variables
There was a problem hiding this comment.
I have stolen here 🙈 . But it is bad practice. I have yet to solve this part but i will. We must fix this bad practice in documentation as well.
|
|
||
| if (ok) | ||
| { | ||
| move_group_interface_ptr_->execute(plan_msg); |
There was a problem hiding this comment.
Do we need the execution at all? If not you could just return move_group_interface_ptr_->plan(plan_msg) with from this function
There was a problem hiding this comment.
I couldn't understand your question. Wasn't our goal to test all pipeline rather than just planning pipeline? By the way i think the effectively measuring of middleware benchmarking is proportional to data-transfering works. I'm wondering your ideas about all of it.
| @@ -7,92 +7,165 @@ | |||
| #include <moveit/moveit_cpp/planning_component.h> | |||
There was a problem hiding this comment.
I think you should separate the declarations form the implementations by adding header files for your classes. That way everything would be more cleaned up and easier to work with. I'd propose this repository structure:
launch/
include/
scenario_perception_pipeline.hpp --> Contains class definitions
src/
scenario_perception_pipeline.cpp --> Contains all the class definitions
benchmark_main.cpp --> contains just the main function
test/ --> Usually this contains the unittests (you can neglect it for now until we get to the point where we write unittests)
There was a problem hiding this comment.
I refactored based on your review with a couple of details. Could you take a look at it and then give a feedback again?
- Added upstream workspace envrionment to CI - Changed ROS version in CI - Fixed CMakeList.txt and package.xml based on the feedbacks of CI - Fixed upstream repository urls - Changed test cases for more convenient benchmarking
- Added middleware implementations to exec_depend ( #1 (comment) ) - Refactored package directory tree structure ( #1 (comment) ) - Added namespace ( #1 (comment) )
@sjahr Is there any documentation of how to create a new moveit repository and what its procedure is e.g. here? Or is there any boilerplate to create moveit-like repository? If the answers are yes, maybe i can help you to spend the less effort by applying this steps. |
- Added documentation for each methods and variables except global variables ( #1 (comment) ) - Put constants into anoymous namespace ( #1 (comment) ) - Modified condition checking in senfTargetPose ( #1 (comment) ) - Fixed middleware packages in package.xml - Renamed the test case config
- Removed ScenarioPerceptionPipelineTestCaseGenerator class and added its methods to ScenarioPerceptionPipelineFixture - Renamed ScenarioPerceptionPipelineFixture/selectTestCase method as ScenarioPerceptionPipelineFixture/selectTestCase - Added documentation for ScenarioPerceptionPipelineFixture/setUp and ScenarioPerceptionPipelineFixture/tearDown methods
|
I'm closing this PR because the PR started to be so complex. It's great to continue from another PR. |
No description provided.