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.

Sign up for GitHub

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

Jump to bottom

Set sdf::Root to contain only one model/actor/light #444

Merged
merged 7 commits into from
Jan 9, 2021
Merged

Set sdf::Root to contain only one model/actor/light #444

merged 7 commits into from
Jan 9, 2021

Conversation

brawner
Copy link
Collaborator

@brawner brawner commented Dec 15, 2020

This is a followup PR to #433. This changes sdf::Root to have only one Actor/Light/Model.

This change will introduce a hard break to the root element.
Signed-off-by: Stephen Brawner brawner@gmail.com

@brawner brawner requested a review from azeey December 15, 2020 21:29
@brawner brawner mentioned this pull request Dec 15, 2020
Merged
@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from dfd1e6b to 0d4ba3e Compare December 15, 2020 21:56
@brawner brawner force-pushed the brawner/include-one-model-sdfs branch from 3a79445 to 89ed957 Compare December 15, 2020 22:00
@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from 0d4ba3e to 6ef5385 Compare December 15, 2020 22:05
@codecov-io
Copy link

codecov-io commented Dec 15, 2020 •
edited
Loading

Codecov Report

Merging #444 (92e26e3) into master (b491e9b) will decrease coverage by 0.09%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   87.65%   87.56%   -0.10%     
==========================================
  Files          64       64              
  Lines        9681     9704      +23     
==========================================
+ Hits         8486     8497      +11     
- Misses       1195     1207      +12
Impacted Files Coverage Δ
src/Root.cc 90.05% <80.00%> (-6.45%) ⬇️
src/ign.cc 74.41% <100.00%> (ø)
src/parser.cc 79.27% <100.00%> (-0.12%) ⬇️

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 b491e9b...92e26e3. Read the comment docs.

@azeey azeey added this to the SDFormat 1.8 / libsdformat11 milestone Dec 17, 2020
@brawner brawner force-pushed the brawner/include-one-model-sdfs branch from 89ed957 to 7a019d3 Compare December 21, 2020 21:37
Base automatically changed from brawner/include-one-model-sdfs to master December 22, 2020 23:33
azeey
azeey reviewed Dec 23, 2020
View reviewed changes
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.

We usually do a tick-tock cycle on public APIs so I think we need to first deprecate the member functions in Root in libsdformat11 and then remove them in libsdformat12.

@brawner
Copy link
Collaborator Author

brawner commented Dec 23, 2020

Makes sense, I'll plan on updating this PR

@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from 40a2561 to bdd536d Compare December 28, 2020 20:40
@brawner
Copy link
Collaborator Author

brawner commented Dec 28, 2020

Deprecated the original methods and adjusted their logic for the addition of the unique_ptr. I also rebased onto master, so this should be easier to review now.

@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from 331a8f6 to 934eef8 Compare December 28, 2020 23:12
@brawner
Copy link
Collaborator Author

brawner commented Dec 28, 2020

In this last commit, I've switched model/light/actor to being stored as part of a std::variant so that the implementation better matches the sdf spec. It adds more lines than the unique_ptr variation, but better enforces the spec.

I'm happy to switch back though.

@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch 2 times, most recently from 3051f5f to 9905b92 Compare January 4, 2021 23:08
brawner
brawner commented Jan 4, 2021
View reviewed changes
src/Root.cc Outdated
/// \brief The models specified under the root SDF element
public: std::vector<Model> models;
/// Deprecated: to be removed in libsdformat12
public: std::vector<sdf::Model> models SDF_DEPRECATED(11.0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if deprecating these member variables is useful here, as compiling this file creates a bunch of warnings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I would only deprecate the public API. Stuff in RootPrivate is all private.

@brawner brawner requested a review from azeey January 4, 2021 23:10
azeey
azeey approved these changes Jan 6, 2021
View reviewed changes
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! Thanks for iterating. Could you add the deprecations to Migration.md before merging?

@brawner
Copy link
Collaborator Author

brawner commented Jan 8, 2021

Updated Migration.md

@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from 23cc0d1 to aec2335 Compare January 8, 2021 23:04
@brawner
Set sdf::Root to contain only one model/actor/light
fe0c624 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Deprecated original functionality
1506ee2 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Switch to std::variant
e81e431 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Revert previous API and logic, keeping new API
ced02fa 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Deprecate member variables
f1a4887 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Remove variable deprecation, revert root_dom integration test
c94f4ef 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Update Migration.md
92e26e3 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/set-sdf-root-one-model branch from aec2335 to 92e26e3 Compare January 8, 2021 23:07
@brawner
Copy link
Collaborator Author

brawner commented Jan 8, 2021

Had to update some older commits to fix DCO.

@brawner brawner merged commit ed198a0 into master Jan 9, 2021
@brawner brawner deleted the brawner/set-sdf-root-one-model branch January 9, 2021 00:33
@brawner brawner mentioned this pull request Jan 14, 2021
Merged
@diegoferigo diegoferigo mentioned this pull request Apr 27, 2021
Merged
@azeey azeey mentioned this pull request Jul 9, 2021
Merged
8 tasks
azeey added a commit that referenced this pull request Jul 14, 2021
@azeey
Emit an error instead of a warning when a file has multiple root level (
2d298f0 
#619)

#433 and #444 made it so that only one root level model, actor or light can be contained in an SDFormat file. So as to not break existing behavior, only a warning was issued if more than one is found. These have now been converted to errors. This PR also removes deprecated functions for accessing multiple model, actor or light elements from sdf::Root.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
SDFormat 1.8 / libsdformat11
Development

Successfully merging this pull request may close these issues.
3 participants
@brawner @codecov-io @azeey