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

[traversaro]

The Matlab-only visualizer in

idyntree/bindings/matlab/+iDynTreeWrappers/getMeshes.m

Line 46 in 50f5879

% Import an STL mesh, returning a PATCH-compatible face-vertex structure

contained an hack to overcome #291 . As #291 was properly solved in #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 #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:

idyntree/examples/matlab/iDynTreeWrappers/visualizeRobot.m

Line 84 in 17d5a7d

[visualizer,objects]=iDynTreeWrappers.prepareVisualization(KinDynModel,meshFilePrefix,...

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:

[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:

idyntree/bindings/matlab/+iDynTreeWrappers/prepareVisualization.m

Line 69 in 64cbaf7

parse(p,KinDynModel,meshFilePrefix,varargin{:});

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:

idyntree/bindings/matlab/+iDynTreeWrappers/prepareVisualization.m

Line 69 in 64cbaf7

parse(p,KinDynModel,meshFilePrefix,varargin{:});

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

idyntree/bindings/matlab/+iDynTreeWrappers/getMeshes.m

Line 52 in 2aa71a2

mesh_triangles = stlread([meshFilePrefix meshFile]);

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