-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add Example for Depalletizing Using MoveIt Task Constructor #184
base: master
Are you sure you want to change the base?
Conversation
Once the PR is merged, I will add the necessary documentation to the wiki. |
Quality Gate passedIssues Measures |
.../iiqka_moveit_task_constructor_example/kuka_moveit_task_constructor/launch/startup.launch.py
Outdated
Show resolved
Hide resolved
auto mtc_depalletizing_task_node = std::make_shared<MTCDepalletizingTaskNode>(options); | ||
rclcpp::executors::MultiThreadedExecutor executor; | ||
|
||
auto spin_thread = std::make_unique<std::thread>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason for making this thread a unique ptr instead of simple var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sometimes use pointers for threading (mostly when they might throw an exception).
Advantages:
Automatic Cleanup: std::unique_ptr ensures the thread is cleaned up when it goes out of scope, preventing resource leaks.
Exception Safety: Ensures the thread’s destructor is called even if an exception is thrown, joining the thread if necessary.
Clear Ownership: Indicates single ownership, making the code easier to understand and maintain.
Disadvantages:
Overhead: May introduce unnecessary overhead in terms of performance and complexity.
Move Semantics: std::thread is already move-only, and wrapping it in a smart pointer can complicate the design.
Readability: Can reduce code readability and clarity by adding another layer of abstraction
Yes, I know it's a simple demo application and std::thread is pretty robust itself, I can change it to a simple var if you wish. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatic Cleanup: std::unique_ptr ensures the thread is cleaned up when it goes out of scope, preventing resource leaks.
Exception Safety: Ensures the thread’s destructor is called even if an exception is thrown, joining the thread if necessary.
are these not only true, if you implement a custom deleter for std::thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the custom deleter is a better approach, as it can gracefully stop a thread, while in the case of a smart pointer, if the pointer runs out of scope, the thread's destructor is called, which terminates the thread abruptly. That's why I manually joined the thread in the example. Having a custom deleter would be more ideal, I just did not think this simple example would need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But compared to the simple std::thread, what is the difference without the custom deleter? the simple var would also run out of scope and the destructor would be called. So I do not see why the unique ptr is better in this case, as without the deleter it will terminate the same way (also in case of an exception, where join() would not be called automatically)
(I do not necessarily want to change to simple var, just want to understand the effects of both and without a custom deleter I think your first two advantages are not valid, as the behaviours of both cases are identical)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read a bit about how exactly it works. Yes, you are right, the only reason one might want to use a smart pointer for storing a thread is for ownership purposes. It does not really apply for our case, so changing it back to a simple var makes a lot of sense.
...it_task_constructor_example/kuka_moveit_task_constructor/src/mtc_depalletizing_task_node.cpp
Show resolved
Hide resolved
...constructor_example/kuka_moveit_task_constructor/launch/startup_with_rviz_extended.launch.py
Outdated
Show resolved
Hide resolved
I do not think we can merge this to master currently, as mtc is not released and would therefore block our release, as the release process does not allow upstream repos. Maybe merge to rolling, but we should discuss this :) |
Yeah, I hope MTC will be available through apt soon(tm). We can discuss what to do tomorrow. :) |
This Pull Request introduces a demonstration package for depalletizing tasks. The package leverages the MoveIt Task Constructor framework and is designed for use with KUKA robots.