Idyntree: Update Matlab-only visualizer to make the use of meshFilePrefix optional

[traversaro]

The Matlab-only visualizer in https://github.com/robotology/idyntree/blob/50f58798c1ab37d6656754c0fe688bc96e38a1a1/bindings/matlab/%2BiDynTreeWrappers/getMeshes.m#L46 contained an hack to overcome https://github.com/robotology/idyntree/issues/291 . As https://github.com/robotology/idyntree/issues/291 was properly solved in https://github.com/robotology/idyntree/pull/798, we need to permit the use of the new feature if meshFilePrefix is set to the default value (empty?).

We should also deprecate the meshFilePrefix option, so that we can remove it in iDynTree 3.

[traversaro]

I think that for this version using the fact that if one specifies "" for meshFilePrefix this is interpreted as the fact that meshFilePrefix should not be used and instead the mechanism introduced in https://github.com/robotology/idyntree/pull/798 is used make sense. However, if we then remove the use of meshFilePrefix at all, the calls to prepareVisualization that explicitly pass a meshFilePrefix such as in:
https://github.com/robotology/idyntree/blob/17d5a7db7f7190944aa6d13f96daaf1d4615946c/examples/matlab/iDynTreeWrappers/visualizeRobot.m#L84
would still work, or that will creates problems in parsing the varargin arguments ?

Asking to MATLAB experts here @nunoguedelha @CarlottaSartore @Giulero @singhbal-baljinder @gabrielenava

[gabrielenava]

If I understood the problem, I think as it is written now it will create problems if we remove the variable meshFilePrefix in the future. But at a first glance does not seem a big issue, we can modify the call to prepareVisualization I think:

https://github.com/robotology/idyntree/blob/devel/bindings/matlab/%2BiDynTreeWrappers/prepareVisualization.m#L1

[traversaro]

If I understood the problem, I think as it is written now it will create problems if we remove the variable meshFilePrefix in the future. But at a first glance does not seem a big issue, we can modify the call to prepareVisualization I think:

https://github.com/robotology/idyntree/blob/devel/bindings/matlab/%2BiDynTreeWrappers/prepareVisualization.m#L1

Yes, my question is: if we remove the prepareVisualization signature from:
~
function [Visualizer,Objects]=prepareVisualization(KinDynModel,meshFilePrefix,varargin)
~
to
~
function [Visualizer,Objects]=prepareVisualization(KinDynModel,varargin)
~

Will existing code like:
~matlab
[visualizer,objects]=iDynTreeWrappers.prepareVisualization(KinDynModel,meshFilePrefix,...
'color',[1,1,1],'transparency',1, 'name', ['Plot frame ', robotName], 'reuseFigure', 'name');
~
continue to work as meshFilePrefix will go to varargin and safely ignored as it is not a used key for an option, or would create problems?
It this will create problems, probably a safer option is just to already create new method:
~
function [Visualizer,Objects]=prepareVisualizer(KinDynModel,varargin)
~
and then when we remove support for meshFilePrefix we just remove the prepareVisualization, so the error will be clear for anyone and not hidden. Furthermore, if we leave both for the next release of iDynTree, users can migrate when they want from prepareVisualization to prepareVisualizer .

[gabrielenava]

Will existing code like:

[visualizer,objects]=iDynTreeWrappers.prepareVisualization(KinDynModel,meshFilePrefix,...
'color',[1,1,1],'transparency',1, 'name', ['Plot frame ', robotName], 'reuseFigure', 'name');

continue to work as meshFilePrefix will go to varargin and safely ignored as it is not a used key for an option, or would create problems?

I this case again at a first glance, I think the varargin will still work. Provided that this line that parses all function inputs is properly adjusted:

https://github.com/robotology/idyntree/blob/64cbaf7e35a0d545167cc9819f4d248d08aa5031/bindings/matlab/%2BiDynTreeWrappers/prepareVisualization.m#L69

In particular, the MATLAB parse function is used, and all the inputs are saved in p.Results.

[traversaro]

Will existing code like:
[visualizer,objects]=iDynTreeWrappers.prepareVisualization(KinDynModel,meshFilePrefix,...
'color',[1,1,1],'transparency',1, 'name', ['Plot frame ', robotName], 'reuseFigure', 'name');
continue to work as meshFilePrefix will go to varargin and safely ignored as it is not a used key for an option, or would create problems?

I this case again at a first glance, I think the varargin will still work. Provided that this line that parses the varargin is properly adjusted:

https://github.com/robotology/idyntree/blob/64cbaf7e35a0d545167cc9819f4d248d08aa5031/bindings/matlab/%2BiDynTreeWrappers/prepareVisualization.m#L69

The MATLAB parse function is used, and all the inputs are saved in p.Results.

Good point, the meshFilePrefix string will probably just go in p.Unmatched, without creating any harm https://www.mathworks.com/help/matlab/matlab_prog/parse-function-inputs.html#d122e45544

[fjandrad]

Hi longtime no see. Probably not relevant anymore since you want to deprecate _meshfileprefix_ , but I tested the matlab bindings today using the docker image diegoferigo/development:latest that has iDyntree in Master branch with Matlab 2020b. I got an error when actually using the _meshfileprefix_ variable :

meshFilePrefix = 

    "~/../../iit/sources/robotology-superbuild/build/install/share"

Error using stlread (line 23)
File name must be a string scalar or character vector.

Error in iDynTreeWrappers.getMeshes (line 52)
                    mesh_triangles = stlread([meshFilePrefix meshFile]);

Error in iDynTreeWrappers.prepareVisualization (line 74)
[linkMeshInfo,map]=iDynTreeWrappers.getMeshes(model,meshFilePrefix);

Error in testidyntree (line 76)
[visualizer,objects]=iDynTreeWrappers.prepareVisualization(KinDynModel,meshFilePrefix,...

Solved it by simply changing https://github.com/robotology/idyntree/blob/2aa71a2794567b39976d0af092b10513ce774a88/bindings/matlab/%2BiDynTreeWrappers/getMeshes.m#L52 to

mesh_triangles = stlread([meshFilePrefix+meshFile]);

Probably not worth making a commit for it but it makes me think _stlread_ might have changed its behavior or the concatenation works differently now ( which would be odd ). So this is just a heads up.

[traversaro]

Hi @fjandrad, thanks for reporting this. Probably we could move that to a separate issue. cc @CarlottaSartore