Loads a monolane YAML file from CLI. #39

agalbachicar · 2018-01-23T16:31:49Z

In this PR you'll find:

GlobalAttirbutes: a super simple library to hold CLI arguments.
A copy of a delphyne cmake macro to look for Drake's libraries.
A copy of ignition gui System.hh file to provide visibility to symbols.
Some basic tests for GlobalAttributes library.
A copy of double_ring.yaml file from Drake to have a sample at hand.
Monolane load in the plugin.

It addresses second and third item of issue #35. It closes #28 .

- Creates a simple library to hold CLI arguments. - Copies a delphyne cmake macro to look for Drake's libraries. - Moves from ignition gui System.hh file to provide visibility to symbols. - Adds test for the CLI library. - Copies double_ring.yaml file from Drake to have a sample at hand.

basicNew

Thanks @agalbachicar ! Left a couple of minor comments. And as usual, wait of a second pair of eyes from Chris or Carlos.

basicNew · 2018-01-23T17:29:49Z

visualizer/double_ring.yaml

@@ -0,0 +1,49 @@
+# -*- yaml -*-


At least on the delphyne side we can take resources from Drake (in particular the car's mesh) by using drake::AddResourceSearchPath(std::string(std::getenv("DRAKE_INSTALL_PATH")) + "/share/drake");. Not sure it applies to yaml files too, but if it did we could save from copying them here.

Having said that, if the former is not an option, let's put this in a different dir (road_samples?)

Done. I had to move it because drake does not export .yaml files when installing.

basicNew · 2018-01-23T17:30:17Z

visualizer/test/GlobalAttributes_TEST.cc

+  EXPECT_EQ(global_attributes::GetArgument(0), std::string("abc_123"));
+  EXPECT_EQ(global_attributes::GetNumberOfArguments(), 1);
+
+


Extra new line?

basicNew · 2018-01-23T17:30:49Z

visualizer/test/GlobalAttributes_TEST.cc

+  return RUN_ALL_TESTS();
+}
+
+}  // namespace backend


AFAIK we are not commenting the namespaces ends (or if we did, you missed them before :))

basicNew · 2018-01-23T17:32:17Z

visualizer/visualizer.cc

-  std::string configFile = initialConfigFile;
-  if (argc > 1) {
-    configFile = argv[1];
+  // Loads all the arguments into global attributes so any plugin can access to


Small suggestion: "Loads all the command line parameters into a set of global attributes, so that any plugin can access them later"

basicNew · 2018-01-23T17:33:34Z

visualizer/test/GlobalAttributes_TEST.cc

+  EXPECT_EQ(global_attributes::SetArgument("def_456"), 1);
+  EXPECT_EQ(global_attributes::GetArgument(0), std::string("abc_123"));
+  EXPECT_EQ(global_attributes::GetArgument(1), std::string("def_456"));
+  EXPECT_EQ(global_attributes::GetNumberOfArguments(), 2);


Wouldn't it be better to have using global_attributes::SetArgument (same with others) at the top to avoid the verbosity in the tests?

Done.

I did it by using the correct namespace.

agalbachicar · 2018-01-25T23:24:04Z

Thanks @basicNew, it's ready for a second review.

basicNew

Thanks @agalbachicar , I don't have further comments. I leave the last word on this to @caguero and/or @clalancette .

clalancette

I have two design concerns with this change:

This is the first instance of delphyne-gui relying on drake itself. In some ways, this sort of breaks the abstraction between the GUI (visualizer) and the backend. While I'm not fundamentally against this, I think it might be a good idea to check with @stonier about this to make sure this is OK.
One of the things that @stonier hated about the old automotive-demo was its reliance on command-line arguments, which quickly got unwieldy. I'm wondering if we could do this a different way. In particular, what if we somehow attached the monolane YAML to the backend, and then had the visualizer request the data via an ignition service? Would that sort of thing make sense?

basicNew · 2018-01-31T21:17:49Z

Thanks @clalancette, both are valid points. Let me add some comments:

The visualizer needs to be able to parse a file (or, later on, a ign transport message) and create a road network in memory. To do this we agreed on using maliput as a library, so there is indeed a dependency
between the road network visualizer and drake (and I think it is ok). However, I agree that we shouldn't depend on the whole libdrake.so but instead find a way to export (or bazel install) only maliput as a library.
Loading the yaml from the command line was a specific requirement for this first iteration. The idea is that someone working on a road network can quickly launch a visualizer from the command line and load a file. However that is just the first version. Further iterations will bring a UI file open prompt and a better integration with the backend to retrieve the model from an ign message. So you are on the right track, but we are just doing a first prototype now :).

basicNew · 2018-02-01T19:51:36Z

@clalancette based on what we discussed in the meeting, do you think this one would be good to go?

clalancette · 2018-02-01T21:35:58Z

@clalancette based on what we discussed in the meeting, do you think this one would be good to go?

Given the discussions, my design questions are answered, so that's good. I'm going to give the code a quick review now.

clalancette · 2018-02-01T21:36:42Z

cmake/TestUtils.cmake

@@ -29,14 +29,15 @@ macro (delphyne_build_tests)
    )

    target_link_libraries(${BINARY_NAME}
-      #${IGNITION-COMMON_LIBRARIES}
+      ${IGNITION-COMMON_LIBRARIES}
      #${IGNITION-GUI_LIBRARIES}


I know you didn't add these, but we should probably just remove all of these commented-out libraries. We can add them as needed.

clalancette · 2018-02-01T21:38:15Z

visualizer/test/GlobalAttributes_TEST.cc

+  EXPECT_EQ(GetArgument(1), std::string("def_456"));
+  EXPECT_EQ(GetNumberOfArguments(), 2);
+}
+


It would probably be a good idea to add a test for the index being out-of-range and throwing an exception.

As the API was changed, a new set of tests were done. I think this one does not apply any more.

clalancette · 2018-02-01T21:42:03Z

visualizer/visualizer.cc

-    configFile = argv[1];
+  // Loads all the command line parameters into a set of global attributes, so
+  // that any plugin can access them later.
+  for (int i = 0; i < argc; ++i) {


I don't think you want to start at 0 here, since that is the name of the program and isn't really a parameter.

clalancette · 2018-02-01T21:47:41Z

visualizer/GlobalAttributes.hh

+/// \throws std::runtime_error When index is negative or it is higher or equal
+/// to the total number of arguments.
+DELPHYNE_GUI_VISIBLE
+std::string GetArgument(const int index);


In general, I really don't like this interface, since it ties an implementation detail of the high-level application (the position of the index in the argv array) into the lower levels, that have to know that. Also, using indices this way means that you can't launch the visualizer with a monolane YAML file and no config file (for instance), which seems like a valid thing to potentially do. I know we want to stay simple here, but I think I would really prefer to use a simple command-line argument parser, make the passing of the YAML files an option (--load-yaml=filename, or similar), and then have this interface be a map between a parameter name and the value (e.g. callers would call GetArgument('load-yaml') and get back filename.

I guess the new code is much more aligned now. Let me know if you think something should be changed.

- Adds a script to easily run the visualizer with MaliputViewer layout.

agalbachicar · 2018-02-02T20:38:57Z

Thanks for your comments @clalancette . I believe the code is much better now.

I've also added a script to run the viewer and ommit Maliput layout when running, so it is less verbose now. After building, you should be able to run it like:

 maliput_viewer.sh --yaml_file=${PATH_TO_DELPHYNE_WS}/install/share/delphyne/road_samples/double_ring.yaml

PTAL.

clalancette

@agalbachicar Thanks a lot! I like this a lot better. There is one backwards-compatibility issue we have to consider here. Prior to this PR, we always passed in the layout as the first, non-option command-line argument. A bunch of scripts in https://github.com/ToyotaResearchInstitute/delphyne rely on that behavior, so if we merge this, those are going to break. I'm fine with making this change (I think this is a big improvement), but could you please do a PR over in delphyne fixing up those users as well? Sorry to keep dragging this out, but I think this will be much better in the end.

agalbachicar · 2018-02-06T13:29:40Z

@clalancette, PR : maliput/delphyne#223 was created to pair delphyne master branch with this PR. PTAL.

caguero · 2018-02-07T16:10:17Z

I see that we're manually parsing all command line arguments. I'm fine with that but if we're going to extend this in the future maybe it's not a terrible idea to add a new gflags dependency or similar to deal with this.

caguero · 2018-02-07T15:55:28Z

visualizer/GlobalAttributes.hh

+
+namespace delphyne {
+namespace gui {
+namespace global_attributes {


Do we need a new namespace? I think the answer depends if we plan to have multiple files related with global attributes. If the answer is yes, I would leave it, although probably in its own directory. Usually, I've seen a new namespace associated to a new directory in the source file. If we don't plan to have additional files related with global attributes maybe we can live without the namespace.

Given that no further archives are going to be added (at least for the moment) to this namespace I chose to create a class to namespace those methods and removed global_attributes namespace.

Let me know if you think this is more convenient.

caguero · 2018-02-07T16:01:28Z

visualizer/test/GlobalAttributes_TEST.cc

+  EXPECT_TRUE(HasArgument("foo"));
+  EXPECT_TRUE(HasArgument("bar"));
+  EXPECT_TRUE(HasArgument("sample_param"));
+  EXPECT_EQ(GetArgument("foo"), "bar");


Consider to swap the parameters on all EXPECT_EQ. The expected value should be in the first place. On failure, this macro outputs text that describes the first argument as the expected and the seconds as the thing under test.

clalancette · 2018-02-07T16:51:48Z

I see that we're manually parsing all command line arguments. I'm fine with that but if we're going to extend this in the future maybe it's not a terrible idea to add a new gflags dependency or similar to deal with this.

Yeah, same here. I think it's fine to not introduce this dependency now, and instead do a separate PR for that. @agalbachicar , if you don't mind, I'd appreciate it if you could open an issue about switching to something more standard for command-line argument parsing.

agalbachicar · 2018-02-07T18:20:14Z

@caguero: thanks for the review! I've addressed your comments. PTAL.

@clalancette : issue #50 was created.

clalancette · 2018-02-07T18:24:41Z

@clalancette : issue #50 was created.

Thank you!

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

agalbachicar added this to the Milestone 2 milestone Jan 23, 2018

agalbachicar assigned basicNew Jan 23, 2018

agalbachicar requested review from clalancette, caguero and basicNew January 23, 2018 16:31

agalbachicar unassigned basicNew Jan 23, 2018

basicNew reviewed Jan 23, 2018

View reviewed changes

Addressed comments I.

41f4710

basicNew approved these changes Jan 26, 2018

View reviewed changes

clalancette reviewed Jan 31, 2018

View reviewed changes

clalancette suggested changes Feb 1, 2018

View reviewed changes

agalbachicar force-pushed the Issue/#28_Load_Maliput_File branch from fc450d8 to a3b54d8 Compare February 2, 2018 20:28

- Addresses Chris' comments.

604554d

- Adds a script to easily run the visualizer with MaliputViewer layout.

agalbachicar force-pushed the Issue/#28_Load_Maliput_File branch from a3b54d8 to 604554d Compare February 2, 2018 20:33

clalancette reviewed Feb 2, 2018

View reviewed changes

This was referenced Feb 5, 2018

Pairs python scripts with delphyne_gui to launch the visualizer. maliput/delphyne#223

Merged

Mesh conversion and visualization #49

Merged

caguero reviewed Feb 7, 2018

View reviewed changes

clalancette approved these changes Feb 7, 2018

View reviewed changes

agalbachicar mentioned this pull request Feb 7, 2018

Command line arguments parser #50

Open

Addresses Carlos' comments.

06df063

caguero approved these changes Feb 7, 2018

View reviewed changes

agalbachicar merged commit 805a6c4 into master Feb 7, 2018

agalbachicar deleted the Issue/#28_Load_Maliput_File branch February 7, 2018 18:24

stonier mentioned this pull request Feb 8, 2018

Maliput viewer #35

Open

16 tasks

		EXPECT_EQ(global_attributes::GetArgument(0), std::string("abc_123"));
		EXPECT_EQ(global_attributes::GetNumberOfArguments(), 1);

Loads a monolane YAML file from CLI. #39

Loads a monolane YAML file from CLI. #39

Conversation

agalbachicar commented Jan 23, 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

agalbachicar commented Jan 25, 2018

basicNew left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

basicNew commented Jan 31, 2018

basicNew commented Feb 1, 2018

clalancette commented Feb 1, 2018

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 commented Feb 2, 2018

clalancette left a comment

Choose a reason for hiding this comment

agalbachicar commented Feb 6, 2018

caguero commented Feb 7, 2018

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

clalancette commented Feb 7, 2018

agalbachicar commented Feb 7, 2018

clalancette commented Feb 7, 2018