Skip to content

Conversation

@ThomasDuvinage
Copy link
Contributor

The goal of this PR is to add ros topic prefix to RobotModule parameters in order for mc_rtc rviz plugin to automatically subscribe and display the robot.

@ThomasDuvinage ThomasDuvinage requested a review from arntanguy May 20, 2025 02:45
@arntanguy
Copy link
Collaborator

Thanks a lot for this, this is indeed a much needed improvement 👍

From a first quick glance at it:

  1. It works. I didn't test complex cases of removing/adding robots on the fly and so on, but at least an FSM controller with multiple robots displays everything as expected, and removes all robots after the controller finishes.
  2. Why do we need to store the topic information in RobotModule::_parameters? This is normally intended to be the list of parameters needed to create the RobotModule itself (<robot module name>, or <robot type (env/object...)>/<description path>/<robot name>, etc).
    I think instead of doing it this way, we could publish the robots by name instead (all robot names are unique). This would remove the special handling of env robots which IMHO is no longer needed, and all robots would be published the same way under control/<robot_name> or real/<robot_name>.

@ThomasDuvinage
Copy link
Contributor Author

@arntanguy
Thanks for the feedbacks.

  • I tried on my side by removing/adding robots on the fly, it works.

  • Thanks for pointing this out, indeed adding topic name to parameters is not the best option. I did that because it was impossible to map robot name and topic due to this env index. I'll update the PR to consider the robot name as part of the topic name.

Copy link
Collaborator

@arntanguy arntanguy 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 taking into account my comments. I have a few more nitpicks and a small additional concern.

Also please rebase onto master, this should now make CI pass.

update_env("control/", controller.robots());
if(publish_real) { update_env("real/", controller.robots()); }

if(controller.robots().size() != mc_rtc::ROSBridge::nb_robot_publisher() / (publish_real ? 2 : 1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this fail if we remove a robot and add a different robot? The size is the same thus we would not remove the extra robots. I think we should explicitly check whether the topic is still required.
Note that in PR #462 we introduced a signal mechanism to notify changes to the robots that would simplify this kind of bookkeeping. We can revisit it once it's merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, a problem will appear here.
Using signals for the #462, would definitely make things better.
I'll remove the condition

@arntanguy
Copy link
Collaborator

Thanks, I merged with master (rebase was tricky due to the many intermediate pre-commit merge), will merge when pipeline succeeds 👍

@arntanguy arntanguy force-pushed the topic/robot-autodisplay branch from f2fdd12 to 01b8700 Compare May 21, 2025 13:03
@arntanguy arntanguy force-pushed the topic/robot-autodisplay branch from 01b8700 to a49d253 Compare May 21, 2025 13:06
@arntanguy arntanguy changed the title [WIP] RVIZ RobotModel dynamic update RVIZ RobotModel dynamic update May 21, 2025
@arntanguy arntanguy merged commit 18f542d into jrl-umi3218:master May 21, 2025
13 checks passed
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