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

Re-implemented setName for tools #989

Merged
merged 2 commits into from
Jun 15, 2023
Merged

Re-implemented setName for tools #989

merged 2 commits into from
Jun 15, 2023

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Jun 6, 2023

In the migration towards RViz2 dynamically setting a tool name seems to got dropped. This PR adds this feature again.

We use one central RViz instance in a multi-mobile-robot application where we have a NavGoal tool for each robot. Not being able to rename the tools is a bit hard to use as you'll have to memorize the actual order.

I don't know whether there is a specific reason why this got dropped, but I think this is valid and useful to keep.

As we currently use ROS 2 Humble, I think this should also be backported to Humble. I can open a second PR if desired.

In the migration towards RViz2 dynamically setting a tool name seems to
got dropped. This commit adds this feature again.
@fmauch fmauch requested a review from ahcorde as a code owner June 6, 2023 09:40
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

@fmauch
Copy link
Contributor Author

fmauch commented Jun 7, 2023

@ahcorde should be fixed now.

@ahcorde
Copy link
Contributor

ahcorde commented Jun 8, 2023

@fmauch, can you share a gif or explain how this feature work?

@fmauch
Copy link
Contributor Author

fmauch commented Jun 9, 2023

Sorry for being so brief, I was assuming that this got kicked out when migrating to RViz2 so I didn't add much explanation. Reading back, it might be that the changes were added to RViz1 in December 2020 after RViz2 was forked out.

Basically, this goes back to this issue and this PR for ROS1 RViz. Reasoning for the changes is well explained in ros-visualization/rviz#1570 (comment).

TLDR: calls to setName() after the tool has been created doesn't have any effect in RViz 2.

I've created a custom tool with a name property, so that we can set its name manually, similar to the action namespace
of the goal tool. Changing the property works fine and it gets saved to the RViz configuration, but it is never applied to the actual tool. The following screenshot shows the tool (the one with the green flag) with its properties opened below. As you can see, the name got applied correctly, the tool also shows up under the new name in the properties view. However, in the toolbar the tool still has the default name "2D Nav Goal".

image

Basically, our initialization looks as follows:

void NavGoalTool::onInitialize()
{
  PoseTool::onInitialize();
  setName(m_name_ptr->getString());
  
// ... more unrelated stuff here...
}

where m_name_ptr has been initialized with

  m_name_ptr.reset(new rviz_common::properties::StringProperty(
    "Name", "2D Nav Goal", "The display name", getPropertyContainer(), SLOT(updateName()), this));

The updateName() method looks like this:

void NavGoalTool::updateName()
{
  setName(m_name_ptr->getString());
}

The docs of setName() suggest that the name can be altered lateron, as well which is not completely wrong, as the actual tool gets renamed in the properties view.

With the changes from this PR applied, the same screenshot with an RViz2 started from the same config looks like this:

image

With this modification we can at least copy the tools inside the config file and rename them so we get a goal tool for each robot:
image

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation @fmauch . In short, this looks good to me, so I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

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

@ahcorde ahcorde merged commit 2052e48 into ros2:rolling Jun 15, 2023
2 checks passed
@fmauch
Copy link
Contributor Author

fmauch commented Jun 16, 2023

Thank you for merging. Is there any chance this could get backported to Humble?

@ahcorde
Copy link
Contributor

ahcorde commented Jun 16, 2023

https://github.com/Mergifyio backport iron humble

@mergify
Copy link

mergify bot commented Jun 16, 2023

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jun 16, 2023
In the migration towards RViz2 dynamically setting a tool name seems to
got dropped. This commit adds this feature again.

(cherry picked from commit 2052e48)
mergify bot pushed a commit that referenced this pull request Jun 16, 2023
In the migration towards RViz2 dynamically setting a tool name seems to
got dropped. This commit adds this feature again.

(cherry picked from commit 2052e48)
ahcorde pushed a commit that referenced this pull request Jun 19, 2023
In the migration towards RViz2 dynamically setting a tool name seems to
got dropped. This commit adds this feature again.

(cherry picked from commit 2052e48)

Co-authored-by: Felix Exner (fexner) <felix_mauch@web.de>
ahcorde added a commit that referenced this pull request Jun 19, 2023
Re-implemented setName for tools (backport #989)
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.

None yet

3 participants