Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the linkFrames in case of the axis of the joint does not pass through the csys #106

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Jul 22, 2024

It fixes:

Thanks heap to @traversaro who added the required feature to idyntree.

This version of creo2urdf is able to export the urdf from the mistreated icub 2.5 simulation model, see:

@Nicogene Nicogene added the team-fix Related to Team Fix label Jul 22, 2024
@Nicogene Nicogene self-assigned this Jul 22, 2024
@Nicogene Nicogene linked an issue Jul 22, 2024 that may be closed by this pull request
@Nicogene
Copy link
Member Author

Right now I am handling the CSYS case as special case but maybe we can generalize it?

Moreover, I am always invoking moveLinkFramesToBeCompatibleWithURDFWithGivenBaseLink, in case the model is already urdf-exportable nothing happens right @traversaro ?

// Convert modelToExport in a URDF-compatible model (using the default base link)
iDynTree::Model modelToExportURDFCompatible;

bool ok = iDynTree::moveLinkFramesToBeCompatibleWithURDFWithGivenBaseLink(mdl, modelToExportURDFCompatible);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit afraid in doing this unconditionally, as it could hide some bugs in the assignment of the frames in the models in which all the link frames are assigned. Perhaps we could do this only if link frames are not assigned, or have an explicit option to enable this mode that is off on models that do not need it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could do this only if link frames are not assigned

Personally I'm in favour of this as it would avoid complicating the yaml.

@Nicogene
Copy link
Member Author

@mfussi66 @traversaro in the latest commit I changed a little bit the state flow:

  • when I get the axis from creo I return for every joints the mid point of the axis, not only for the joints where the parent link frame is csys.
  • I compute the distance of the axis from the origin of the frame and if it is greater than 0 I print a warning, I change the origin of the axis and I set m_need_to_move_link_frames_to_be_compatible_with_URDF
  • if true m_need_to_move_link_frames_to_be_compatible_with_URDF force the invocation of moveLinkFramesToBeCompatibleWithURDFWithGivenBaseLink

idyn_model.visualSolidShapes().getLinkSolidShapes()[idyn_model.getLinkIndex(urdf_child_link_name)][0]->setLink_H_geometry(oldChild_H_newChild.inverse() * link_H_visual_solid_shape);
// Check if the axis is aligned with the link frame
if ( parent_link_frame == "CSYS" || idyn_axis.getDistanceBetweenAxisAndPoint(iDynTree::Position::Zero()) > 1e-7 ) {
printToMessageWindow("The axis " + axis_name + " of "+ joint_name+ " is not aligned with the link frame " + parent_link_frame + ", the axis will be moved to the link frame in order to be urdf-compliant", c2uLogLevel::WARN);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit dubious about printing this as a warning. In the case of iCub 2.7, this is expected, and adding warning that can't be fixed is typically a good way to ensure that people actually do not check warnings. Anyhow, this is non blocking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that if is not correct, because that distance will be never > 0, I put the PR in draft until I fix it

@Nicogene Nicogene marked this pull request as draft July 29, 2024 14:57
@Nicogene Nicogene marked this pull request as ready for review August 2, 2024 09:59
@Nicogene
Copy link
Member Author

Nicogene commented Aug 2, 2024

@mfussi66, @traversaro I managed to correctly handle when moving the frames, only when it is not user-specified (i.e. CSYS) AND the axis is not passing from the csys. If the user specifies a linkframe that breaks the URDF convention, exportation will fail.

Creo2urdf exports both ergocub and iCub correctly now.

Moreover, I removed the warning, since there is already plenty of and they can be confusing for the user.

Merging!

@Nicogene Nicogene merged commit 1d2f8a9 into master Aug 2, 2024
1 check passed
@Nicogene Nicogene deleted the fixCSYSProblem branch August 2, 2024 10:02
@mfussi66
Copy link
Member

mfussi66 commented Aug 2, 2024

@mfussi66, @traversaro I managed to correctly handle when moving the frames, only when it is not user-specified (i.e. CSYS) AND the axis is not passing from the csys. If the user specifies a linkframe that breaks the URDF convention, exportation will fail.

Creo2urdf exports both ergocub and iCub correctly now.

Moreover, I removed the warning, since there is already plenty of and they can be confusing for the user.

Merging!

Awesome! I'll try that with other models

@Nicogene
Copy link
Member Author

Nicogene commented Aug 2, 2024

This feature is available in the latest release:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-fix Related to Team Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CREO2URDF – Correctly handle the joint definition when using CSYS
3 participants