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

Root to element #842

Merged
merged 13 commits into from
Mar 5, 2022
Merged

Root to element #842

merged 13 commits into from
Mar 5, 2022

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Feb 2, 2022

🎉 New feature

Summary

Adds Root::ToElement, similar to the other ToElement functions. This does change the API of World::ToElement, which I think is okay because sdf12 has not been released with the original World::ToElement function.

Test it

Run the tests.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (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.

Nate Koenig added 6 commits January 26, 2022 13:45
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig nkoenig changed the base branch from sdf12 to root_mutable_accessors February 2, 2022 21:42
@osrf-triage osrf-triage added this to Inbox in Core development Feb 2, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Feb 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #842 (9309982) into sdf12 (9641718) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 9309982 differs from pull request most recent head 093e6a8. Consider uploading reports for the commit 093e6a8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf12     #842      +/-   ##
==========================================
+ Coverage   88.48%   88.51%   +0.02%     
==========================================
  Files          92       92              
  Lines       14190    14222      +32     
==========================================
+ Hits        12556    12588      +32     
  Misses       1634     1634              
Impacted Files Coverage Δ
src/Model.cc 92.72% <100.00%> (+0.16%) ⬆️
src/Root.cc 95.12% <100.00%> (+0.58%) ⬆️
src/World.cc 95.02% <100.00%> (ø)

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 9641718...093e6a8. Read the comment docs.

@scpeters scpeters moved this from Inbox to In review in Core development Feb 7, 2022
Base automatically changed from root_mutable_accessors to sdf12 February 16, 2022 20:13
@azeey
Copy link
Collaborator

azeey commented Feb 18, 2022

Can you resolve the conflicts?

Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig
Copy link
Contributor Author

nkoenig commented Mar 3, 2022

Can you resolve the conflicts?

Resolved.

@nkoenig nkoenig mentioned this pull request Mar 3, 2022
7 tasks
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! My main question is regarding plugins

include/sdf/Root.hh Show resolved Hide resolved
src/Model.cc Show resolved Hide resolved
Nate Koenig and others added 4 commits March 3, 2022 17:03
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig nkoenig requested a review from azeey March 4, 2022 16:20
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.

LGTM!

Since using ToElement(true) won't capture the effects of the param passing functionality, it would be good to mentiona that in ToElement's documentation.

One overall observation is that we have recently made efforts to avoid printing default elements and attributes (see #575) when calling sdf::Element::ToString. However, this currently doesn't work if the ElementPtr is obtained using ToElement. Should I ticket an issue?

Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig
Copy link
Contributor Author

nkoenig commented Mar 4, 2022

Since using ToElement(true) won't capture the effects of the param passing functionality, it would be good to mentiona that in ToElement's documentation.

I've add documentation to the ToElement functions.

One overall observation is that we have recently made efforts to avoid printing default elements and attributes (see #575) when calling sdf::Element::ToString. However, this currently doesn't work if the ElementPtr is obtained using ToElement. Should I ticket an issue?

Yes please.

@nkoenig nkoenig enabled auto-merge (squash) March 4, 2022 18:09
@nkoenig nkoenig disabled auto-merge March 4, 2022 23:14
@nkoenig nkoenig merged commit c496c5b into sdf12 Mar 5, 2022
Core development automation moved this from In review to Done Mar 5, 2022
@nkoenig nkoenig deleted the root_to_element branch March 5, 2022 00:40
@azeey
Copy link
Collaborator

azeey commented Mar 5, 2022

Ticketed #867

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
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.

None yet

4 participants