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

Remove tf subscription from frame manager and use tf buffer instead #62

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Remove tf subscription from frame manager and use tf buffer instead #62

wants to merge 7 commits into from
+84 −114

Conversation

shrijitsingh99
Copy link

@shrijitsingh99 shrijitsingh99 commented Oct 14, 2020 •
edited
Loading

Summary:

  1. Removes subscription of tf since buffer already does that internally
  2. Since now there is no way to generate an event on an update of tf frames, therefore fetch the latest frames from the frame manager when and where it is needed and not rely on events to automatically update the frames.
  3. Remove locks from functions that are mutually exclusive

@shrijitsingh99
Use TF Buffer to fetch frame pose instead of maintaining a custom map…
f53c8cd 
… of transformations

Also fixes the issues where the tf arrow glitched due to requesting data from tf buffer before it was populated

Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
@shrijitsingh99
Remove all mutex locks in frame manager
12b9018 
Since in all the functions only data is being read and is not being written/updated so there is no need for any lock. 

Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>

Tf buffer itself is thread safe as well.
@shrijitsingh99
Remove frame list changed event
4ea2e7b 
Each plugin can fetch the latest frames from the frame mange as and when needed.
No need of listening to an event.

Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
Sarath18
Sarath18 reviewed Oct 14, 2020
View reviewed changes
Copy link
Collaborator

@Sarath18 Sarath18 left a comment

Choose a reason for hiding this comment

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

Thank you for these improvements!

Please address the following comments.

ign_rviz_common/src/rviz/common/frame_manager.cpp Show resolved Hide resolved
ign_rviz_plugins/res/qml/TFDisplay.qml Outdated Show resolved Hide resolved
ign_rviz_common/include/ignition/rviz/common/rviz_events.hpp Outdated Show resolved Hide resolved
@shrijitsingh99
Add locks when reading fixed frames
7fe3018 
Since atomic operations aren't possible with strings, locks do the job

Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
@shrijitsingh99
Create timer for generating events when frames are updated
e245e75 
Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
@shrijitsingh99
Revert to using event to monitor tf frames
dccf2f8 
Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
@shrijitsingh99
Remove timeout of 1 second when waiting for transform
b8c4c22 
Most plugins require the latest TF and and handle scenarios in case TF is unavailable so no need to wait for TF to become available

Signed-off-by: Shrijit Singh <shrijitsingh99@gmail.com>
@Sarath18
Copy link
Collaborator

Thank you for adding back the FrameListChanged event. Can you revert the changes in the AxesDisplay plugin and GlobalOptions plugin to update Combo-box on receiving the event? Everything else looks good.

@Sarath18 Sarath18 mentioned this pull request Dec 2, 2020
Merged
Base automatically changed from master to main March 8, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sarath18 Sarath18 Sarath18 left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@shrijitsingh99 @Sarath18