Skip to content
This repository has been archived by the owner on Feb 3, 2025. It is now read-only.

Fix corruption when a URDF file is included from a SDFormat 1.6 model #2734

Merged
merged 11 commits into from
Jul 20, 2020

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented May 12, 2020

This fixes #2728 by sending the non-parsed text of the model to gzserver. Before this, when spawning a model using the GUI or the gz tool, the SDFormat/URDF file was parsed by libsdformat on the client side for error checking or previewing model placement in the world. This was also used to set a new name for the model. The parsed model was then reserialized to xml and sent to the server in a Factory message. With this PR, the parsing still happens for the same reasons, but instead of reserializing the model, the original text in the file is sent over a Factory message. To allow setting a new name, a new field ,new_name, is added to the Factory message type.

See #2728, specifically this comment for why reserialization is a problem.

@traversaro
Copy link
Collaborator

Thanks a lot @azeey for working on this! I tried this branch on my machine, but it seems that the problem #2728 is still present, I wounder if I have some local problem or there are still some problems after the fix in this PR.

@traversaro
Copy link
Collaborator

traversaro commented May 14, 2020

For reference, if instead I change https://github.com/osrf/gazebo/pull/2734/files#diff-b8479763685d74e12aa126230aae05ccR112 from:

sdf::readFileWithoutConversion(_filename, this->dataPtr->modelSDFUnconverted,
          _filename, this->dataPtr->modelSDF, errors))
                                 errors);

to

sdf::readFile(_filename, this->dataPtr->modelSDFUnconverted,
          _filename, this->dataPtr->modelSDF, errors))
                                 errors);

the behaviour is:
gazebo_fix

so it seems that the preview is malformed, but the final spawned model is correct.

@azeey azeey marked this pull request as draft May 14, 2020 17:05
@azeey
Copy link
Collaborator Author

azeey commented May 14, 2020

@traversaro Apparently I had set the sdf version to 1.7 in my model_sdf_1_6 model for testing and forgot to change it back. As you observed, the problem is still there.

azeey added 3 commits May 22, 2020 15:09
poses even if there are errors when loading DOM objects.

Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@EricCousineau-TRI
Copy link

Per VC, Addisu has some tests that should show the fix, and then this will turn into a non-draft PR.

azeey added 2 commits June 26, 2020 00:27
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey marked this pull request as ready for review June 26, 2020 05:45
@azeey azeey requested a review from scpeters June 26, 2020 05:48
@azeey
Copy link
Collaborator Author

azeey commented Jun 26, 2020

I've added tests that exercise spawning a nested URDF from the GUI as well as using the gz tool. @traversaro, do you mind reviewing the changes I made? BTW, thanks for providing the sample files.

@traversaro
Copy link
Collaborator

Thanks a lot, I am checking them now.

Copy link
Collaborator

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

The PR seems great, especially the GUI tests. However, it would be useful if the PR contained a small description about the fix (just a sentence, nothing to elaborate) as I far as I understand:

This fixes #2728 by keeping an extra model that is read in without up-conversion. This is then used for serializing and sending the model to gzserver.

Is not relevant anymore.

@azeey
Copy link
Collaborator Author

azeey commented Jun 26, 2020

Thanks for the review @traversaro . I've updated the PR description.

@azeey azeey mentioned this pull request Jun 26, 2020
3 tasks
Also, reverts addition of a new field in the Factory message.

Signed-off-by: Addisu Z. Taddese <[email protected]>
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Quick look at the code and added some questions.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@scpeters scpeters added the 11 Gazebo 11 label Jul 15, 2020
@j-rivero j-rivero merged commit e7fdfcd into gazebosim:gazebo11 Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
11 Gazebo 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URDF included in SDF 1.6 is silently corrupted in Gazebo 11 when spawned from the GUI
5 participants