New matlab visualization #668
Conversation
There was a problem hiding this comment.
Thanks, I made a first round of reviews. The first comment is: how is this supposed to be used? These files are not installed anywhere, so they are not found in the MATLAB path of the users that follow the documentation on how to use the bindings. Furthermore, could it make sense that instead of creating a new Visualization directory, we put this functions under the existing +iDynTreeWrappers directory so that installation is already sorted out?
Other aspects:
- Does this run in Octave? If not, we should document it.
|
It would be useful if also @gabrielenava could review this PR. |
|
Last request: could we change https://github.com/robotology/idyntree/blob/devel/bindings/iDynTree.i#L306 to |
That was something I had asked @gabrielenava in the beginning it makes sense to me to have it iDynTreeWrappers since I use the wrapper. So I will add them there and update the example. I do not know if it works in Octave. I would have to install and check. My gut feeling is that it doesn't. In the past I saw that Octave and Matlab differ when plotting and handling of figures cones in the picture. Do not know if that changed. If it's compatible it would be really nice. |
|
Also the visualize robot file should not go to the iDynTreeWrappers. Where should I put it? Is supposed to be an example/tutorial. There is also the file I used for benchmarking the times. Should I delete it? It served its purpose so now it does not really have a reason to stay in the repo. If we merge without squashing is enough for me to be there in the history. |
You can also just document that in Octave is not tested.
Yes, any additional file in a repo is a burden to maintain, so if you do not have any use for it please delete it. You can also just copy it in an issue comment so if you need to recover it is there, even because ideally the commit history will need to be cleanup before merging the PR. |
fjandrad
force-pushed
the
newMatlabVisualization
branch
3 times, most recently
from
c4ffec6
to
cfac9c4
Compare
Sorry it was a typo, I mean to cleanup the commits w.r.t. ot the base branch of the PR. |
fjandrad
force-pushed
the
newMatlabVisualization
branch
from
bfba59a
to
8beaacc
Compare
|
CC |
|
Thanks @fjandrad , that is much better. @gabrielenava I think we need your review just on the modifications in the commit 8beaacc (and later minor fixes in 4f1678a and d664edc). |
There was a problem hiding this comment.
Ok for me, but let's wait for @gabrielenava review when he is available.
|
@gabrielenava if you comment on the lines from the PR instead of opening the separate commit and commenting on those, then the discussion are more manageable, because you can reply directly from the PR, i.e. this is good: This is not convenient instead: |
|
To clarify, even in the "Files Changed" view of the PR, you can still select which commit (or files) to review, by changing the option under "Change from": |
There was a problem hiding this comment.
For what concerns my above comments, you can better see them by opening this commit: 8beaacc
| zlabel('{z}'); | ||
|
|
||
| % Apply views | ||
| view(parent,custom_view); |
There was a problem hiding this comment.
missing void line at the end also here (test to see if now my comment is fine)
| @@ -0,0 +1,27 @@ | |||
| function []=updateVisualization(KinDynModel,linkNames,transforms) | |||
There was a problem hiding this comment.
I see there are not debug checks in the wrappers but maybe we can add them later (if necessary), based on the users feedback.
There was a problem hiding this comment.
Which kind of debug things would you add?
There was a problem hiding this comment.
For example, on the previous visualizer based on irrlicht sometimes the robot was not visualized correctly (I saw a white window), but the code was not stopping and the error message was only on the Matlab terminal or absent (e.g. I had this issue when using softwareopengl option in Matlab). I would say that we need to be sure that in case something in the visualization fails, it is clear for the user why the robot is not visualized correctly. But said this, it is too early now and we need to first use the visualizer in order to understand which error messages may help the user.
|
I added the lines at the end of the files and address your comments. @gabrielenava let me know if its fine for you. |
|
Great, finally done. Can you please curate the commit once more so that we can merge? Thanks! |
…b visualizer. Add visualizer functions and self-contained tutorial/example. Changes to iDynTreeWrappers Address review Delete meshes and update example accordingly. Return empty vector if a wrong frame is requested. Use the appropiate invalid frame index. Erase commented line not used in mat4x4vec.i. Delete user variables to leave clean example Corrected typo, added extra line at the end and eliminate extra lines at the end Add line a the end
fjandrad
force-pushed
the
newMatlabVisualization
branch
from
ada523f
to
293ed0f
Compare
|
@traversaro done I think is ready to merge or fast forward it should be on top of current devel |
|
|
|
Thanks @fjandrad and @gabrielenava ! |
|
CC @robotology/iit-dynamic-interaction-control |
@fjandrad @traversaro the link in the first comments is broken, Please update it with the correct reference which, if I am not wrong is https://github.com/robotology/idyntree/blob/master/examples/iDynTreeWrappers/visualizeRobot.m |
Thanks, I updated it with the correct version, just linking the version v1.1.0 as the master version can eventually change in the future. |
This PR provides the functions to create a Matlab iDyntree visualizer.
It uses Matlab plotting and stl reading capabilities and the forward kinematics for floating base robots of iDyntree.
The two main functions are :
A tutorial/testing script can also be found in visualizeRobot.
A new function was required to get better update timings. The function is