Note that we moved the robot to rviz_default_plugins. This means that it is no longer part of any public interface. Is this the way to go?

It's possible that some of those files belong in the public part of rviz_common. We can move them again later if that's the case.

In that case, there is quite some functionality in robot which is part of its public interface but not used in the display. Shall we delete it?

If you can try to figure out what was using these functions (interactive markers? something else?) then we could decided if they should be moved to rviz_common or if they should be deleted. At the very least, I'd say it would be helpful to enumerate these functions here briefly.

LGTM, the only thing I found I was missing was a file browser button in the text box (usually as an ellipsis at the end of the dialog) which would let me browse to a file rather than type it in.

For a file, does it need to be a URL? Like file:// or can it just be a path. Either way it might be nice to have a resource_retriever/URL option (which could use file:// or package://<package_name>/<relative_path> instead of a file path or topic).

I'd like to have an answer on the removing dead code question and the dependency on dummy_robot_bringup as well as CI before merging. But so far it looks great, thanks!

Regarding the robot, maybe we really want to explicitly have it in rviz_common? Now that I once again read through the code, it seems to be designed for subclassing. The reason I think so is the following comment in Robot::setLinkFactory(...):
https://github.com/ros-visualization/rviz/blob/fe0512e6ce3c7d4ae54d57ec11c8cdd87acca4d5/src/rviz/robot/robot.h#L179

The functionality is never used in RViz (I'll show below). If subclassing is not used outside RViz (probably that can't be answered), the following functions can be removed (links are always to the ROS master and not to our ROS 2 fork):

• Robot::setLinkFactory(...) - sets a Link factory.
• RobotLink::unsetColor() and RobotLink::setColor(...) - this could color a robot link, e.g. when selecting. However, the code is not used in rviz:
  https://github.com/ros-visualization/rviz/search?utf8=%E2%9C%93&q=unsetColor%28&type=
  It might of course be that this functionality is intended to be used in other plugins (the class is at least partially designed for inheritance?).
• RobotLink::setSelectable(...) and RobotLink::getSelectable() - could limit selectability. Currently everything is set to be selectable, but it's not used anyway. See:
  https://github.com/ros-visualization/rviz/search?utf8=%E2%9C%93&q=getSelectable%28%29&type=
  https://github.com/ros-visualization/rviz/search?utf8=%E2%9C%93&q=setSelectable()&type=
• RobotLink::setOnlyRenderDepth(...) and RobotLink::getOnlyRenderDepth() - convenience wrapper around RobotLink::setRenderQueueGroup(uint_8 group) which in turn is only used in that wrapper, but the wrapper is never used, see - it would render the robot in a different queue - maybe to make it less prominent?
  https://github.com/ros-visualization/rviz/search?utf8=%E2%9C%93&q=setOnlyRenderDepth%28&type=
  https://github.com/ros-visualization/rviz/search?utf8=%E2%9C%93&q=getOnlyRenderDepth(&type=
• In addition, there are some getters that I won't list.

Suggestion: We'll leave it in rviz_default_plugins. In a second PR, we'll remove all dead code. That way, it can easily be reverted if somebody complains that he/she needs it.

I added a file picker to the file property. Right now, it'll only work with the normal filepath, no package:// or anything. I also took the liberty to delete the old code from the old rviz package.

I agree that it might be nice to be able to react to package:// or file:// type paths (both send via topic and by just typing), but maybe we'll just add a ticket for now? I'm not exactly sure how one could add resource_retriever + file picker or how the user can be helped there.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

Next try...

• Linux
• Linux-aarch64
• macOS
• Windows

And now with all tests disabled as they can't run: Failures are unrelated

• Linux
• Linux-aarch64
• macOS
• Windows

Rebased after changes on ros2 branch.

New CI:

• Linux
• Linux-aarch64
• macOS (failures unrelated)
• Windows (failures unrelated)

Suggestion: We'll leave it in rviz_default_plugins. In a second PR, we'll remove all dead code. That way, it can easily be reverted if somebody complains that he/she needs it.

Sounds good to me.

I agree that it might be nice to be able to react to package:// or file:// type paths (both send via topic and by just typing), but maybe we'll just add a ticket for now?

That's fine.

I'm not exactly sure how one could add resource_retriever + file picker or how the user can be helped there.

I wasn't thinking that the file picker would be used but result in a package:// or file:// like url, but if the user wanted to type that in manually, it should work. That should be as simple as passing it to resource_retriever and falling back to opening it as a file if it fails.