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

Revert "[foxy backport] Ensure rviz_common::MessageFilterDisplay processes messages in the main thread (#620)" #783

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

gbalke
Copy link
Contributor

@gbalke gbalke commented Oct 8, 2021

Reverts #765

See #782. This broke rviz pointcloud displays (and likely other displays dependent on this class). Locally reverted and tested (points are displayed properly).

…esses messages in the main thread (#620) (#765)"

This reverts commit 154feac.
@gbalke gbalke requested a review from jacobperron October 8, 2021 21:22
@gbalke gbalke self-assigned this Oct 8, 2021
@gbalke
Copy link
Contributor Author

gbalke commented Oct 8, 2021

CI up to Rviz:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@gbalke
Copy link
Contributor Author

gbalke commented Oct 8, 2021

To note, #620 works as intended. Apparently the virtual/override removal was the culprit. The code works with them back in place. These two were expected to break ABI which is why they were removed.

diff --git a/rviz_common/include/rviz_common/message_filter_display.hpp b/rviz_common/include/rviz_common/message_filter_display.hpp
index 64d61970..bf46001d 100644
--- a/rviz_common/include/rviz_common/message_filter_display.hpp
+++ b/rviz_common/include/rviz_common/message_filter_display.hpp
@@ -187,7 +187,7 @@ protected:
     Q_EMIT typeErasedMessageTaken(std::static_pointer_cast<const void>(msg));
   }

-  void processTypeErasedMessage(std::shared_ptr<const void> type_erased_msg)
+  void processTypeErasedMessage(std::shared_ptr<const void> type_erased_msg) override
   {
     auto msg = std::static_pointer_cast<const MessageType>(type_erased_msg);

diff --git a/rviz_common/include/rviz_common/ros_topic_display.hpp b/rviz_common/include/rviz_common/ros_topic_display.hpp
index fdbce90b..15411a31 100644
--- a/rviz_common/include/rviz_common/ros_topic_display.hpp
+++ b/rviz_common/include/rviz_common/ros_topic_display.hpp
@@ -116,7 +116,7 @@ Q_SIGNALS:
   void typeErasedMessageTaken(std::shared_ptr<const void> type_erased_message);

 protected Q_SLOTS:
-  void processTypeErasedMessage(std::shared_ptr<const void> type_erased_message)
+  virtual void processTypeErasedMessage(std::shared_ptr<const void> type_erased_message)
   {
     (void)type_erased_message;
   }

@jacobperron
Copy link
Member

@gbalke Could you try out https://github.com/ros2/rviz/tree/jacob/fix_782? I hope fixes the issue and remains ABI compatible.

@gbalke
Copy link
Contributor Author

gbalke commented Oct 11, 2021

@gbalke Could you try out https://github.com/ros2/rviz/tree/jacob/fix_782? I hope fixes the issue and remains ABI compatible.

@jacobperron This isn't working. It immediately crashes on publish of a PointCloud2 message:

QObject::connect: No such signal rviz_common::_RosTopicDisplay::typeErasedMessageTaken(std::shared_ptr<const void>)
QObject::connect:  (sender name:   'PointCloud2')
QObject::connect:  (receiver name: 'PointCloud2')
rviz2: symbol lookup error: /home/ubuntu/dev/scratchwork/issue-782/new_ws/install/lib/librviz_default_plugins.so: undefined symbol: _ZN11rviz_common20MessageFilterDisplayIN11sensor_msgs3msg12PointCloud2_ISaIvEEEE22typeErasedMessageTakenESt10shared_ptrIKvE

@gbalke gbalke merged commit f835456 into foxy Oct 11, 2021
@delete-merged-branch delete-merged-branch bot deleted the revert-765-foxy_backport_620 branch October 11, 2021 17:55
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