RoadGeometryConfiguration and RoadNetworkConfiguration key constants not accessible to downstream project

[liangfok]

::malidrive::builder::RoadGeometryConfiguration defines numerous keys: maliput_malidrive/src/maliput_malidrive/builder/road_geometry_configuration.h#L46-L57

::malidrive::builder::RoadGeometryConfiguration does too: maliput_malidrive/src/maliput_malidrive/builder/road_network_configuration.h#L18-L21

These keys are helpful when creating a std::map<std::string, std::string> to pass into ::malidrive::loader::Load().

The problem is the above-mentioned constants are defined in maliput_malidrive/src/ header files. Since they are not in malidrive/include, they should not be accessed by downstream projects.

Victory condition: Move these constants into a header file in maliput_malidrive/include/ and are thus accessible to downstream projects.

[francocipollone]

I see some pros:

• Having constants defined would avoid misspelling keys downstream.
• If keys are modified, unless the name of the variable change, it will be transparent downstream.
• If keys are removed, compile error will occur downstream avoiding having unused parameters in the map.

I have a comment about this: from a user perspective, how the load method is used when using the plugin architecture would be a bit different from the linked-in-compile-time way:

• When using the plugin architecture the user won't be able to include this header file and access the keys. So the aforementioned pros won't apply in this case.

---

Following with the implementation. I plan to place the keys under a namespace like malidrive::builder::params or another name that is suitable

[liangfok]

Interesting note regarding the plugin architecture. Is this because these keys are specific to maliput_malidrive and the plugin architecture is more general (can only reference things shared among all Maliput backends)?

[francocipollone]

Interesting note regarding the plugin architecture. Is this because these keys are specific to maliput_malidrive and the plugin architecture is more general (can only reference things shared among all Maliput backends)?

One comment just to avoid any ambiguity: The keys(the string value) that are needed to be passed are the same for both methods.

Having said that...

When using the maliput plugin architecture the user :

• only "target_link" against maliput.
• instance maliput::plugin::MaliputPluginManager which guarantee that the backends(plugins) are loaded in runtime through dl library.
• get a RoadNetwork selecting the backend/plugin(dragway, multilane or malidrive) you want

Therefore you won't be able to include any header file from the backends.

For reference: See the test in maliput_maladrive: road_network_plugin_test.cc

Or even better the app we have in maliput_integration that uses the plugin systems: maliput_to_string_with_plugins

[liangfok]

Okay, makes sense. Are the set of keys for building a particular backend documented somewhere? I'm thinking they should be listed here: https://shiny-fiesta-6b790eb7.pages.github.io/from_doxygen/html/deps/maliput_malidrive/html/classmalidrive_1_1builder_1_1_road_network_builder.html

[francocipollone]

Okay, makes sense. Are the set of keys for building a particular backend documented somewhere? I'm thinking they should be listed here: https://shiny-fiesta-6b790eb7.pages.github.io/from_doxygen/html/deps/maliput_malidrive/html/classmalidrive_1_1builder_1_1_road_network_builder.html

Each backend should be responsible for correctly documenting its own parameters.

With respect to maliput_malidrive the set of keys available to be used in the builder are documented at the malidrive::loader::Load method: See online doc

[francocipollone]

Let me know @liangfok If I was clear enough, I don't want to add noise. I could provide more information if needed. :+1:

[liangfok]

Yes, it is clear. I see that the keys are documented here: https://shiny-fiesta-6b790eb7.pages.github.io/from_doxygen/html/deps/maliput_malidrive/html/namespacemalidrive_1_1loader.html

[liangfok]

There is potential brittleness between the Load() docstring and the keys defined in road_geometry_configuration.h#L46-L57 and road_network_configuration.h#L18-L21. I wonder whether it would be best to update the Load() docstring to simply reference another header file in malidrive::builder::params that defines the keys.

[francocipollone]

That's true, any change in the builder's parameters should be reflected in the load docstring and nowadays it is something done manually. It's prone to get outdated.

Extending your comment: Having a header file at malidrive::builder::params that can be referenced/used from private entities (like RoadGeometryConfiguration and RoadNetworkConfiguration) and from the public load method's docstring could guarantee to keep it updated in tandem. That would solve the documentation getting outdated issue.

Regarding the visibility of this header file at malidrive::builder::params, and just to be sure after the plugin architecture chat, are you still up with having this public to be used downstream? right?

[liangfok]

Regarding the visibility of this header file at malidrive::builder::params, and just to be sure after the plugin architecture chat, are you still up with having this public to be used downstream? right?

Yes since our downstream project currently doesn't use plugins, and it would be less brittle to have these constants public so that our downstream project can use them.

[francocipollone]

Sure! I will tackle this!

Regarding the visibility of this header file at malidrive::builder::params, and just to be sure after the plugin architecture chat, are you still up with having this public to be used downstream? right?

Yes since our downstream project currently doesn't use plugins, and it would be less brittle to have these constant that our downstream project can use.

Sure! I will tackle this!

[liangfok]

Great! Thank you!

[agalbachicar]

Just a side note: we could also change the API of the plugin to provide a method that returns / populates all the possible keys so it can be inspected via Python.