Provides a MaliputWidgetViewer plugin. #34

agalbachicar · 2018-01-18T12:56:42Z

This PR:

Creates MaliputViewerWidget class from RenderWidget.
Creates layoutMaliputViewer.config to run visualizer with the new widget.

Just to show how the visualizer looks like after running:

visualizer /delphyne_ws/src/delphyne_gui/visualizer/layoutMaliputViewer.config

here is an image.

It closes #26 .

First item of issue #35 .

- Creates MaliputViewerWidget class from RenderWidget. - Creates layoutMaliputViewer.config to run the visualizer with the new widget.

basicNew

Thanks @agalbachicar ! I left a couple of comments

basicNew · 2018-01-18T14:02:08Z

visualizer/CMakeLists.txt

+  ${IGNITION-GUI_LIBRARIES}
+  ${IGNITION-MSGS_LIBRARIES}
+  ${IGNITION-RENDERING_LIBRARIES}
+  ${IGNITION-TRANSPORT_LIBRARIES}


I think this one is not needed yet (although it may be required in the future)

basicNew · 2018-01-18T14:03:47Z

visualizer/MaliputViewerWidget.cc

+  QObject::connect(this->updateTimer, SIGNAL(timeout()), this, SLOT(update()));
+
+  auto paths =
+      ignition::common::SystemPaths::PathsFromEnv("DELPHYNE_PACKAGE_PATH");


IIRC this was needed to be able to load meshes, which I don't think we are doing in this plugin.

basicNew · 2018-01-18T14:04:56Z

visualizer/MaliputViewerWidget.cc

+}
+
+/////////////////////////////////////////////////
+void MaliputViewerWidget::LoadConfig(const tinyxml2::XMLElement* _pluginElem) {


I would keep things simple for the time being and remove the load/save config.

basicNew · 2018-01-18T14:05:39Z

visualizer/MaliputViewerWidget.cc

+}
+
+/////////////////////////////////////////////////
+void MaliputViewerWidget::RenderGroundPlane() {


I would also remove the ground and leave the grid only

basicNew · 2018-01-18T14:08:25Z

visualizer/layoutMaliputViewer.config

+    /* We need an empty stylesheet so that it loads the system's default style */
+  </stylesheet>
+  <menus>
+    <plugins from_paths="false">


I would remove the plugins menu entirely for this layout, as we want it to look like a standalone app.

I left it but changed to visible=false because when I removed the plugin tag the view left it and listed all the available plugins.

basicNew · 2018-01-18T14:09:17Z

~~Adding @clalancette and @caguero for style and more authoritative review on front-end code.~~

Scratch that, they already are.

agalbachicar · 2018-01-18T16:34:58Z

Thanks @basicNew, it's ready for another pass.

basicNew

It LGTM, but please wait for either Carlos or Chris green light. Thanks!

clalancette · 2018-01-19T19:32:36Z

visualizer/CMakeLists.txt

@@ -73,3 +92,7 @@ install (FILES initialLayout.config DESTINATION
 # Install layoutWithTeleop.config
 install (FILES layoutWithTeleop.config DESTINATION
  ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATAROOTDIR}/delphyne/)
+
+# Install layoutMaliputViewer.config
+install (FILES layoutMaliputViewer.config DESTINATION


I know you are just copying what was above, but I think we can probably collapse this down into a single install call, something like:

install (FILES initalLayout.config layoutWithTeleopt.config layoutMaliputViewer.config DESTINATION ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATAROOTDIR}/delphyne/)

clalancette · 2018-01-19T19:32:49Z

visualizer/MaliputViewerWidget.cc

@@ -0,0 +1,351 @@
+// Copyright 2017 Open Source Robotics Foundation


Minor, but it is 2018 now :). (same in the .hh file)

Ha, yes, same thing was noted here: maliput/delphyne#205 (comment)

So, for all new files we should add 2018. If we change one that was created on 2017, should we leave as is?

Replying to @clalancette , moved to 2018.

clalancette · 2018-01-19T19:35:42Z

visualizer/MaliputViewerWidget.hh

+
+#include "OrbitViewControl.hh"
+
+// Forward declarations.


Is there any reason to do these forward declarations here? They aren't used in the header, and the real versions of these classes should get included by the .cc. I'd remove them unless there is a specific reason for them.

I've just forgot to remove them. Thanks.

clalancette · 2018-01-19T19:48:03Z

visualizer/MaliputViewerWidget.cc

+    const double       kCellLength = 1;
+    const unsigned int kVerticalCellCount = 0u;
+
+    this->RenderGrid(kCellCount, kCellLength, kVerticalCellCount,


One thing I'm concerned about with this widget is that it will be relatively easy for the user to confuse it with the RenderWidget for the simulation. One way we might partially avoid that is with a different color grid and background (below) for this widget. What do you think of switching this to another color, maybe light blue or something?

+1, maybe we can use some color that goes hand-in-hand with the colors used to generate the meshes.

What do you think?

How about changing a bit the background color?

It's darker now.

Better?

What's on 52c830b should look like:

agalbachicar · 2018-01-20T19:02:33Z

It's ready for another pass. Thanks @clalancette

Provides a MaliputWidgetViewer plugin.

edfd74f

- Creates MaliputViewerWidget class from RenderWidget. - Creates layoutMaliputViewer.config to run the visualizer with the new widget.

agalbachicar added the component: maliput viewer label Jan 18, 2018

agalbachicar requested review from clalancette, caguero and basicNew January 18, 2018 12:56

basicNew suggested changes Jan 18, 2018

View reviewed changes

Addresses Andres' comments.

183c8f8

basicNew approved these changes Jan 18, 2018

View reviewed changes

clalancette suggested changes Jan 19, 2018

View reviewed changes

agalbachicar force-pushed the Issue/#26_Kickoff_maliput_viewer_widget branch from a15d548 to 45b3482 Compare January 20, 2018 18:55

Second review.

52c830b

agalbachicar force-pushed the Issue/#26_Kickoff_maliput_viewer_widget branch from 45b3482 to 52c830b Compare January 20, 2018 18:59

clalancette approved these changes Jan 23, 2018

View reviewed changes

agalbachicar merged commit 71f8aad into master Jan 23, 2018

agalbachicar deleted the Issue/#26_Kickoff_maliput_viewer_widget branch January 23, 2018 16:22

agalbachicar mentioned this pull request Jan 23, 2018

Maliput viewer #35

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provides a MaliputWidgetViewer plugin. #34

Provides a MaliputWidgetViewer plugin. #34

agalbachicar commented Jan 18, 2018 •

edited

Loading

basicNew left a comment

basicNew Jan 18, 2018

agalbachicar Jan 18, 2018

basicNew Jan 18, 2018

agalbachicar Jan 18, 2018

basicNew Jan 18, 2018

agalbachicar Jan 18, 2018

basicNew Jan 18, 2018

agalbachicar Jan 18, 2018

basicNew Jan 18, 2018

agalbachicar Jan 18, 2018

basicNew commented Jan 18, 2018 •

edited

Loading

agalbachicar commented Jan 18, 2018

basicNew left a comment

clalancette Jan 19, 2018

agalbachicar Jan 20, 2018

clalancette Jan 19, 2018

basicNew Jan 19, 2018

agalbachicar Jan 20, 2018

clalancette Jan 19, 2018

agalbachicar Jan 20, 2018

clalancette Jan 19, 2018

basicNew Jan 19, 2018

agalbachicar Jan 19, 2018

basicNew Jan 19, 2018

agalbachicar Jan 19, 2018

agalbachicar Jan 20, 2018 •

edited

Loading

agalbachicar commented Jan 20, 2018

		@@ -0,0 +1,351 @@
		// Copyright 2017 Open Source Robotics Foundation

Provides a MaliputWidgetViewer plugin. #34

Provides a MaliputWidgetViewer plugin. #34

Conversation

agalbachicar commented Jan 18, 2018 • edited Loading

basicNew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basicNew commented Jan 18, 2018 • edited Loading

agalbachicar commented Jan 18, 2018

basicNew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agalbachicar Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

agalbachicar commented Jan 20, 2018

agalbachicar commented Jan 18, 2018 •

edited

Loading

basicNew commented Jan 18, 2018 •

edited

Loading

agalbachicar Jan 20, 2018 •

edited

Loading