From 9bca799953babfbc9d9ed2e31b9aa849d9eac843 Mon Sep 17 00:00:00 2001 From: Marcel Zeilinger Date: Fri, 19 Aug 2022 12:39:24 +0200 Subject: [PATCH 1/3] Add configuration dialog and wait window setting --- rqt_image_overlay/CMakeLists.txt | 2 +- .../resource/configuration_dialog.ui | 66 +++++++++++++++++++ rqt_image_overlay/src/compositor.cpp | 12 +++- rqt_image_overlay/src/compositor.hpp | 6 +- rqt_image_overlay/src/image_overlay.cpp | 40 ++++++++++- rqt_image_overlay/src/image_overlay.hpp | 6 +- 6 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 rqt_image_overlay/resource/configuration_dialog.ui diff --git a/rqt_image_overlay/CMakeLists.txt b/rqt_image_overlay/CMakeLists.txt index 9639bc8..9ca7c70 100644 --- a/rqt_image_overlay/CMakeLists.txt +++ b/rqt_image_overlay/CMakeLists.txt @@ -36,7 +36,7 @@ qt5_wrap_cpp(SOURCES # Must do this for qt's Meta-Object Compiler. src/image_manager.hpp src/color_dialog_delegate.hpp src/overlay_manager_view.hpp) -qt5_wrap_ui(UIS resource/image_overlay.ui) +qt5_wrap_ui(UIS resource/image_overlay.ui resource/configuration_dialog.ui) add_library(rqt_image_overlay SHARED ${SOURCES} ${UIS}) diff --git a/rqt_image_overlay/resource/configuration_dialog.ui b/rqt_image_overlay/resource/configuration_dialog.ui new file mode 100644 index 0000000..68f1a91 --- /dev/null +++ b/rqt_image_overlay/resource/configuration_dialog.ui @@ -0,0 +1,66 @@ + + + ConfigurationDialog + + + + 0 + 0 + + + + Image Overlay configuration + + + + + + + + Waiting &window (sec) + + + Time to wait before composing an image. If overlay messages arrive much later than the image, increase this value. + + + window + + + + + + + 3 + + + + + + + + + Qt::Horizontal + + + QDialogButtonBox::Cancel|QDialogButtonBox::Ok + + + + + + + + + dialog_button_box + accepted() + configuration_dialog + accept() + + + dialog_button_box + rejected() + configuration_dialog + reject() + + + diff --git a/rqt_image_overlay/src/compositor.cpp b/rqt_image_overlay/src/compositor.cpp index 5a578c1..e6b9e60 100644 --- a/rqt_image_overlay/src/compositor.cpp +++ b/rqt_image_overlay/src/compositor.cpp @@ -24,11 +24,19 @@ namespace rqt_image_overlay Compositor::Compositor( const ImageManager & imageManager, const OverlayManager & overlayManager, float frequency, rclcpp::Duration window) -: imageManager(imageManager), overlayManager(overlayManager), window(window) +: imageManager(imageManager), overlayManager(overlayManager), window_(window) { startTimer(1000.0 / frequency); } +void Compositor::setWindow(rclcpp::Duration window) { + window_ = window; +} + +rclcpp::Duration Compositor::window() const { + return window_; +} + void Compositor::setCallableSetImage(std::function)> setImage) { this->setImage = setImage; @@ -40,7 +48,7 @@ std::shared_ptr Compositor::compose() return nullptr; } - rclcpp::Time targetTime = systemClock.now() - window; + rclcpp::Time targetTime = systemClock.now() - window_; auto [composition, imageHeaderTime] = imageManager.getClosestImageAndHeaderTime(targetTime); OverlayTimeInfo overlayTimeInfo{targetTime, imageHeaderTime}; overlayManager.overlay(*composition, overlayTimeInfo); diff --git a/rqt_image_overlay/src/compositor.hpp b/rqt_image_overlay/src/compositor.hpp index efc4c05..2463d09 100644 --- a/rqt_image_overlay/src/compositor.hpp +++ b/rqt_image_overlay/src/compositor.hpp @@ -35,6 +35,10 @@ class Compositor : public QObject const ImageManager & imageManager, const OverlayManager & overlayManager, float frequency, rclcpp::Duration window = rclcpp::Duration{0, 300000000}); + void setWindow(rclcpp::Duration window); + + rclcpp::Duration window() const; + void setCallableSetImage(std::function)> setImage); private: @@ -46,7 +50,7 @@ class Compositor : public QObject std::function)> setImage; - const rclcpp::Duration window; // Wait window for collecting messages before composing image + rclcpp::Duration window_; // Wait window for collecting messages before composing image rclcpp::Clock systemClock{RCL_SYSTEM_TIME}; }; diff --git a/rqt_image_overlay/src/image_overlay.cpp b/rqt_image_overlay/src/image_overlay.cpp index 00f0c08..4365ea9 100644 --- a/rqt_image_overlay/src/image_overlay.cpp +++ b/rqt_image_overlay/src/image_overlay.cpp @@ -16,6 +16,7 @@ #include #include #include "./ui_image_overlay.h" +#include "./ui_configuration_dialog.h" #include "image_overlay.hpp" #include "compositor.hpp" #include "overlay_manager.hpp" @@ -35,6 +36,7 @@ ImageOverlay::~ImageOverlay() = default; void ImageOverlay::initPlugin(qt_gui_cpp::PluginContext & context) { ui = std::make_unique(); + ui_configuration_dialog = std::make_unique(); menu = std::make_unique(); imageManager = std::make_shared(node_); overlayManager = std::make_shared(node_); @@ -97,6 +99,7 @@ void ImageOverlay::saveSettings( instance_settings.setValue("image_topic", QString::fromStdString(imageTopic.topic)); instance_settings.setValue("image_transport", QString::fromStdString(imageTopic.transport)); } + instance_settings.setValue("window", compositor->window().seconds()); overlayManager->saveSettings(instance_settings); } @@ -111,7 +114,17 @@ void ImageOverlay::restoreSettings( imageManager->addImageTopicExplicitly(ImageTopic{topic, transport}); ui->image_topics_combo_box->setCurrentIndex(1); } - + auto window_setting = instance_settings.value("window"); + if (!window_setting.isNull()) { + auto window_string = window_setting.toString().toStdString(); + char * err; + double window = std::strtod(window_string.c_str(), &err); + if (err == window_string.c_str()) { + // double conversion error + } else { + compositor->setWindow(rclcpp::Duration::from_seconds(window)); + } + } overlayManager->restoreSettings(instance_settings); } @@ -132,6 +145,31 @@ void ImageOverlay::fillOverlayMenu() ui->add_overlay_button->setMenu(menu.get()); } +bool ImageOverlay::hasConfiguration() const +{ + return true; +} + +void ImageOverlay::triggerConfiguration() +{ + if (configuration_dialog) { + return; + } + configuration_dialog = std::make_unique(); + configuration_dialog->setAttribute(Qt::WA_DeleteOnClose); + ui_configuration_dialog->setupUi(configuration_dialog.get()); + ui_configuration_dialog->window->setValue(compositor->window().seconds()); + auto connection = connect( + configuration_dialog.get(), &QDialog::finished, [this](int result) { + if (result == QDialog::Accepted) { + auto window_seconds = ui_configuration_dialog->window->value(); + compositor->setWindow(rclcpp::Duration::from_seconds(window_seconds)); + } + configuration_dialog.reset(); + }); + configuration_dialog->open(); +} + } // namespace rqt_image_overlay diff --git a/rqt_image_overlay/src/image_overlay.hpp b/rqt_image_overlay/src/image_overlay.hpp index 77377b4..9bc568f 100644 --- a/rqt_image_overlay/src/image_overlay.hpp +++ b/rqt_image_overlay/src/image_overlay.hpp @@ -20,7 +20,7 @@ #include #include "rqt_gui_cpp/plugin.h" -namespace Ui {class ImageOverlay;} +namespace Ui {class ImageOverlay;class ConfigurationDialog;} namespace rqt_image_overlay { class ImageManager; @@ -42,6 +42,8 @@ class ImageOverlay : public rqt_gui_cpp::Plugin void restoreSettings( const qt_gui_cpp::Settings &, const qt_gui_cpp::Settings & instanceSettings) override; + bool hasConfiguration() const override; + void triggerConfiguration() override; public slots: void removeOverlay(); @@ -50,10 +52,12 @@ public slots: void fillOverlayMenu(); std::unique_ptr ui; + std::unique_ptr ui_configuration_dialog; std::unique_ptr menu; std::shared_ptr imageManager; std::shared_ptr overlayManager; std::unique_ptr compositor; + std::unique_ptr configuration_dialog; }; } // namespace rqt_image_overlay From 97f76b0c1a65ca1784ea5829674c57e004bf0549 Mon Sep 17 00:00:00 2001 From: Kenji Brameld Date: Sat, 20 Aug 2022 21:48:07 +0900 Subject: [PATCH 2/3] reduce complexity, and ensure thread safety Signed-off-by: Kenji Brameld --- rqt_image_overlay/src/compositor.cpp | 22 ++++++++----- rqt_image_overlay/src/compositor.hpp | 15 +++++---- rqt_image_overlay/src/image_overlay.cpp | 42 +++++++++---------------- rqt_image_overlay/src/image_overlay.hpp | 4 +-- 4 files changed, 38 insertions(+), 45 deletions(-) diff --git a/rqt_image_overlay/src/compositor.cpp b/rqt_image_overlay/src/compositor.cpp index e6b9e60..2aad132 100644 --- a/rqt_image_overlay/src/compositor.cpp +++ b/rqt_image_overlay/src/compositor.cpp @@ -22,19 +22,22 @@ namespace rqt_image_overlay { Compositor::Compositor( - const ImageManager & imageManager, const OverlayManager & overlayManager, - float frequency, rclcpp::Duration window) -: imageManager(imageManager), overlayManager(overlayManager), window_(window) + const ImageManager & imageManager, const OverlayManager & overlayManager, float frequency) +: imageManager(imageManager), overlayManager(overlayManager), + window_(std::make_shared(0, 300000000)) { startTimer(1000.0 / frequency); } -void Compositor::setWindow(rclcpp::Duration window) { - window_ = window; +void Compositor::setWindow(const rclcpp::Duration & window) +{ + std::atomic_store(&window_, std::make_shared(window)); } -rclcpp::Duration Compositor::window() const { - return window_; +rclcpp::Duration Compositor::getWindow() const +{ + auto window = std::atomic_load(&window_); + return *window; } void Compositor::setCallableSetImage(std::function)> setImage) @@ -48,7 +51,10 @@ std::shared_ptr Compositor::compose() return nullptr; } - rclcpp::Time targetTime = systemClock.now() - window_; + // Get window_ value through the getWindow() method so that it is thread-safe + auto window = getWindow(); + + rclcpp::Time targetTime = systemClock.now() - window; auto [composition, imageHeaderTime] = imageManager.getClosestImageAndHeaderTime(targetTime); OverlayTimeInfo overlayTimeInfo{targetTime, imageHeaderTime}; overlayManager.overlay(*composition, overlayTimeInfo); diff --git a/rqt_image_overlay/src/compositor.hpp b/rqt_image_overlay/src/compositor.hpp index 2463d09..637ec67 100644 --- a/rqt_image_overlay/src/compositor.hpp +++ b/rqt_image_overlay/src/compositor.hpp @@ -32,12 +32,11 @@ class Compositor : public QObject public: Compositor( - const ImageManager & imageManager, const OverlayManager & overlayManager, - float frequency, rclcpp::Duration window = rclcpp::Duration{0, 300000000}); + const ImageManager & imageManager, const OverlayManager & overlayManager, float frequency); - void setWindow(rclcpp::Duration window); - - rclcpp::Duration window() const; + // Thread safe setter/getter for window + void setWindow(const rclcpp::Duration & window); + rclcpp::Duration getWindow() const; void setCallableSetImage(std::function)> setImage); @@ -50,7 +49,11 @@ class Compositor : public QObject std::function)> setImage; - rclcpp::Duration window_; // Wait window for collecting messages before composing image + // Wait window for collecting messages before composing image. + // To access this value, use the getWindow() method to ensure thread-safety, even from within + // this class. + std::shared_ptr window_; + rclcpp::Clock systemClock{RCL_SYSTEM_TIME}; }; diff --git a/rqt_image_overlay/src/image_overlay.cpp b/rqt_image_overlay/src/image_overlay.cpp index 4365ea9..12298f7 100644 --- a/rqt_image_overlay/src/image_overlay.cpp +++ b/rqt_image_overlay/src/image_overlay.cpp @@ -36,7 +36,6 @@ ImageOverlay::~ImageOverlay() = default; void ImageOverlay::initPlugin(qt_gui_cpp::PluginContext & context) { ui = std::make_unique(); - ui_configuration_dialog = std::make_unique(); menu = std::make_unique(); imageManager = std::make_shared(node_); overlayManager = std::make_shared(node_); @@ -99,8 +98,7 @@ void ImageOverlay::saveSettings( instance_settings.setValue("image_topic", QString::fromStdString(imageTopic.topic)); instance_settings.setValue("image_transport", QString::fromStdString(imageTopic.transport)); } - instance_settings.setValue("window", compositor->window().seconds()); - + instance_settings.setValue("compositor_window", compositor->getWindow().seconds()); overlayManager->saveSettings(instance_settings); } @@ -114,16 +112,11 @@ void ImageOverlay::restoreSettings( imageManager->addImageTopicExplicitly(ImageTopic{topic, transport}); ui->image_topics_combo_box->setCurrentIndex(1); } - auto window_setting = instance_settings.value("window"); - if (!window_setting.isNull()) { - auto window_string = window_setting.toString().toStdString(); - char * err; - double window = std::strtod(window_string.c_str(), &err); - if (err == window_string.c_str()) { - // double conversion error - } else { - compositor->setWindow(rclcpp::Duration::from_seconds(window)); - } + + if (instance_settings.contains("compositor_window")) { + auto window_double = instance_settings.value("compositor_window").toDouble(); + auto duration = rclcpp::Duration::from_seconds(window_double); + compositor->setWindow(duration); } overlayManager->restoreSettings(instance_settings); } @@ -152,22 +145,15 @@ bool ImageOverlay::hasConfiguration() const void ImageOverlay::triggerConfiguration() { - if (configuration_dialog) { - return; + QDialog * configuration_dialog = new QDialog(); + auto ui_configuration_dialog = std::make_unique(); + ui_configuration_dialog->setupUi(configuration_dialog); + ui_configuration_dialog->window->setValue(compositor->getWindow().seconds()); + + if (configuration_dialog->exec() == QDialog::Accepted) { + auto window_seconds = ui_configuration_dialog->window->value(); + compositor->setWindow(rclcpp::Duration::from_seconds(window_seconds)); } - configuration_dialog = std::make_unique(); - configuration_dialog->setAttribute(Qt::WA_DeleteOnClose); - ui_configuration_dialog->setupUi(configuration_dialog.get()); - ui_configuration_dialog->window->setValue(compositor->window().seconds()); - auto connection = connect( - configuration_dialog.get(), &QDialog::finished, [this](int result) { - if (result == QDialog::Accepted) { - auto window_seconds = ui_configuration_dialog->window->value(); - compositor->setWindow(rclcpp::Duration::from_seconds(window_seconds)); - } - configuration_dialog.reset(); - }); - configuration_dialog->open(); } diff --git a/rqt_image_overlay/src/image_overlay.hpp b/rqt_image_overlay/src/image_overlay.hpp index 9bc568f..5772bbd 100644 --- a/rqt_image_overlay/src/image_overlay.hpp +++ b/rqt_image_overlay/src/image_overlay.hpp @@ -20,7 +20,7 @@ #include #include "rqt_gui_cpp/plugin.h" -namespace Ui {class ImageOverlay;class ConfigurationDialog;} +namespace Ui {class ImageOverlay;} namespace rqt_image_overlay { class ImageManager; @@ -52,12 +52,10 @@ public slots: void fillOverlayMenu(); std::unique_ptr ui; - std::unique_ptr ui_configuration_dialog; std::unique_ptr menu; std::shared_ptr imageManager; std::shared_ptr overlayManager; std::unique_ptr compositor; - std::unique_ptr configuration_dialog; }; } // namespace rqt_image_overlay From c16ae306bed7707e2b3fd2caeb9d142f5409ca98 Mon Sep 17 00:00:00 2001 From: Kenji Brameld Date: Sun, 21 Aug 2022 06:22:23 +0900 Subject: [PATCH 3/3] use uniqueptr to prevent memory leak Signed-off-by: Kenji Brameld --- rqt_image_overlay/src/image_overlay.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rqt_image_overlay/src/image_overlay.cpp b/rqt_image_overlay/src/image_overlay.cpp index 12298f7..6ec3d17 100644 --- a/rqt_image_overlay/src/image_overlay.cpp +++ b/rqt_image_overlay/src/image_overlay.cpp @@ -145,9 +145,9 @@ bool ImageOverlay::hasConfiguration() const void ImageOverlay::triggerConfiguration() { - QDialog * configuration_dialog = new QDialog(); + auto configuration_dialog = std::make_unique(); auto ui_configuration_dialog = std::make_unique(); - ui_configuration_dialog->setupUi(configuration_dialog); + ui_configuration_dialog->setupUi(configuration_dialog.get()); ui_configuration_dialog->window->setValue(compositor->getWindow().seconds()); if (configuration_dialog->exec() == QDialog::Accepted) {