Migrate RobotModelDisplay #210

Martin-Idel-SI · 2018-02-20T16:36:05Z

Closes #101

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?

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?

wjwwood · 2018-02-22T01:00:37Z

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.

wjwwood · 2018-02-22T02:52:28Z

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).

wjwwood

code review lgtm too, with one comment for a change or a TODO

wjwwood · 2018-02-22T02:55:42Z

rviz_default_plugins/package.xml

@@ -35,6 +35,7 @@
  <test_depend>ament_cmake_gmock</test_depend>
  <test_depend>ament_cmake_lint_cmake</test_depend>
  <test_depend>ament_cmake_uncrustify</test_depend>
+  <test_depend>dummy_robot_bringup</test_depend>


I'm hesitant about this, since I don't expect dummy_robot_bringup to exist forever. I'd rather have it self contained in the rviz repository, though I understand that makes it a lot harder.

I'll take it as-is, but please add a TODO to use something less volatile in the future.

I agree - that's why we added our own urdf file for testing. This is a leftover and we'll remove it. Thanks for catching it!

wjwwood · 2018-02-22T03:01:41Z

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!

Martin-Idel-SI · 2018-02-22T08:30:56Z

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.

Martin-Idel-SI · 2018-02-22T16:01:50Z

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.

Martin-Idel-SI · 2018-02-22T16:04:22Z

CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-02-23T08:07:23Z

Next try...

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-02-23T09:40:52Z

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

Linux
Linux-aarch64
macOS
Windows

- Extract AssimpLoader to new class - Delete code using tinyxml2, use assimp to get the functionality - Scaling is done in the assimp root node

greimela-si · 2018-03-06T13:07:55Z

Rebased after changes on ros2 branch.

New CI:

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

wjwwood · 2018-03-08T18:27:21Z

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.

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

wjwwood

lgtm

wjwwood approved these changes Feb 22, 2018

View reviewed changes

Martin-Idel-SI and others added 21 commits March 6, 2018 13:26

Move last temp files to correct location

0911cef

Expose only what is necessary and add visibility symbols

f3eb461

Adjust namespacing

e02c241

Add tests for mesh_loader

65302fe

Move test_meshes from rviz folder to rviz_redering_tests

45657d4

Convert tifs to pngs

f63863c

Refactor mesh loader

d01b50b

- Extract AssimpLoader to new class - Delete code using tinyxml2, use assimp to get the functionality - Scaling is done in the assimp root node

Add urdf file for testing purposes

b33eb71

Add file and topic property for robot description to robot model display

1a0961f

Extract interface from SelectionManager

7bf4c9f

Move default color material creation to rviz_rendering

7aa60a7

Add robot tests and refactoring of robot

4fe6924

Create common base class for robot_link and robot_joint.

71dbfdb

Reenable tf_prefix and refactor tf_link_updater

d88f2b7

Remove code for old urdf versions

1745046

Hide unreliable property and fix update rate

db2361b

Move stl_loder and assimp_loader to own folder.

186ab48

Move free methods in assimp_loader to class methods

600ce62

Reorder functions in robot

cdf2739

Remove superfluous function in robot_joint

1e03ccd

Add Property::isExpanded() for testability

49a88c6

greimela-si and others added 11 commits March 6, 2018 13:26

Additional property tests for the robot

25a60be

Remove unused slot changedHideSubProperties()

812de98

Disable test (can't run on Windows)

a2ac341

Fix billboard_line_include

7416285

Reorder plugins alphabetically and use old RobotModel description

09b209a

Remove dummy_robot_bringup dependency

df69fb5

Delete code migrated with RobotModelDisplay from old rviz folder

a6f3924

Expose a file_picker_property (similar to color picking)

617ce35

Use file_picking property in robot_model_display

37ff22d

Fix package.xml

79bec3b

Disable tests in rviz_rendering_tests (need display)

c8bc311

greimela-si force-pushed the feature/migrate_robot_model_display branch 2 times, most recently from 218b9e5 to c8bc311 Compare March 6, 2018 12:42

wjwwood added 4 commits March 8, 2018 10:41

remove unnecessary vertical whitespace

c9bac30

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

remove unnecessary vertical whitespace

adcf833

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

remove vertical whitespace

5fb3327

remove vertical whitespace

09e3c1c

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

wjwwood approved these changes Mar 8, 2018

View reviewed changes

wjwwood merged commit 8718ef5 into ros2:ros2 Mar 8, 2018

Martin-Idel-SI deleted the feature/migrate_robot_model_display branch March 9, 2018 09:02

mikaelarguedas mentioned this pull request Mar 21, 2018

Add missing dependency on urdf #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate RobotModelDisplay #210

Migrate RobotModelDisplay #210

Martin-Idel-SI commented Feb 20, 2018 •

edited by wjwwood

Loading

wjwwood commented Feb 22, 2018

wjwwood commented Feb 22, 2018

wjwwood left a comment

wjwwood Feb 22, 2018

Martin-Idel-SI Feb 22, 2018

wjwwood commented Feb 22, 2018

Martin-Idel-SI commented Feb 22, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 22, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 22, 2018

Martin-Idel-SI commented Feb 23, 2018

Martin-Idel-SI commented Feb 23, 2018 •

edited

Loading

greimela-si commented Mar 6, 2018 •

edited

Loading

wjwwood commented Mar 8, 2018

wjwwood left a comment

Migrate RobotModelDisplay #210

Migrate RobotModelDisplay #210

Conversation

Martin-Idel-SI commented Feb 20, 2018 • edited by wjwwood Loading

wjwwood commented Feb 22, 2018

wjwwood commented Feb 22, 2018

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Feb 22, 2018

Choose a reason for hiding this comment

Martin-Idel-SI Feb 22, 2018

Choose a reason for hiding this comment

wjwwood commented Feb 22, 2018

Martin-Idel-SI commented Feb 22, 2018 • edited Loading

Martin-Idel-SI commented Feb 22, 2018 • edited Loading

Martin-Idel-SI commented Feb 22, 2018

Martin-Idel-SI commented Feb 23, 2018

Martin-Idel-SI commented Feb 23, 2018 • edited Loading

greimela-si commented Mar 6, 2018 • edited Loading

wjwwood commented Mar 8, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Feb 20, 2018 •

edited by wjwwood

Loading

Martin-Idel-SI commented Feb 22, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 22, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 23, 2018 •

edited

Loading

greimela-si commented Mar 6, 2018 •

edited

Loading