Skip to content

SDF to USD: check and fix if path is valid#976

Merged
ahcorde merged 7 commits intosdf12from
ahcorde/sdf_to_usd/fix_path
Apr 14, 2022
Merged

SDF to USD: check and fix if path is valid#976
ahcorde merged 7 commits intosdf12from
ahcorde/sdf_to_usd/fix_path

Conversation

@ahcorde
Copy link
Copy Markdown
Collaborator

@ahcorde ahcorde commented Apr 13, 2022

Signed-off-by: Alejandro Hernández ahcorde@gmail.com

🦟 Bug fix

Fixes #

Summary

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.

@ahcorde
SDF to USD: check and fix if path is valid
7c5ee5b 
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@ahcorde ahcorde added the usd label Apr 13, 2022
@ahcorde ahcorde requested review from azeey and scpeters as code owners April 13, 2022 15:04
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Apr 13, 2022
ahcorde and others added 2 commits April 13, 2022 18:28
@ahcorde
make linters happy
acb26f2 
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@adlarkin
review feedback
78b661a 
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
adlarkin
adlarkin reviewed Apr 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! One question though - it looks like there's still a place in the sdf -> usd geometry parser where we are manually replaceing dashes with underscores: https://github.com/ignitionrobotics/sdformat/blob/78b661a598c54b1530c9abea63a58a26f1b99128/usd/src/sdf_parser/Geometry.cc#L353

Should we update the validPath method to also check for dashes, and then replace the line I linked above to use validPath?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 13, 2022 •
edited
Loading

Codecov Report

Merging #976 (27832c3) into sdf12 (0fa9e78) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            sdf12     #976   +/-   ##
=======================================
  Coverage   65.38%   65.38%           
=======================================
  Files           2        2           
  Lines          26       26           
=======================================
  Hits           17       17           
  Misses          9        9

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 0fa9e78...27832c3. Read the comment docs.

ahcorde added 2 commits April 13, 2022 20:38
@ahcorde
feedbac
6d7f212 
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
@ahcorde
Merge branch 'sdf12' into ahcorde/sdf_to_usd/fix_path
a36a656
@ahcorde
Copy link
Copy Markdown
Collaborator Author

ahcorde commented Apr 13, 2022

@adlarkin Done here 6d7f212

@ahcorde ahcorde requested a review from adlarkin April 13, 2022 18:39
@ahcorde ahcorde mentioned this pull request Apr 13, 2022
Closed
@adlarkin
make validPath method check for dashes
27832c3 
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@ahcorde ahcorde merged commit b45fcd0 into sdf12 Apr 14, 2022
@ahcorde ahcorde deleted the ahcorde/sdf_to_usd/fix_path branch April 14, 2022 08:20
@ahcorde ahcorde mentioned this pull request Apr 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azeey azeey Awaiting requested review from azeey azeey is a code owner

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

1 more reviewer

@adlarkin adlarkin adlarkin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ahcorde ahcorde

@adlarkin adlarkin

Labels

🏯 fortress Ignition Fortress usd

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ahcorde @codecov-commenter @adlarkin @j-rivero @osrf-triage