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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Oct 5, 2021
Merged

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

merged 4 commits into from
Oct 5, 2021

Conversation

spurnvoj
Copy link

Backport #620 to Foxy

@hidmic @spurnvoj
Ensure rviz_common::MessageFilterDisplay processes messages in the ma…
bb884ab 
…in thread (ros2#620)

Since tf2_ros::MessageFilter callback is called from tf2_ros::TransformListener
dedicated thread, concurrency issues will arise if a derived display (like
rviz_defaults_plugins::Polygon) attempts to modify the UI from
rviz_common::MessageFilterDisplay::processMessage().

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@spurnvoj spurnvoj changed the title [backport Foxy] Ensure rviz_common::MessageFilterDisplay processes messages in the main thread (#620) [foxy backport] Ensure rviz_common::MessageFilterDisplay processes messages in the main thread (#620) Sep 13, 2021
@spurnvoj spurnvoj mentioned this pull request Sep 13, 2021
Merged
@ahcorde
Copy link
Contributor

ahcorde commented Sep 13, 2021

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

@ahcorde
Copy link
Contributor

ahcorde commented Sep 13, 2021

  • Windows Build Status

@ahcorde ahcorde requested a review from jacobperron September 17, 2021 11:21
jacobperron
jacobperron reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should accept this change since it is not ABI compatible (ie. it would require users of MessageFilterDisplay to re-compile). I'm not as concerned about the changes to _RosTopicDisplay since that is more of an implementation detail, but I think we should remain ABI compatible with MessageFilterDisplay.

rviz_common/include/rviz_common/ros_topic_display.hpp Outdated
protected Q_SLOTS:
virtual void processTypeErasedMessage(std::shared_ptr<const void> type_erased_message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a virtual method is not ABI compatible

Greg Balke added 2 commits September 23, 2021 13:55
@gbalke gbalke mentioned this pull request Sep 24, 2021
Merged
@gbalke
Copy link
Contributor

gbalke commented Sep 24, 2021

I've made some adjustments that are being PRed to @spurnvoj 's branch. Here are the results from my abi compatibility check:

:~/dev$ abi-dumper --compare ABI-1.dump ABI-2.dump
Added _Z17qRegisterMetaTypeISt10shared_ptrIKvEEiPKcPT_N9QtPrivate21MetaTypeDefinedHelperIS5_Xaasr12QMetaTypeId2IS5_E7DefinedntsrSA_9IsBuiltInEE11DefinedTypeE
Added _Z27qRegisterNormalizedMetaTypeISt10shared_ptrIKvEEiRK10QByteArrayPT_N9QtPrivate21MetaTypeDefinedHelperIS6_Xaasr12QMetaTypeId2IS6_E7DefinedntsrSB_9IsBuiltInEE11DefinedTypeE
Added _ZN11rviz_common16_RosTopicDisplay22typeErasedMessageTakenESt10shared_ptrIKvE
Added _ZN17QtMetaTypePrivate23QMetaTypeFunctionHelperISt10shared_ptrIKvELb1EE8DestructEPv
Added _ZN17QtMetaTypePrivate23QMetaTypeFunctionHelperISt10shared_ptrIKvELb1EE9ConstructEPvPS2_

image

Tested with the following commands in a docker container where pkg_ws contained ros2/rviz:foxy and workspace contained gbalke/rviz:foxy_backport_620:

$ colcon build --merge-install --packages-up-to rviz_common --cmake-args -DBUILD_TESTING=OFF --cmake-args -DCMAKE_CXX_FLAGS="-g -Og" -DCMAKE_C_FLAGS="-g -Og"

$ abi-dumper /home/ubuntu/pkg_ws/build/rviz_common/librviz_common.so -o ABI-1.dump -lver 1 -ld-library-path /home/ubuntu/dev/pkg_ws/build/rviz_ogre_vendor/ogre_install/lib/:/opt/ros/foxy/lib/

$ abi-dumper /home/ubuntu/workspace/build/rviz_common/librviz_common.so -o ABI-2.dump -lver 2 -ld-library-path /home/ubuntu/dev/pkg_ws/build/rviz_ogre_vendor/ogre_install/lib/:/opt/ros/foxy/lib/

$ abi-compliance-checker -l librviz_common -old ABI-1.dump -new ABI-2.dump

@spurnvoj
Merge pull request #1 from gbalke/foxy_backport_620
a1f087e 
Changes to support ABI compatibility
@spurnvoj
Copy link
Author

@gbalke thanks for the changes, looks good. @jacobperron can you please review again the current version?

jacobperron
jacobperron approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @gbalke for looking into an ABI compatible fix.

I'd like to get an extra +1 from one of the original reviewers to confirm that this is ABI-compatible.
@hidmic @clalancette @wjwwood (thanks in advance!)

@gbalke
Copy link
Contributor

gbalke commented Sep 30, 2021

CI up to Rviz:

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

@gbalke
Copy link
Contributor

gbalke commented Oct 1, 2021

@jacobperron seems like the windows build is borked for unrelated reasons. I'll leave it to your best judgement when it comes to merging.

@ahcorde
Copy link
Contributor

ahcorde commented Oct 1, 2021

  • Windows Build Status

@jacobperron
Copy link
Member

Let's try Windows again: Build Status

@ahcorde
Copy link
Contributor

ahcorde commented Oct 5, 2021

Windows warnings are not related.

@ahcorde ahcorde merged commit 154feac into ros2:foxy Oct 5, 2021
@gbalke gbalke mentioned this pull request Oct 8, 2021
Closed
gbalke added a commit that referenced this pull request Oct 8, 2021
@gbalke
Revert "[foxy backport] Ensure rviz_common::MessageFilterDisplay proc…
57c3519 
…esses messages in the main thread (#620) (#765)"

This reverts commit 154feac.
@gbalke gbalke mentioned this pull request Oct 8, 2021
Merged
gbalke added a commit that referenced this pull request Oct 11, 2021
@gbalke
Revert "[foxy backport] Ensure rviz_common::MessageFilterDisplay proc…
f835456 
…esses messages in the main thread (#620) (#765)" (#783)

This reverts commit 154feac.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@jacobperron jacobperron jacobperron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spurnvoj @ahcorde @gbalke @jacobperron @clalancette @hidmic