Skip to content

Add Example for Depalletizing Using MoveIt Task Constructor#184

Open
PetoAdam wants to merge 16 commits into
masterfrom
feature/moveit_task_constructor
Open

Add Example for Depalletizing Using MoveIt Task Constructor#184
PetoAdam wants to merge 16 commits into
masterfrom
feature/moveit_task_constructor

Conversation

@PetoAdam

Copy link
Copy Markdown
Collaborator

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.

@PetoAdam

Copy link
Copy Markdown
Collaborator Author

Once the PR is merged, I will add the necessary documentation to the wiki.

@sonarqubecloud

Copy link
Copy Markdown

auto mtc_depalletizing_task_node = std::make_shared<MTCDepalletizingTaskNode>(options);
rclcpp::executors::MultiThreadedExecutor executor;

auto spin_thread = std::make_unique<std::thread>(

Copy link
Copy Markdown
Member

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?

@PetoAdam PetoAdam Sep 17, 2024

Copy link
Copy Markdown
Collaborator Author

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. :)

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Collaborator Author

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.

Copy link
Copy Markdown
Member

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)

Copy link
Copy Markdown
Collaborator Author

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.

@Svastits

Copy link
Copy Markdown
Member

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 :)

@PetoAdam

Copy link
Copy Markdown
Collaborator Author

Yeah, I hope MTC will be available through apt soon(tm). We can discuss what to do tomorrow. :)

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.

2 participants