robotology / gz-sim-yarp-plugins

YARP plugins for Modern Gazebo (gz-sim).
BSD 3-Clause "New" or "Revised" License
8 stars 0 forks source link

Update configuration parsing logic #116

Closed xela-95 closed 3 months ago

xela-95 commented 3 months ago

[!NOTE]
This PR affects all plugins

With this PR every plugin can specify its yarp configuration both with <yarpConfigurationString> and <yarpConfigurationFile>.

When using <yarpConfigurationFile> the path to the file can be specified in the following ways:

Closes #68 Closes #80

xela-95 commented 3 months ago

@traversaro any idea on why the unit test with the CI fails in loading the plugins? I don't know how to debug this, since on my machine it works. Maybe on my machine gazebo finds the plugin libraries in the install directory?

xela-95 commented 3 months ago

@traversaro any idea on why the unit test with the CI fails in loading the plugins? I don't know how to debug this, since on my machine it works. Maybe on my machine gazebo finds the plugin libraries in the install directory?

Ok found! This test was missing his GZ_SIM_SYSTEM_PLUGIN_PATH environment variable so it was unable to find plugins.

xela-95 commented 3 months ago

Now there is the Windows conda CI failing with:

D:\a\gz-sim-yarp-plugins\gz-sim-yarp-plugins\libraries\common\ConfigurationHelpers.cpp(140): error C2679: binary '=': no operator found which takes a right-hand operand of type 'std::filesystem::path' (or there is no acceptable conversion)

traversaro commented 3 months ago

Relative path

Relative path w.r.t. to what?

By the way, do you think we could document the semantics of yarpConfigurationFile that you wrote down in this PR somewhere in the docs of the repo?

xela-95 commented 3 months ago

Relative path w.r.t. to what?

By the way, do you think we could document the semantics of yarpConfigurationFile that you wrote down in this PR somewhere in the docs of the repo?

Sorry for the ambiguity, the path is relative to the current working directory.

For example, looking at the ForceTorqueTest, the robotinterface path is specified as: https://github.com/robotology/gz-sim-yarp-plugins/blob/9617ad992f062e79212f7b02785345c9deb844d5/tests/forcetorque/model.sdf#L152

and it is actually resolved to the absolute path /home/acroci/repos/gz-sim-yarp-plugins/build/tests/forcetorque/../../../tests/forcetorque/forcetorque_nws.xml

I'm thinking to update the handling of the relative path by using std::filesystem::canonical or std::filesystem::weakly_canonical (see https://en.cppreference.com/w/cpp/filesystem/canonical) in order to remove ./ and ../ sections and have a "canonical" path.

Yep I will also add a section in the README specifying the alternatives to specify paths.

traversaro commented 3 months ago

I am not sure it is is a great feature, basically it (silently, not in a really clear way) constraints the sdf to be opened only if the simulator or the executable using the gz-sim as a library is launched in the directory it is intended to be launched. I guess we need it know for the tests, but probably we can avoid to advertise it/discourage to use it in actual models (a bit like absolute paths).

xela-95 commented 3 months ago

I am not sure it is is a great feature, basically it (silently, not in a really clear way) constraints the sdf to be opened only if the simulator or the executable using the gz-sim as a library is launched in the directory it is intended to be launched. I guess we need it know for the tests, but probably we can avoid to advertise it/discourage to use it in actual models (a bit like absolute paths).

After more carefully thinking about it, I agree with you. I mainly added the relative path feature in order to be compatible with the models already present in tests and tutorials that were using relative paths, but this could become tricky and cumbersome to debug for users.

Do you think it's better to remove this feature for now and refactor the paths in test and tutorials to use the URI? Or we should treat relative paths like absolute ones, by logging a warning for a potentially unreliable path while allowing it?

traversaro commented 3 months ago

Do you think it's better to remove this feature for now and refactor the paths in test and tutorials to use the URI? Or we should treat relative paths like absolute ones, by logging a warning for a potentially unreliable path while allowing it?

I would go for the fast option, that I guess it is the second.

xela-95 commented 3 months ago

macOS test disabled related issue: #90

xela-95 commented 3 months ago

Updated README

xela-95 commented 3 months ago

Merging! 🚀