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

Skip serializing nested model with //pose/@relative_to attribute #1454

Merged
merged 6 commits into from
May 3, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 20, 2022

Signed-off-by: Ian Chen ichen@osrfoundation.org

🦟 Bug fix

Not a bug but suppresses misleading error messages.
Related issue: #1071

Summary

Details are described in #1071. This PR prevents serialization of a model component if it has the //pose/@relative_to attribute to avoid flooding the console with error messages when there are multiple models with nested models using this attribute. Since model deserialization with //pose/@relative_to never worked, this should not affect existing behavior. I believe the model serialization / deserialization functionality is mainly used by the component editor.

More info:

Problem is that during ign-gazebo serialization, a nested model is treated as an individual (top level) model (wrapped by <sdf> tag) and serialized to string. However, when ign-gazebo tries to deserialize it, sdformat sees it a top level model so the //pose/@relative_to attribute causes root.LoadSdfString to fail. It then produces these error messages:

Error:   Could not find the 'robot' element in the xml file
         at line 80 in /build/urdfdom-YMMa9X/urdfdom-1.0.0/urdf_parser/src/model.cpp
Error [parser_urdf.cc:3255] Unable to call parseURDF on robot model
Error [parser.cc:820] parse as old deprecated model file failed.
[GUI] [Err] [Model.hh:73] Unable to unserialize sdf::Model

I think sdformat is doing its job correctly when it fails to parse the sdf string. Maybe we need a way for ign-gazebo to tell sdformat to skip this check, or have a better serialization / deserialization method for [nested] models.

I also changed the verbosity level for serialization / serialization issue from error to warning.

See #1071 on how to reproduce the issue.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 requested a review from chapulina as a code owner April 20, 2022 23:15
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Apr 20, 2022
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1454 (1ef1c55) into ign-gazebo6 (d57d9de) will not change coverage.
The diff coverage is n/a.

❗ Current head 1ef1c55 differs from pull request most recent head 716512d. Consider uploading reports for the commit 716512d to get more accurate results

@@             Coverage Diff              @@
##           ign-gazebo6    #1454   +/-   ##
============================================
  Coverage        33.58%   33.58%           
============================================
  Files               44       44           
  Lines             2260     2260           
============================================
  Hits               759      759           
  Misses            1501     1501           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d57d9de...716512d. Read the comment docs.

@scpeters
Copy link
Member

once gazebosim/sdformat#820 is fully resolved, there should be an API that returns sdf::Errors objects instead of printing console messages, so it would be easier to ignore specific errors.

@scpeters
Copy link
Member

scpeters commented May 2, 2022

once ignitionrobotics/sdformat#820 is fully resolved, there should be an API that returns sdf::Errors objects instead of printing console messages, so it would be easier to ignore specific errors.

this was just an FYI, I think this is fine to merge

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor Author

iche033 commented May 2, 2022

once gazebosim/sdformat#820 is fully resolved, there should be an API that returns sdf::Errors objects instead of printing console messages, so it would be easier to ignore specific errors.

I added this comment to the code as a reminder. 91d77a5

@iche033 iche033 merged commit c756b60 into ign-gazebo6 May 3, 2022
@iche033 iche033 deleted the skip_serialize_model branch May 3, 2022 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants