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

Warn child joint that resolves to world #1211

Merged
merged 4 commits into from
Dec 7, 2022
Merged

Conversation

liamhan0905
Copy link
Contributor

@liamhan0905 liamhan0905 commented Dec 7, 2022

Summary

SDFormat doesn't allow

  • [already implemented] joint child to be world
  • [new] joint child to resolve to world (e.g. joint child can be a frame that's attached to a world)

The latter is implemented in this PR

I originally created a new test with a example sdf. However, after pushing the code, I realized that there's actually a very similar TEST that checks for the same thing. Therefore the latest push cleans up all the unnecessary noise.

Test it

The first commit only contains the test so it doesn't catch when the resolved child link is the world. Therefore, it'll result in a FAIL test. But the second commit contains the logic to catch when the resolved child link is the world. Hence the test will PASS

  • checkout 1390e88f550a5a40329b433b1f2324c721b2bb9b commit and rebuild. Run the gazebo_garden/build/sdformat13/bin/INTEGRATION_joint_dom test. This should result in one test that FAILS.
  • checkout 58319f00e436bd82263de6767b269cb3d3815a17 commit and rebuild. Run the test again and this time, the test will PASS.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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

…it hasn't been implemented yet

Signed-off-by: Liam Han <liamhan0905@gmail.com>
Signed-off-by: Liam Han <liamhan0905@gmail.com>
Signed-off-by: Liam Han <liamhan0905@gmail.com>
@osrf-triage osrf-triage added this to Inbox in Core development Dec 7, 2022
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Dec 7, 2022
@liamhan0905 liamhan0905 removed the request for review from scpeters December 7, 2022 15:10
@liamhan0905 liamhan0905 changed the title Liam/resolve child link Catch child joint that resolves to world Dec 7, 2022
@liamhan0905
Copy link
Contributor Author

liamhan0905 commented Dec 7, 2022

@azeey now that I think about it, maybe this can be much simpler...

  1. I don't think I need to add another TEST since worldJointInvalidChildWorld test does pretty much the same.
  2. I won't have to add additional example sdf since it's already pretty much the same as world_joint_child_resolved_to_world sdf
  3. Maybe it's not worth adding another ErrorCode JOINT_CHILD_LINK_RESOLVED_TO_WORLD since it can just be under JOINT_CHILD_LINK_INVALID

I'm going to make the necessary changes and push again. I think it'll be much cleaner. Feel free to check out the previous commits and let me know your thoughts

Signed-off-by: Liam Han <liamhan0905@gmail.com>
@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #1211 (9a090f4) into sdf13 (a8b3f44) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            sdf13    #1211   +/-   ##
=======================================
  Coverage   87.19%   87.20%           
=======================================
  Files         124      124           
  Lines       16218    16222    +4     
=======================================
+ Hits        14142    14146    +4     
  Misses       2076     2076           
Impacted Files Coverage Δ
src/parser.cc 86.98% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@liamhan0905 liamhan0905 changed the title Catch child joint that resolves to world Warn child joint that resolves to world Dec 7, 2022
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks great! And props on realizing that we don't need another test.

/cc @scpeters

Core development automation moved this from Inbox to In review Dec 7, 2022
@liamhan0905 liamhan0905 merged commit 74b5785 into sdf13 Dec 7, 2022
Core development automation moved this from In review to Done Dec 7, 2022
@liamhan0905 liamhan0905 deleted the liam/resolve_child_link branch December 7, 2022 23:21
@scpeters
Copy link
Member

scpeters commented Dec 8, 2022

Looks great! And props on realizing that we don't need another test.

/cc @scpeters

good call; I guess I had made the test but didn't add all the checking logic. It would have been more obvious if I had split the error cases into separate files, but I was lazy. Nice job catching it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants