search

robotology / idyntree

Multibody Dynamics Library designed for Free Floating Robots
BSD 3-Clause "New" or "Revised" License
169 stars 67 forks source link

linkIndices in iDynTreeWrappers matlab visualizer differs from model #809

Open lrapetti opened 3 years ago

lrapetti commented 3 years ago

I noticed that when using the matlab visualizer in iDynTreeWrappers, there is the possibility to change the visualization options of the links using iDynTreeWrappers.modifyLinksVisualization method.

This method can be used by passing a list of links indices using the linkIndices options, e.g. we can call it as:

iDynTreeWrappers.modifyLinksVisualization(visualizer,'linksIndices', [5 6 7 ...], ...);

However, the indices passed in this function should not correspond to the links indices used in the iDynTree model (that is 0-based), but corresponds to the indices of the links in the matlab vector visualizer.linkNames (that is 1-based) as you can see in https://github.com/robotology/idyntree/blob/master/bindings/matlab/%2BiDynTreeWrappers/modifyLinksVisualization.m#L40.

I think this can be a source of confusion, and it would be better to use the iDynTree model indices also in this parameter.

traversaro commented 3 years ago

Can't we use the link names instead? Those are much less ambiguous. See also https://github.com/robotology/idyntree/issues/805 .

lrapetti commented 3 years ago

Can't we use the link names instead? Those are much less ambiguous. See also #805 .

Yes, it can be done using the option linksToModify in place of linkIndices. If there is not specific reason/users to maintain the linkIndices option, we can remove it

traversaro commented 3 years ago

Can't we use the link names instead? Those are much less ambiguous. See also #805 .

Yes, it can be done using the option linksToModify in place of linkIndices. If there is not specific reason/users to maintain the linkIndices option, we can remove it

Let's deprecate it for now, and then in iDynTree 4 we can remove it.

traversaro commented 3 years ago

Mentioning a few users that may be interested in this discussion: @nunoguedelha @CarlottaSartore @Giulero @singhbal-baljinder @gabrielenava @lrapetti @fjandrad .

gabrielenava commented 3 years ago

If there is not specific reason/users to maintain the linkIndices option, we can remove it

I think link names are perfect, and from my side I don't think it is a problem to deprecate the indexes in the next iDynTree release

nunoguedelha commented 3 years ago

Let's deprecate it for now, and then in iDynTree 4 we can remove it.

I would have an objection regarding this...

As far as I understood, @lrapetti 's proposal was to remove the linkIndices option from iDynTreeWrappers which should be ok. But removing it from iDynTree altogether is another story. It would introduce some limitations in Simulink if we want to extend the range of iDynTree functions implemented as WBT blocks. This limitation is related to the fact that Simulink does not support strings in block input signals, but only in block parameters (processed at compile time).

For instance:

The ForwardKinematics WBT block has a single input parameter which is the "frame name" we wish to get the homogeneous transform w.r.t. the world. "frame name" is a string, and is passed at compile time. Such function, or other iDynTree functions that do some processing depending on a frame name, have an equivalent function taking a frame index instead. For those functions, we can create a WBT block that would have a frame index as input signal.

This won't be possible anymore without having a specific conversion [frame index -> frame name] inside each WBT.

lrapetti commented 3 years ago

I agree with @nunoguedelha that removing link indices from whole iDynTree might be limiting, but I think the discussion here was strictly related to the interface in the matlabWrapper visualizer.

@traversaro how would you proceed for depracating the option from the Matlab wrapper? I didn't find any example rather then what done in https://github.com/robotology/idyntree/pull/744/files#diff-a7e1dfcd9f56abf2dab658c921be377adfc0fb1fd5ae46743d06adc07fd535a5R76, do you think adding a comment in the README/documentation would be enough, or should we display some Matlab-warning message?

traversaro commented 3 years ago

Let's deprecate it for now, and then in iDynTree 4 we can remove it.

I would have an objection regarding this...

I was referring to the linkIndeces parameter of the iDynTreeWrappers.modifyLinksVisualization MATLAB function. I totally agree taht link indeces are quite useful in iDynTree, and they will not be removed!

traversaro commented 3 years ago

@traversaro how would you proceed for depracating the option from the Matlab wrapper? I didn't find any example rather then what done in https://github.com/robotology/idyntree/pull/744/files#diff-a7e1dfcd9f56abf2dab658c921be377adfc0fb1fd5ae46743d06adc07fd535a5R76, do you think adding a comment in the README/documentation would be enough, or should we display some Matlab-warning message?

I was thinking something like:

if 'linksIndices' is in the arguments % Sorry, I Am not sure how to do that in MATLAB
    warning('This parameter is deprecated, etc etc')
end

using MATLAB built-in support for warnings (see https://www.mathworks.com/help/matlab/ref/warning.html). I think this is quite idiomatic and difficult to ignore, but if you have in mind other alternatives feel free to propose them!