-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add support for nested models in TPE #86
Conversation
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
@azeey so now we want to declare that At a minimum, we should document the new restriction on the |
Signed-off-by: Ian Chen <[email protected]>
done. added comment in ef6503e |
Signed-off-by: Steve Peters <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-physics2 #86 +/- ##
================================================
+ Coverage 83.28% 83.43% +0.15%
================================================
Files 106 106
Lines 3804 3900 +96
================================================
+ Hits 3168 3254 +86
- Misses 636 646 +10
Continue to review full report at Codecov.
|
@iche033 I've made a prerelease of sdformat 9.3.0; to use prereleases in CI for this PR, we need the following: gazebo-tooling/gzdev#16 |
@osrf-jenkins run tests please |
@osrf-jenkins run tests please |
CI looks good with the prerelease, except for the slight decrease in code coverage |
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
////////////////////////////////////////////////// | ||
Entity &Model::GetCanonicalLink() | ||
{ | ||
std::set<Model *> models; |
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.
will a std::set
maintain the same order of nested models?
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 don't think so. Similarly, all the children (links and nested mdoels) in a TPE Model are stored in an std::map
which also does not guarantee order. Will that be an issue?
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.
the spec calls for the first link to be the canonical link if the attribute //model/@canonical_link
is unspecified, so if this ignores the xml order then the choice of canonical link may not be consistent
it's a bit pedantic I guess. we can make an issue for it ("TPE canonical link choice may be inconsistent") instead of spending more time on it
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.
ok I added some logic to mark the first link added to the model as the canonical link, and also added a todo note about handling (or preventing) removal of this link. The order in which the links are added should be consistent with the xml order so the canonical link should also be consistent now. d2a9481
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.
the handling of links looks good now. for a model with no links and multiple nested models, I'm not sure that it will always pick the same model since it is storing them in a std::set
, but I'm fine with adding a todo
comment and opening an issue instead of iterating further
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.
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
I think we can ignore the abichecker failure, since it was not configured to use prereleases. It should be fixed by gazebo-tooling/release-tools#273, but we don't need to wait for that. |
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.
looks ok to me, though I haven't tested it
Signed-off-by: Ian Chen <[email protected]>
@osrf-jenkins run tests please |
Depends on gazebosim/sdformat#316
This PR adds support for nested models in TPE plugin (dartsim plugin does not support nested models yet).
Example sdf description of a nested model:
The main changes are: