-
Notifications
You must be signed in to change notification settings - Fork 94
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
Set sdf::Root to contain only one model/actor/light #444
Conversation
dfd1e6b
to
0d4ba3e
Compare
3a79445
to
89ed957
Compare
0d4ba3e
to
6ef5385
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
89ed957
to
7a019d3
Compare
There was a problem hiding this 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.
Makes sense, I'll plan on updating this PR |
40a2561
to
bdd536d
Compare
Deprecated the original methods and adjusted their logic for the addition of the |
331a8f6
to
934eef8
Compare
In this last commit, I've switched model/light/actor to being stored as part of a I'm happy to switch back though. |
3051f5f
to
9905b92
Compare
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
Updated Migration.md |
23cc0d1
to
aec2335
Compare
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
aec2335
to
92e26e3
Compare
Had to update some older commits to fix DCO. |
#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 <[email protected]>
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 [email protected]